GNOME Bugzilla – Bug 760211
Allow undo when removing tracks from playlists
Last modified: 2018-01-10 14:47:58 UTC
If someone removes a track from a playlist, we should offer them the opportunity to undo: the removal might be a mistake. The standard way to do this is with an in-app notification that includes an Undo button. There are mockups for this here: https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/music/wire-playlists.png
Allan Day i want to work on this issue, if anyone is already working on it please let me know.
if a user remove more than one song than how notification message should look like??, in the mockups it is given for a single song.
@Allan Day: Please answer these question 1: if a user select more than 1 song how undo notification should look like?? 2: The position of songs in the playlist can be as they are added right now or does not change?? 3. if one of the songs among them is currently playing will that be resume after undo, or leave the behaviour like as they are now added??
(In reply to Prashant Tyagi from comment #3) > @Allan Day: Please answer these question > > 1: if a user select more than 1 song how undo notification should look like?? You would offer a single undo action for each remove action. If an action removes several tracks, the in-app notification would say something like "3 tracks removed", and the undo button would revert removing all three. > 2: The position of songs in the playlist can be as they are added right now > or does not change?? I'm not sure whether I fully understand the question. If someone undos removing the tracks, they should be restored to their original positions in the playlist. > 3. if one of the songs among them is currently playing will that be resume > after undo, or leave the behaviour like as they are now added?? I wouldn't expect removing the currently playing track from the playlist to stop playback.
Created attachment 354407 [details] [review] Undo song removal from playlist Here is a first attempt to cover that feature. It seems to work well but I am not exactly sure this is the right approach. Any suggestions are welcome.
Review of attachment 354407 [details] [review]: Thanks for the patch, apologies for the delayed review. This looks pretty good, but at first glance it has one major issue. I won't go into too much details, but my main gripe with the approach is that the logic for the deletion is in window.py, which is (or should be) just about the UI (specifically under the comment '# Remove the song(s)'). I think the actual removal logic (and state) should be kept in playlists.py, window.py should be kept agnostic to whatever is being deleted. So it should just call something like 'playlist.finish_pending_removals()'.
Also, the new strings need proper plural handling: https://wiki.gnome.org/TranslationProject/DevGuidelines/Plurals
(In reply to Marinus Schraal from comment #6) > Review of attachment 354407 [details] [review] [review]: > > Thanks for the patch, apologies for the delayed review. > > This looks pretty good, but at first glance it has one major issue. > > I won't go into too much details, but my main gripe with the approach is > that the logic for the deletion is in window.py, which is (or should be) > just about the UI (specifically under the comment '# Remove the song(s)'). I > think the actual removal logic (and state) should be kept in playlists.py, > window.py should be kept agnostic to whatever is being deleted. So it should > just call something like 'playlist.finish_pending_removals()'. In fact this approach is a "copy-paste" of the playlist removal approach. So, maybe, I should first refactor the playlist removal code. But I'm not sure I understand your comment. From the playlist removal code, we have got the following states - 'pl_to_delete' and 'pl_to_delete_index' in playlistview.py - '_playlist_notification_timeout_id' in window.py Should they be moved to playlists.py ? Thanks.
Apologies for the delayed response. (In reply to Jean Felder from comment #8) > In fact this approach is a "copy-paste" of the playlist removal approach. > So, maybe, I should first refactor the playlist removal code. That would be a good idea, maybe the code can be refactored so it can transparently do both playlist and song removal undo. Removing the need of duplication. > > But I'm not sure I understand your comment. > From the playlist removal code, we have got the following states > - 'pl_to_delete' and 'pl_to_delete_index' in playlistview.py > - '_playlist_notification_timeout_id' in window.py > > Should they be moved to playlists.py ? I looked at it briefly and the Playlists object is really a mess, it really isn't a state object as far as most playlists go (it only keeps state for static playlists). It just manipulates tracker directly. This isn't necessarily bad, but should probably be abstracted away so more generic playlist handling code is in the Playlists object. So the view keeps state currently (which is really a no-no), but a full rewrite of the playlist code is maybe a bridge too far at this point (and there is more plumbing to be done beforehand), so keeping the state in playlistview.py is ok for now. So I guess the state should be kept there and have some functionality added to it (eg. view[3].finish_pending_removals()). As far as the timeout goes, that is fine to keep in window.py. It really is the timeout of the notification widget that drives the actual removal. It should just be agnostic to whatever is removed.
Created attachment 357628 [details] [review] Undo song removal from playlist v2 Here is a new version: - undo removal is now agnostic with the introduction of 3 functions: undo_pending_removals, finish_pending_removal and get_removal_notification_label - unique notification widget - window.py is kept clean (as far as possible...) - plural handling for the new strings - songs_count is now correct when aborting songs removal
Review of attachment 357628 [details] [review]: Not a full review yet, but looks pretty ok to me. Eventually we should probably have a generic notification pop-up handler class or something. ::: gnomemusic/views/playlistview.py @@ +473,3 @@ + """Revert the last playlist removal + or the last song(s) removal + """ for docstrings follow pep-8 (line length 72) @@ +484,3 @@ + elif self._songs_todelete_pl is not None: + if (self._current_playlist + and self._songs_todelete_pl.get_id() == self._current_playlist.get_id()): line length 80 (pep8) ::: gnomemusic/window.py @@ +370,1 @@ """ Missing space at least, but maybe better reword it so it becomes clearer. @@ +370,3 @@ """ # Callback to remove playlists This comment needs updating, but is kind of spurious.
Created attachment 357724 [details] [review] Undo song removal from playlist v3 pep8 fixes.
Created attachment 364176 [details] [review] notification: create popup notification manager (popup and loading) Allow undo when removing tracks from playlists (fix #760211) Create a generic notification handler in notification.py
Review of attachment 364176 [details] [review]: First glance this looks pretty awesome. I need some time to go through it thoroughly.
Review of attachment 364176 [details] [review]: Overall it's a great cleanup. There are some minor code points (naming/comments/etc). There's also the issue of singletons and how the class is split, I think it should be possible to do better there. We could possibly discuss it on IRC and see what the options are. ::: gnomemusic/notification.py @@ +95,3 @@ + @classmethod + def get_default(cls): + pass Can we avoid these classes being singletons? @@ +104,3 @@ + + def _setup_view(self): + self.revealer = Gtk.Revealer(halign=Gtk.Align.CENTER, Python isn't very strict about this, but I prefer to have the revealer internal and provide it as a property. @@ +117,3 @@ + + @log + def show_notification(self, message): This is specific for the subclass and depending on the class it does 2 different things. To me the baseclass should just be the common code or interfaces. The name could just be 'show' @@ +134,3 @@ + + @log + def push_loading_notification(self): subclass specific the name could just be 'push' (eg. LoadingNotification.push) @@ +138,3 @@ + + @log + def pop_loading_notification(self): subclass specific @@ +156,3 @@ + + def __init__(self): + PopupNotificationManager.__init__(self) python3 super().__init__() @@ +158,3 @@ + PopupNotificationManager.__init__(self) + + # Add label and undo button Spurious comment, it's obvious from the code. @@ +179,3 @@ + + # hide notification and remove playlist + # if one notification already exists unneeded comment @@ +180,3 @@ + # hide notification and remove playlist + # if one notification already exists + self._hide_notification('finish-removal') Shouldn't the signal emission be just here? @@ +182,3 @@ + self._hide_notification('finish-removal') + + # display notification message -comment @@ +186,3 @@ + self.revealer.set_reveal_child(True) + + # add timeout -comment and the same issues go for LoadingNotification ::: gnomemusic/window.py @@ +103,3 @@ self._setup_view() + self._overlay.add_overlay(loading_notification.revealer) + self._overlay.add_overlay(playlist_notification.revealer) I'm not sure this is the right approach. Although I don't have an alternative off the bat here.
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gnome-music/issues/50.