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

Play Order rework #1411

Closed
lazka opened this Issue Mar 15, 2015 · 11 comments

Comments

Projects
None yet
3 participants
@lazka
Member

lazka commented Mar 15, 2015

Original issue 1411 created by reiter.christoph on 2014-06-06T18:19:31.000Z:

Meta bug for play order changes...

Current design: http://f.666kb.com/i/cozwusc91t7xzr57s.png

  • Two modes, shuffle/inorder
  • Menu to select a modifier(check item) e.g. follow cursor / variant(radio item) e.g. weighted random

We can implement that on top of the current interface by grouping all orders in two groups... a good first step I guess. For the things below we probably need two kinds of plugins, orders and order wrappers.

Feedback welcome.

Menu examples:

--------------------------- <- modifiers
| [x] follow cursor (falls back to variant if cursor doesn't change)
| [ ] queue only (only affects clicks)
| [ ] autofill queue
| [ ] one song (falls back to variant on explicit track changes)
| [ ] track repeat (submenu to select count?) (falls back to variant on explicit track changes)
|-------------------------- <- variants
| o weigthed (random only)
| o spotify (random only)
| o reverse (inorder only)
---------------------------

The queue could have:

----------
| [x] stop after empty
| [ ] consume songs (remove after played)
| [ ] shuffle (or a menu button like in the main UI but only with variants, to keep things simple)
----------

Other bugs which should be considered:

  • Issue #1407 - Spotify shuffle
  • Issue #1111 - Repeat-block based play mode modifier
  • Issue #903 - "Next" should not shuffle a new song if within a string of already played songs
  • Issue #894 - Add "party shuffle" play order
  • Issue #703 - Support un-shuffleable tracks for DJ mixes / classical pieces

Queue related:

  • Issue #442 - The Play Queue should use the Play Order setting
  • Issue #43 - rfe: stop playing once queue runs out

Implementation related:

  • Issue #575 - Play order plugins should handle song list changes
@lazka

This comment has been minimized.

Member

lazka commented Mar 15, 2015

Comment #1 originally posted by reiter.christoph on 2014-06-06T18:22:19.000Z:

spotify could also have a submenu, shuffle by artist/album/genre

@lazka

This comment has been minimized.

Member

lazka commented Mar 15, 2015

Comment #2 originally posted by nick.boultbee on 2014-06-07T08:35:11.000Z:

Sounds interesting. Still working out how / whether this covers all those Issues reports but sounds like it probably does.

To me the primary change needed would be moving from QL having no idea what's ahead of the queue, to pre-filling [the queue, or some meta-queue], which makes Issue #1407, Issue #894, Issue #703, Issue #903 much easier. That and/or keeping a proper history of songs maybe.

@lazka

This comment has been minimized.

Member

lazka commented Mar 15, 2015

Comment #3 originally posted by reiter.christoph on 2014-06-14T06:34:09.000Z:

See revision d782ec3 for the initial UI changes.

It's a bit confusing that the menu changes depending on the toggle state of the button... On the other hand it should be easier to use shuffle/inorder for new users and it's clearer which play order is a shuffle one and which is not.

Another improvement is that external interfaces (mpris, mpd) can now toggle between shuffle/inorder without QL forgetting the previous order.

@lazka

This comment has been minimized.

Member

lazka commented Mar 15, 2015

Comment #4 originally posted by nick.boultbee on 2014-06-14T09:59:55.000Z:

Looks really good, and feels much friendlier / more intuitive to newbies as well (probably!)

A few ideas:

  1. I still think that repeating is a first-class concept with variants (not just a toggle), and "one song" belongs far more with that than as a variant of in-order ordering. So repeat mode (or some better name? Essentially it's how much to play the songs we have ordered) could have options:
    • "no repeat" (as current off)
    • "one song" (moved from in-order mode - but allowing an "I'm feeling lucky" approach to shuffle any one song when combined with shuffle orderings...)
    • "playlist" / "all" (i.e. as current on mode, just repeat everything once finished)
    • "track repeat" (orthogonal to shuffle mode, thus allowing track-repeat-in-order as requested in Issue #1111)
  2. Should we merge the queue "random" toggle into this as discussed (maybe this was planned anyway)?
  3. The config.py confuses me a bit: order_shuffle and shuffle seem very related. would shuffle_mode and inorder_mode (if even necessary - see above) make more sense? Though I've not properly examined it all yet..

Anyway good stuff, excited to see this long-awaited reworking :) Let me know if there's anything I can help with.

@lazka

This comment has been minimized.

Member

lazka commented Mar 15, 2015

Comment #5 originally posted by reiter.christoph on 2014-06-14T10:45:58.000Z:

  1. So UI wise add a menu to the repeat button as well? Sounds good. This is how banshee does it IIRC.
  2. The main question here if it should inherit the order or have it's own (like now but maybe provide all of them). I tend to the latter. Also see the menu example above.
  3. "order_shuffle" and "order" are the order names, "shuffle" is which of them is active. If you find the naming confusing, feel free to change it, but we need all three afaics.
@lazka

This comment has been minimized.

Member

lazka commented Mar 15, 2015

Comment #6 originally posted by nick.boultbee on 2014-06-14T13:42:37.000Z:

  1. Cool, agreed
  2. I like the simplicity of one ordering mode at any one time, but yes, I think too many users already exploit the power of separate controls (use-case: play a whole album in order by queueing it, then revert to shuffling your current songlist - I do this sometimes). So we'd probably need to keep it.
    We could, of course, have an explicit (meta-)mode for the queue of "use main order mode" (i.e. inherit) which would allow both the newbie case, and the power-user case, at the expense of a small amount of explanation.
  3. OK. Let's wait and see. To me "order_songlist" is more explicit about what it's doing than "order" (and that's it's a sibling to "order_shuffle", not a parent). And shuffle_mode does sound clearer here IMO
@lazka

This comment has been minimized.

Member

lazka commented Mar 15, 2015

Comment #7 originally posted by nick.boultbee on 2014-06-18T20:22:48.000Z:

FWIW I've found our original discussion too: https://etherpad.mozilla.org/ql-playorder

@timpulver

This comment has been minimized.

timpulver commented Oct 22, 2015

Very hard to follow this discussion and bring in new ideas. Could you post a summary of the problem and what might be planned? @lazka

@declension declension self-assigned this Sep 20, 2016

@declension

This comment has been minimized.

Member

declension commented Sep 22, 2016

The closer I get to a refactoring path the more Kafkaesque the codebase seems. So some collected thoughts / plan:

Highlights

  • No complete rethink right now. The good ideas around pre-filling a queue e.g. #1407, #904 would make this far too big to take on right now unfortunately.
  • However, enable more powerful and flexible combinations of repeat and shuffle
  • Simplify the code where possible. It's arguable the complexity here has meant that none of the abovementioned 11 issues have been closed yet (or maybe we're lazy, I don't know)

Development plan

  • Two widgets: shuffle (order) mode, repeat mode (both with on/off) - similar to current set up, but random has menu too (see old discussions above)
  • Update prefs sections to allow these to be saved
  • Consider migration issues in prefs as a result of above
  • Some refactoring to cut through the layers / duplication in light of the above
  • Make on/off less confusing, i.e. don't adjust the menu options dynamically.
  • New class Repeat(Order) to model Repeat as an Order of its own separately.
  • A more flexible play system by using a single Order that is a composition with this new Repeat subclass wrapping, i.e. Repeat∘Shuffle. None (off state of widget) would be a pass-through for Repeat and for Shuffle could just be implemented as current OrderInOrder or equivalent. This makes more sense to me than any other scheme I considered, and would immediately solve #1111, #1836, plus a bunch of more niches uses (track repeat follow cursor, anyone?)
  • New / updated PluginHandler to deal with plugin versions of Repeat orders.
  • Update current plugins to give more appropriate names given slightly new functionality
  • Convert existing plugins (e.g. TrackRepeat) to appropriate new types, and document.
  • Rework / simplify PlayOrder, and possiubly PlaylistModel, arguably because details of play order and repeating has leaked out over our domain. So perhaps try avoid state in these, and by inverting control...
  • ...so maybe move single source of play controls nearer the origin, i.e. dependency of Player. For example, BasePlayer.next() currently delegates to PlaylistMux.next() which does some queue vs songlist logic (does this need abstracting? Probably not?) then delegates to PlaylistModel.next() which delegates to Order.next_explicit(), yet Order needs a back-reference to PlaylistModel in all of these requests. Or maybe not, let's see. But at least some more documentation / code clarification is needed here...

declension added a commit that referenced this issue Sep 25, 2016

Play order rethink #1411. Lots left but this to start:
 * Split `PlayOrderPlugin` to `ShufflePlugin` and `RepeatPlugin` marker subclasses
 * Add `Repeat` as an explicit (delegating) `Order`, and `Reorder` as the base class for shuffle-style `Order`s
  (necessary for plugin manager)
 * New generalised class `ToggledPlayOrderMenu`
 * `PlayOrder` now `PlayOrderWidget` which tries to do a little less; it's about aggregating the repeat and shuffle controls, and exposing higher level information (notably, the single composed `Order`).
 * Remove concept of repeat from Playlist, as repeating is more complex now and handled elsewhere.
 * Config: Finally keep `memory.repeat` and `memory.shuffle` together (removing `settings.repeat`)
 * Use more properties everywhere here
 * remove mixins that no longer help
 * Fix plugins to work with new structure
 * Add timeout to mpris test that never finish (still failing though)
@declension

This comment has been minimized.

Member

declension commented Sep 25, 2016

Lots of progress in play-order branch.

@declension

This comment has been minimized.

Member

declension commented Sep 28, 2016

#2043 Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment