Skip to content
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

Switch tracks by swipe on cover #496

Merged
merged 2 commits into from
Dec 17, 2023

Conversation

Koitharu
Copy link
Contributor

What is it?

  • Bugfix (user facing)
  • Feature (user facing)
  • Codebase improvement (dev facing)
  • Meta improvement to the project (dev facing)

Description of changes

Simple implementation of swipe gestures support on cover in player.
I can try to implement this feature using Carousel or ViewPager if it is ok

Fixes the following issues

#94

Any additional information

output.mp4

APK testing

None yet

Due Diligence

@OxygenCobalt
Copy link
Owner

OxygenCobalt commented Jun 30, 2023

Wow! This is not what I expected today at all. The implementation looks really good, I just have a few requests with the UX:

First, I'm unsure about how user-friendly this is in the context of the layout-h480dp layout. The cover in that layout is small, as can be seen below:
image

Skipping by swiping the cover alone would be really hard. Ideally, I think the gesture behavior should become some kind of parent ViewGroup that wraps the cover view and the song information within the layout-h480dp layout. Otherwise, it can simply wrap the cover in other layouts.

Additionally, I'd like it more if there was some kind of animated gesture indicator. Otherwise, this behavior is a bit mysterious and janky. For example, a circle with a "skip" icon inside it could grow from the end of the view and then highlight once the skip threshold was met. This is a lot like the "Swipe to Refresh" pattern, but just sideways.

There are a few other ideas I've had regarding this, but I can handle those later since they aren't really part of an MVP of #94. I just want the above issues to be resolved before I merge. If you are having trouble with working on these, I'll also be okay with merging it and then filling in the gaps myself.

Great work!

@OxygenCobalt
Copy link
Owner

Just bumping this in the case that the notification for my prior comment didn't go through @Koitharu.

@Koitharu
Copy link
Contributor Author

Koitharu commented Jul 7, 2023

I can also propose using of ViewPager2 for cover. Pros: user can see previous/next cover art while swiping

@OxygenCobalt
Copy link
Owner

OxygenCobalt commented Jul 7, 2023

I agree, but when I tried to do it once, I kept running into a visual bug when reshuffling. You can try on your end @Koitharu.

@Koitharu
Copy link
Contributor Author

Koitharu commented Jul 8, 2023

Closed in favor of #503

@Koitharu Koitharu closed this Jul 8, 2023
@OxygenCobalt OxygenCobalt mentioned this pull request Dec 17, 2023
3 tasks
@OxygenCobalt OxygenCobalt reopened this Dec 17, 2023
@OxygenCobalt
Copy link
Owner

Reopening since #503 turned out to not to create high-quality UX.

@OxygenCobalt OxygenCobalt merged commit b0dd13b into OxygenCobalt:dev Dec 17, 2023
1 check failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants