Fix/tapping sliders#1855
Open
kissu wants to merge 5 commits into
Open
Conversation
Contributor
|
Just a little heads up, we deliberately implemented a 'tap to volume step up/down' to make sure we do not accidentally blast music at 100%. On desktop, we do allow clicking to a certain volume level. The changes for 'seek' do make sense here |
Author
|
Yeah I commented on that topic here, thought that it would be maybe intentional. Yet the increment was only 2 by 2. Maybe we should bump it to 10 pts per step instead? Anyway, very happy to revert the tap on mobile if it makes more sense here. 🙏🏻 |
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
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.
Changelog
Makes the player sliders seek/jump to the tapped position on touch, and stops the transport controls flickering during a seek.
I made a few extra fews along the way that were related to the same part of the app to smoothen the UX a little bit. 🌟
This is how it looks with all of the bugs fixed from below, the videos are here to showcase the bugs in question so that we have full context on what was wrong
final.mp4
Tapping on mobile was overall not working at all if not dragging the seek along.
original_bug.mp4
I fixed the ugly behavior on desktop where it would take the stale value.
glitchy_skipping.mp4
I wasn't sure about this one initially but then decided to just match parity with the desktop behavior, kinda safe decision I think. 🤗
This one didn't look professional because of the flickering controls on both mobile + desktop.
Technical details
Progress tap-to-seek:
@endseeked to a lazily-syncedtempTimemirror that only refreshed during a continuous drag, so a quick tap sent a stale value.Now it uses the value from Vuetify's
@endevent (the committed position).Post-seek bounce: between the seek command and the backend's
queue_time_updatedconfirmation, the elapsed-time watcher wrote the stale pre-seek value back.The slider is now held optimistically at the target until the backend's elapsed catches up (±2s) or a 3s safety timeout.
Volume tap: on touch,
onTouchEndstepped volume up/down based on which side of the thumb was tapped, instead of jumping. Desktop already jumps via the reka-ui slider's@update:model-value. Touch taps now jump to the tapped position too (reusing the existinggetPercentageFromXhelper). Drag and the group-popout tap are unchanged.Control-bar flicker:
play_action_in_progressblips true for ~50ms during a seek (the stream restarts). Five buttons each read it directly to drive their spinner / disabled state. Extracted into a sharedusePlayActionInProgresscomposable that debounces it (200ms), so the spinner only shows for genuinely slow actions.Refactor in commit #3
Removing
tempTimeletcomputedElapsedTimedrop a mutation it performed inside the getter (tempTime.value = curTimeValue.value, previously suppressed with aneslint-disable).Mostly wanted to avoid doing the Vue anti-pattern there.
Commits details
I split my work across 3 commits so that any could be reverted if not considered atomic enough (or just outside of the scope of this PR).
We could argue that this PR is "doing too much", yet I wanted to leave it better than it initially was without making it too huge.
PlayerTimeline.vue: tap-to-seek + optimistic post-seek hold, plus the side-effect-freecomputedElapsedTimecleanup above.PlayerVolume.vue(jump to tapped position).Verification
Linter + typecking + tests all pass (added coverage of my new composable). ✅
mini player and the fullscreen player:
Closes music-assistant/backlog#67
PS: disclaimer regarding the OHF AI Policy: