Skip to content

[Refactor/#1] PickerState to index-based generic state with reliable onValueChange#2

Merged
DongChyeon merged 6 commits intomainfrom
refactor/#1-pickerstate-index-based
Jul 2, 2025
Merged

[Refactor/#1] PickerState to index-based generic state with reliable onValueChange#2
DongChyeon merged 6 commits intomainfrom
refactor/#1-pickerstate-index-based

Conversation

@DongChyeon
Copy link
Copy Markdown
Owner

🎯 Related Issue


📝 Description

What does this PR do?

Refactor PickerState to use index-based unidirectional state management for better consistency with Compose paradigms.
Make PickerState generic for type safety and reusability.
Refactor PickerItem scroll observer logic to calculate adjustedIndex before distinctUntilChanged, preventing skipped onValueChange triggers.
Ensure getItemForIndex returns an empty string when item is null to avoid displaying "null".


🔄 Comparison

Before After
PickerState stored mutable selectedItem directly Uses selectedIndex: StateFlow<Int> with derived selectedItem getter, enforcing unidirectional data flow
Only supported String items Now supports generic types with type-safe selection
distinctUntilChanged applied to centerIndex, mapping afterwards Calculates adjustedIndex before distinctUntilChanged, preventing skipped updates
getItemForIndex used toString() directly, risking "null" display Returns empty string "" safely when item is null

✅ Changes

  • Refactor PickerState to index-based unidirectional state
  • Make PickerState generic for type safety
  • Calculate adjustedIndex before distinctUntilChanged in PickerItem
  • Return empty string when item is null in getItemForIndex

🔍 Screenshots / Test Results (if applicable)

N/A


👤 Reviewer Checklist

N/A

@DongChyeon DongChyeon requested a review from Copilot July 2, 2025 07:15
@DongChyeon DongChyeon self-assigned this Jul 2, 2025
@DongChyeon DongChyeon added the enhancement New feature or request label Jul 2, 2025
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Refactors the picker components to use an index-based, generic state model and fixes skipped value-change callbacks by calculating adjusted indices before deduplication.

  • Converts PickerState to a generic, index-driven design with StateFlow<Int>
  • Updates PickerItem to compute adjustedIndex before distinctUntilChanged and safely formats items
  • Adapts TimePicker to supply item lists as typed List<T> and use itemFormatter for padding

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
timepicker/src/main/java/com/dongchyeon/timepicker/ui/PickerItem.kt Made PickerItem generic, refactored scroll observer and onValueChange logic, added null-safe item formatting
timepicker/src/main/java/com/dongchyeon/timepicker/model/PickerState.kt Changed PickerState to generic type T, replaced mutable item with index-based StateFlow
timepicker/src/main/java/com/dongchyeon/timepicker/TimePicker.kt Switched hour/minute lists to typed lists, updated rememberPickerState calls, added itemFormatter
Comments suppressed due to low confidence (1)

timepicker/src/main/java/com/dongchyeon/timepicker/model/PickerState.kt:10

  • With the new generic and index-based implementation, consider adding unit tests for PickerState<T> and getItemForIndex to verify correct index-to-item mapping, boundary handling, and onValueChange behavior.
class PickerState<T>(

@DongChyeon DongChyeon merged commit a5e1e10 into main Jul 2, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

♻️ Refactor - PickerState to use index-based state

2 participants