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

Add Play Next & Add to Queue for songs and albums. #481

Merged
merged 5 commits into from
Aug 19, 2023
Merged

Conversation

ray-kast
Copy link
Contributor

@ray-kast ray-kast commented Aug 7, 2023

I've seen that the UI redesign project intends to cover this, but I thought I'd offer this as a quality-of-life backport for the current UI until that gets released. This partially covers #221, and maybe covers #92 depending on opinion. Apologies for any style errors (and for the faux pas of swinging in out of the blue with a PR), my Dart is pretty rusty.

This has been tested on iOS Simulator and an iPhone 11 running iOS 16.6.

Open questions and future work:

  • There is no i18n for this, I'm monolingual
  • The icon is confusing but Icons doesn't really offer anything better than queue_music
  • There are no toolbar items in the album screen for queuing, seemed like too big of a UX question for the scope of this PR
  • Shuffle is horribly broken (FIXED)
    • The existing Add to Queue behaves badly with shuffle enabled (is there an open issue for this?), and the added ones behave worse see linked comment
    • What is the expected behavior when the queue is modified and then shuffle is enabled/disabled?
    • Does it make sense (and is it worth the effort) to calculate the preimage of shuffleIndices when modifying the queue with shuffle enabled?

@ray-kast ray-kast marked this pull request as draft August 7, 2023 07:07
@Chaphasilor
Copy link
Collaborator

Regarding shuffle, yeah it's a mess, mostly due to the way the queue is handled in just_audio/audio_service. Given that it's added functionality it's probably fine if there are issues.

Would you mind posting screenshots of how the user interface for this looks? Can't build it at the moment :)

@ray-kast
Copy link
Contributor Author

ray-kast commented Aug 7, 2023

@Chaphasilor Here's what it looks like on my device.
small-1 small-2

@Chaphasilor
Copy link
Collaborator

@ray-kast so it's only an option for albums when long-pressing on the album in a list of albums, correct? I think that's fine for the moment :)

@ray-kast
Copy link
Contributor Author

ray-kast commented Aug 7, 2023

Okay, after delving into just_audio.dart I figured out a way to alter the shuffle behavior such that previous behavior is untouched, but inserting and appending to the queue behaves consistently.

Specifically, I just want to make sure the use case of shuffling all songs then selecting Play Next on an item doesn't just send the inserted items into queue purgatory, since I think that particular use case is a significant portion of the added value of a Play Next button.

@ray-kast ray-kast marked this pull request as ready for review August 7, 2023 10:46
@ray-kast
Copy link
Contributor Author

ray-kast commented Aug 7, 2023

Okay so it turns out I'm blind and just now noticed this discussion. I bandaid-fixed shuffle breaking everything and added all the new strings to app_en.arb, is it apropos to apply the fixes from #142 to this PR or should I close this one and contribute to the older one?

@Chaphasilor
Copy link
Collaborator

Not sure if that PR is still active. You could clone it and test it yourself, but if that implementation isn't significantly better, I think this one should be merged

@ray-kast
Copy link
Contributor Author

ray-kast commented Aug 7, 2023

Not sure if that PR is still active. You could clone it and test it yourself, but if that implementation isn't significantly better, I think this one should be merged

Checked out the branch, it doesn't merge onto the current upstream. Combed through it and identified a handful of things it does that this PR doesn't:

  • Hides queue buttons when the queue is empty (I ended up adding this as part of a fix for something else)
  • Adds a swipe gesture for Play Next on songs
  • Has different behavior when selecting Play Next when paused/stopped

The latter two seem better suited for separate UX tickets, especially in the case of handling a paused/stopped player since most actions currently don't attempt to resume playback.

@rom4nik
Copy link
Contributor

rom4nik commented Aug 8, 2023

Thanks for working on this! I've closed my PR (#142) since I don't have time to revive it, but I hope that at least the discussions there were valuable.

@ray-kast
Copy link
Contributor Author

ray-kast commented Aug 8, 2023

I hope that at least the discussions there were valuable.

For sure! I read through the comments and largely agree with the general sentiment there (and I'd like to think the code of this PR does too, given that we both landed on the same ShuffleOrder strategy). I don't know if the UI redesign project already has this covered but I'd be more than happy to help chip away at other queue-related tickets.

Copy link
Owner

@jmshrv jmshrv left a comment

Choose a reason for hiding this comment

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

These changes look good, just the discussion over addQueueItems that needs addressing :)

Comment on lines +71 to +74
@Deprecated("Use addQueueItems instead")
Future<void> addQueueItem(BaseItemDto item) async {
await addQueueItems([item]);
}
Copy link
Owner

Choose a reason for hiding this comment

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

Should this be marked deprecated? It would be more ergonomic to allow adding single items, and would remove the need to change everything that adds a single item

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My initial train of thought was the few extra characters for one less function call on the stack and a slightly more concise API surface was a worthwhile tradeoff, but I don't really want to die on that hill (and truth be told I don't know much about inlining in modern Dart) so I can drop the deprecated attributes if desired.

Comment on lines +264 to +267
@Deprecated("Use addQueueItems instead")
Future<void> addQueueItem(MediaItem mediaItem) async {
addQueueItems([mediaItem]);
}
Copy link
Owner

Choose a reason for hiding this comment

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

Same as the note on AudioServiceHelper for completeness 🙃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ditto 🫠

@jmshrv
Copy link
Owner

jmshrv commented Aug 19, 2023

Just gave this a test, it works nicely. UX will be improved in the redesign, where a more complex implementation is being made (#484).

Merging :)

@jmshrv jmshrv merged commit ad241ba into jmshrv:main Aug 19, 2023
2 checks passed
@ray-kast
Copy link
Contributor Author

Awesome! I use this app daily so I'd be more than happy to get involved with future improvements.

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.

4 participants