-
Notifications
You must be signed in to change notification settings - Fork 297
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
Queue history #1068
base: master
Are you sure you want to change the base?
Queue history #1068
Conversation
…s queues that they have created within the session. Considering how often I mess up my carefully crafted queue with a wrong button, an "undo" button helps.
@@ -125,6 +125,7 @@ public boolean onCreateOptionsMenu(Menu menu) { | |||
|
|||
menu.add(0, MENU_SHOW_QUEUE, 20, R.string.show_queue); | |||
menu.add(0, MENU_HIDE_QUEUE, 20, R.string.hide_queue); | |||
menu.add(0, MENU_PREV_QUEUE, 20, R.string.prev_queue); |
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.
Just wondering: wouldn't showing a snackbar be better?
At least, the option should be greyed out if there is no queue we can restore.
@@ -793,6 +860,14 @@ public int addSongs(Context context, QueryTask query) | |||
case MODE_PLAY: | |||
case MODE_PLAY_POS_FIRST: | |||
case MODE_PLAY_ID_FIRST: | |||
/* |
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 think it would be better to generalize this and take a snapshot every time the queue is modified: this would also help in cases where users accidental clear the whole queue.
Maybe this could be hooked up into saveActiveSongs()
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 could. However, as queues get longer, we would get longer and longer snapshots in the memory. The main point of this feature is to undo massive edits. It is relatively easy to change one song. Rebuilding an entire queue is harder. With this, I feel that a little moderation would be needed, especially with memory concerns.
this.songHistory.addFirst(new ArrayList<>(timeline)); | ||
} | ||
timeline.clear(); | ||
timeline.addAll(this.songHistory.removeLast()); |
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.
Probably not fatal, but it would be better if we checked for songs which were deleted in the meantime.
See readState()
in SongTimeline.java
I played with this idea in a different branch using the existing write/read-state code: https://github.com/vanilla-music/vanilla/tree/queue-revert The nice thing about this approach is that we can use a lot of the existing code & logic - and the increase in RAM should be minimal: We use about 12 bytes per song, so if your queue contains 100'000 items, we will use about 1.1MB of RAM to store the 'undo' version of the queue, which shouldn't be a big problem on modern android devices. |
For fat-fingered mobile users (i.e. me), it is rather easy to click a "play" button when I'm trying to build a queue, resulting in my losing work. To mitigate this, I took a page out of Windows Media Player's book and implemented a queue history. Every time the user makes a queue-resetting change that clears the queue (playing an album or playlist, for example), the current playlist is saved in the memory. Later, the user can select the "Play previous queue" option in the queue screen to cycle through the history of queues. At this point, we have just one active queue and one queue in the history (making more queues deletes the oldest one in the history), but that can easily be changed by changing the MAX_QUEUE_HISTORY variable in the SongTimeline.addSongs method.
Note that this functionality is not triggered when dequeuing songs (even when most or all of the queue is emptied). I can implement that if needed, but I didn't see the need for it
Furthermore, the single string entry has not been localized. Considering the unreliability of Google Translate and such, I figured it would be best to leave that to experts.