Closed
Conversation
Snapping behaviour is now strange, needs to be fixed before merge
I described the issue here: adrielcafe/voyager#541. Thanks to nvkleban for noting that the issue was not present in compose >1.9.0-alpha01
Cleans up the last departure code and attempts to resolve #275
Fix last departure times that end on the next day
…e-stop Search for content banners matching stops/route
Bump compose version to stable
…down Implement countdown to arrival as an option
…-location-updates Increase location update frequency
…box-lockup-clickable Make entire checkbox lockup clickable
Reviewer's GuideRefactors the custom bottom sheet implementation to wrap Compose’s new draggable APIs, introduces a dedicated SinatraSheetState, adds optional countdown-style arrival/departure text and related preferences, refines last‑departure selection logic and tests, wires route/stop-specific content banners, increases location update frequency, and updates build tooling and dependencies for newer Kotlin/Compose/Gradle/Java versions. Sequence diagram for bottom sheet drag and settle using SinatraAnchoredDraggableStatesequenceDiagram
participant User as User
participant Compose as SinatraBottomSheet
participant Modifier as sinatraAnchoredDraggable
participant State as SinatraAnchoredDraggableState
participant Draggable as DraggableState
User->>Compose: Drag sheet gesture
Compose->>Modifier: Apply sinatraAnchoredDraggable(state, orientation,...)
Modifier->>Draggable: drag(dragPriority) { dragBy(delta) }
Draggable->>State: anchoredDrag(dragPriority) { dragScope.dragBy(delta) }
State->>State: newOffsetForDelta(delta)
State->>State: anchoredDragScope.dragTo(newOffset, lastVelocity)
State->>State: update offset and lastVelocity
User-->>Compose: Release gesture with velocity
Compose->>State: settle(velocity)
State->>State: computeTarget(offset, currentValue, velocity)
alt confirmValueChange(targetValue)
State->>State: animateTo(targetValue, velocity)
else veto
State->>State: animateTo(previousValue, velocity)
end
State->>State: anchoredDrag(targetValue) { animate from offset to targetOffset }
State-->>Compose: currentValue updated at closest anchor
Class diagram for updated bottom sheet state and draggable infrastructureclassDiagram
direction LR
class SinatraAnchoredDraggableState_T_ {
- MutatorMutex dragMutex
- DraggableState draggableState
- AnchoredDragScope anchoredDragScope
- T currentValue
- T targetValue
- T closestValue
- Float offset
- Float lastVelocity
- T dragTarget
- DraggableAnchors_T_ anchors
- fun requireOffset() Float
- val isAnimationRunning Boolean
- val progress Float
- fun updateAnchors(newAnchors: DraggableAnchors_T_, newTarget: T)
- suspend fun anchoredDrag(dragPriority: MutatePriority, block: suspend AnchoredDragScope.() -> Unit)
- suspend fun anchoredDrag(targetValue: T, dragPriority: MutatePriority, block: suspend AnchoredDragScope.() -> Unit)
- suspend fun animateTo(targetValue: T, velocity: Float)
- suspend fun snapTo(targetValue: T)
- suspend fun settle(velocity: Float)
- fun newOffsetForDelta(delta: Float) Float
- companion object Saver(animationSpec: () -> AnimationSpec_Float_, confirmValueChange: (T) -> Boolean, positionalThreshold: (Float) -> Float, velocityThreshold: () -> Float)
}
class SinatraSheetState {
+ Boolean skipPartiallyExpanded
+ Boolean skipHiddenState
+ SinatraSheetValue currentValue
+ SinatraSheetValue targetValue
+ Boolean isVisible
+ Boolean isAnimationRunning
+ SinatraAnchoredDraggableState_SinatraSheetValue_ anchoredDraggableState
+ AnimationSpec_Float_ anchoredDraggableMotionSpec
+ FiniteAnimationSpec_Float_ showMotionSpec
+ FiniteAnimationSpec_Float_ hideMotionSpec
+ fun requireOffset() Float
+ fun expand() suspend
+ fun partialExpand() suspend
+ fun halfExpand() suspend
+ fun show() suspend
+ fun hide() suspend
+ internal fun animateTo(targetValue: SinatraSheetValue, animationSpec: FiniteAnimationSpec_Float_, velocity: Float) suspend
+ internal fun snapTo(targetValue: SinatraSheetValue) suspend
+ internal fun settle(velocity: Float) suspend
+ companion object Saver(skipPartiallyExpanded: Boolean, positionalThreshold: () -> Float, velocityThreshold: () -> Float, confirmValueChange: (SinatraSheetValue) -> Boolean, skipHiddenState: Boolean)
}
class SinatraDraggableAnchors_T_ {
- Map_T_Float_ anchors
+ fun positionOf(value: T) Float
+ fun hasPositionFor(value: T) Boolean
+ fun closestAnchor(position: Float) T
+ fun closestAnchor(position: Float, searchUpwards: Boolean) T
+ fun minPosition() Float
+ fun maxPosition() Float
+ val size Int
+ fun anchorAt(index: Int) T
+ fun positionAt(index: Int) Float
}
class SinatraDraggableAnchorsConfig_T_ {
+ MutableMap_T_Float_ anchors
+ infix fun T.at(position: Float)
}
class SinatraBottomSheet {
<<Composable>>
+ fun SinatraBottomSheet(sheetState: SinatraSheetState, sheetSwipeEnabled: Boolean)
}
class SinatraSheetValue {
<<enum>>
Hidden
PartiallyExpanded
HalfExpanded
Expanded
}
class DraggableState {
<<interface>>
+ fun drag(dragPriority: MutatePriority, block: suspend DragScope.() -> Unit)
+ fun dispatchRawDelta(delta: Float)
}
class DragScope {
<<interface>>
+ fun dragBy(pixels: Float)
}
class AnchoredDragScope {
<<interface>>
+ fun dragTo(newOffset: Float, lastKnownVelocity: Float)
}
class DraggableAnchors_T_ {
<<interface>>
}
class AnimationSpec_Float_ {
}
class FiniteAnimationSpec_Float_ {
}
class SinatraAnchoredDraggableModifier {
<<extension>>
+ fun sinatraAnchoredDraggable(state: SinatraAnchoredDraggableState_T_, orientation: Orientation, enabled: Boolean, reverseDirection: Boolean, interactionSource: MutableInteractionSource?) Modifier
}
SinatraSheetState "1" *-- "1" SinatraAnchoredDraggableState_SinatraSheetValue_ : uses
SinatraAnchoredDraggableState_T_ "1" o-- "1" DraggableState : exposes
SinatraAnchoredDraggableState_T_ "1" *-- "1" AnchoredDragScope : uses
SinatraAnchoredDraggableState_T_ "1" *-- "1" DraggableAnchors_T_ : anchors
SinatraAnchoredDraggableState_T_ ..> AnimationSpec_Float_ : animationSpec
SinatraSheetState ..> FiniteAnimationSpec_Float_ : showMotionSpec
SinatraSheetState ..> SinatraSheetValue
SinatraDraggableAnchors_T_ ..|> DraggableAnchors_T_
SinatraBottomSheet ..> SinatraSheetState : parameter
SinatraAnchoredDraggableModifier ..> SinatraAnchoredDraggableState_T_ : parameter
SinatraAnchoredDraggableModifier ..> DraggableState : wraps
SinatraAnchoredDraggableState_T_ ..> SinatraDraggableAnchors_T_ : default factory
SinatraDraggableAnchorsConfig_T_ ..> SinatraDraggableAnchors_T_ : builds
Class diagram for preferences, countdown time helpers, and stop time textclassDiagram
direction LR
class Preference_T_ {
<<sealed interface>>
}
class Preference_CountdownUntilArrival {
<<object>>
}
class Preference_ShowAccessibilityIconsNavigation {
<<object>>
}
class Preference_RequiresWheelchair {
<<object>>
}
class Preference_RequiresBikes {
<<object>>
}
class Preference_MaximumWalkingTime {
<<object>>
}
class Preference_Use24HourUnits {
<<object>>
}
class PreferencesRepository {
- PreferencesUnit_Boolean_ countdownUntilArrival
+ fun preference(preference: Preference_T_) PreferencesUnit_T_
}
class PreferencesUnit_T_ {
<<interface>>
+ val value Flow_T_
+ suspend fun set(value: T)
}
class TimeHelpers {
<<file>>
+ fun scheduleStartOfDay() Instant
+ fun startOfDay(timeZone: TimeZone) Instant
+ fun rememberCountdown(instant: kotlin.time.Instant) Duration
+ fun Time.toTodayInstant() Instant
+ fun Time.isInPast() Boolean
+ fun Time.isNowish() Boolean
+ fun Instant.format() String
+ fun Instant.isSameDay(timeZone: TimeZone) Boolean
+ fun Time.format() String
+ fun countdown(time: kotlin.time.Instant, negative: Boolean) String
}
class LocalScheduleTimeZone {
<<CompositionLocal TimeZone>>
}
class LocalClock {
<<CompositionLocal kotlin.time.Clock>>
}
class StopStationTime {
<<sealed interface>>
+ val stop: Stop
+ val stationTime: TimetableStationTime
+ val text String
}
class StopStationTime_Arrival {
<<data>>
}
class StopStationTime_Departure {
<<data>>
}
class TimetableStationTime {
+ val time Time
+ val approximate Boolean
}
class StationTime_Scheduled {
+ val approximate Boolean
}
class StationTime_Live {
+ val delay kotlin.time.Duration
}
class StopCard_TimeTextLogic {
<<extension>>
+ val StopStationTime.text String
}
class HorizontalPreferencesCheckboxLockup {
<<Composable>>
+ fun HorizontalPreferencesCheckboxLockup(preference: Preference_Boolean_, title: String, subtitle: String?, modifier: Modifier)
}
class UnitsPreferencesScreen {
<<Composable screen>>
+ fun Preferences()
}
class RoutingPreferencesScreen {
<<Composable screen>>
+ fun Preferences()
}
Preference_CountdownUntilArrival ..|> Preference_Boolean_
Preference_ShowAccessibilityIconsNavigation ..|> Preference_Boolean_
Preference_RequiresWheelchair ..|> Preference_Boolean_
Preference_RequiresBikes ..|> Preference_Boolean_
Preference_MaximumWalkingTime ..|> Preference_kotlin_time_Duration_
Preference_Use24HourUnits ..|> Preference_Time24HSetting_
PreferencesRepository ..> Preference_T_ : maps
PreferencesRepository "1" *-- "1" PreferencesUnit_Boolean_ : countdownUntilArrival
TimeHelpers ..> LocalClock : uses
TimeHelpers ..> LocalScheduleTimeZone : uses
StopStationTime <|-- StopStationTime_Arrival
StopStationTime <|-- StopStationTime_Departure
StopStationTime ..> TimetableStationTime : stationTime
StopStationTime_Departure ..> StationTime_Live : may use delay
StopCard_TimeTextLogic ..> StopStationTime : extension
StopCard_TimeTextLogic ..> Preference_CountdownUntilArrival : reads
StopCard_TimeTextLogic ..> TimeHelpers : uses countdown and isNowish
HorizontalPreferencesCheckboxLockup ..> Preference_Boolean_ : binds
UnitsPreferencesScreen ..> HorizontalPreferencesCheckboxLockup : uses
RoutingPreferencesScreen ..> HorizontalPreferencesCheckboxLockup : uses
Class diagram for LastDepartureForStopUseCase and related dataclassDiagram
direction LR
class LastDepartureForStopUseCase {
- MetadataRepository metadataRepository
- ServicesAndTimesForStopUseCase servicesAndTimesForStopUseCase
- RemoteConfigRepository remoteConfigRepository
- PreferencesRepository preferencesRepository
- Clock clock
- companion object CUTOFF_TIME kotlin.time.Duration
+ operator fun invoke(stopId: StopId, routeId: RouteId?) Flow_List_IStopTimetableTime__
}
class ServicesAndTimesForStopUseCase {
+ operator fun invoke(stopId: StopId): Flow_ServicesAndTimes_
}
class ServicesAndTimes {
+ val services List_Service_
+ val times List_StopTimetableTime_
}
class Service {
+ val id String
+ fun active(instant: Instant, scheduleTimeZone: TimeZone): Boolean
}
class StopTimetableTime {
+ val serviceId String
+ val routeId RouteId
+ val heading String
+ val departureTime Time
+ val last Boolean
+ fun withTimeReference(startOfDay: Instant) StopTimetableTime
}
class IStopTimetableTime {
<<interface>>
+ val serviceId String
+ val routeId RouteId
+ val departureTime Time
}
class MetadataRepository {
+ fun timeZone(): TimeZone
}
class RemoteConfigRepository {
+ fun showSchoolServices(): Flow_Boolean_
}
class PreferencesRepository_LD {
+ fun preference(preference: Preference_T_): PreferencesUnit_T_
}
class Clock {
+ fun now(): Instant
}
class RouteAndHeading {
+ val routeId RouteId
+ val heading String
}
LastDepartureForStopUseCase ..> MetadataRepository : uses
LastDepartureForStopUseCase ..> ServicesAndTimesForStopUseCase : uses
LastDepartureForStopUseCase ..> RemoteConfigRepository : uses
LastDepartureForStopUseCase ..> PreferencesRepository_LD : uses
LastDepartureForStopUseCase ..> Clock : uses
LastDepartureForStopUseCase ..> ServicesAndTimes : aggregates
LastDepartureForStopUseCase ..> RouteAndHeading : groups
ServicesAndTimes ..> Service : contains
ServicesAndTimes ..> StopTimetableTime : contains
StopTimetableTime ..|> IStopTimetableTime
ServiceTest ..> LastDepartureForStopUseCase : tests
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
SinatraDraggableAnchors,anchorAtandpositionAtare left asTODO("Not yet implemented"), which will crash at runtime if the framework starts calling these methods; either implement them (e.g., via a stable ordered backing list) or throw a more explicitUnsupportedOperationExceptionwith justification, and add a test to ensure they’re not accidentally invoked. - You’ve switched various clocks and instants over to
kotlin.time.Clock/kotlin.time.Instant(e.g.,LocalClock,MemoryCache,SharedModule) while other APIs still usekotlinx.datetime.Instantand extensions likestartOfDay; it would be safer to centralize on a single time abstraction or provide clear conversion helpers to avoid subtle type/extension mismatches later.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `SinatraDraggableAnchors`, `anchorAt` and `positionAt` are left as `TODO("Not yet implemented")`, which will crash at runtime if the framework starts calling these methods; either implement them (e.g., via a stable ordered backing list) or throw a more explicit `UnsupportedOperationException` with justification, and add a test to ensure they’re not accidentally invoked.
- You’ve switched various clocks and instants over to `kotlin.time.Clock`/`kotlin.time.Instant` (e.g., `LocalClock`, `MemoryCache`, `SharedModule`) while other APIs still use `kotlinx.datetime.Instant` and extensions like `startOfDay`; it would be safer to centralize on a single time abstraction or provide clear conversion helpers to avoid subtle type/extension mismatches later.
## Individual Comments
### Comment 1
<location> `ui/src/commonMain/kotlin/cl/emilym/sinatra/ui/widgets/bottomsheet/SinatraDraggableAnchors.kt:48-49` </location>
<code_context>
+ return anchors == other.anchors
+ }
+
+ override fun anchorAt(index: Int): T? {
+ TODO("Not yet implemented")
+ }
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Implement `anchorAt`/`positionAt` instead of leaving TODOs to avoid runtime crashes.
Because the `DraggableAnchors` gesture system will call `anchorAt`/`positionAt`, leaving them as `TODO` will cause a runtime crash whenever they’re used. Please implement them using the backing `anchors` map (e.g., via `entries.toList()[index]`) or refactor storage to maintain a stable ordered list so these lookups are O(1) and safe to call.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+48
to
+49
| override fun anchorAt(index: Int): T? { | ||
| TODO("Not yet implemented") |
There was a problem hiding this comment.
issue (bug_risk): Implement anchorAt/positionAt instead of leaving TODOs to avoid runtime crashes.
Because the DraggableAnchors gesture system will call anchorAt/positionAt, leaving them as TODO will cause a runtime crash whenever they’re used. Please implement them using the backing anchors map (e.g., via entries.toList()[index]) or refactor storage to maintain a stable ordered list so these lookups are O(1) and safe to call.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Update bottom sheet interaction, last departure selection, time handling, and preferences while upgrading tooling for the 0.18.0 release candidate.
New Features:
Bug Fixes:
Enhancements:
Build:
CI:
Tests: