-
Notifications
You must be signed in to change notification settings - Fork 97
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add a seek projection track position model #1690
Conversation
@@ -58,7 +58,7 @@ internal class PickerRotaryScrollAdapter( | |||
} | |||
|
|||
/** | |||
* Temporary implementation of RotaryScrollAdapter for PickerState | |||
* Temporary implementation of RotaryScrollAdapter for PickerState |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
I'd defer to @fstanis . I guess I'm not sure how best to model it, or whether it could be handled closer to the UI. But this design would seems nicely flexible. |
3a5f50d
to
97468c9
Compare
@@ -37,6 +38,12 @@ public object TrackPositionUiModelMapper { | |||
if (currentPositionMs == null || durationMs == null || durationMs <= 0) { | |||
return TrackPositionUiModel.Hidden | |||
} | |||
event.playbackState.seekProjection?.let { seek -> | |||
if (seek > Duration.ZERO) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would maybe merge this with the line above: event.playbackState.seekProjection?.takeIf { it > Duration.ZERO }?.let { seek -> ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -16,6 +16,7 @@ | |||
|
|||
package com.google.android.horologist.media.ui.state.model | |||
|
|||
import android.os.SystemClock |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should avoid android-specific dependencies in models if possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed these get properties from models.
when (this) { | ||
is Actual -> duration | ||
is SeekProjection -> duration | ||
is Predictive -> predictor.predictDuration(SystemClock.elapsedRealtime()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes these methods harder to test, since they're relying on the global clock. I'd make this method take elapsedRealtime: Long
as a parameter - but ideally, this logic shouldn't be in the model itself but in the call site.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed these get properties from models.
WHAT
Add a new type of
TrackPositionUiModel
for displaying seeking projection.WHY
This gives clients an opportunity to show seek projection in addition to the Actual, Loading, Predictive ui models. For example, this could enable
PlayerRepositoryImpl
to accumulate seeking duration and displaying it without making multiple repeated calls toseekBack
orseekForward
on long press and only flush the position on finger up.HOW
When the seek projection is passed in (as duration), use the
TrackPositionUiModel
Checklist 📋