GNOME Bugzilla – Bug 744822
Rename playlists
Last modified: 2017-12-19 08:51:06 UTC
User should be able to rename playlists after creation (either by double-clicking on playlist name in sidebar, or via dropdown menu (as an option alongside 'play' and 'delete').
Designs for this: https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/music/wire-playlists-editing.png
just adding a rename option in the dropdown menu (as an option alongside 'play' and 'delete') will solve it ??
(In reply to pankaj from comment #2) > just adding a rename option in the dropdown menu (as an option alongside > 'play' and 'delete') will solve it ?? no. You will also need to update the backend (Tracker) too. For that, you'd need to create a sparql query to rename the playlist.
Removing the target-milestone - 3.18 is over.
The mockups for playlist renaming can now be found here: https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/music/wire-playlists.png
Created attachment 320816 [details] [review] Patch for playlist rename Hi Felipe. This patch will allow user's to rename their created playlists. This is my first time contributing to GNOME. Is there any other change required in this ?
Review of attachment 320816 [details] [review]: Thank you for your patch. It is much appreciated! Firstly, where are you exposing the _Rename_ feature? I don't see an entry in the playlist menu (playlistMenu at data/PlaylistControls.ui) (as it was supposed to be according to the mockup[0]). Secondly, You seem to be creating a specific dialog for that. According to [0], you should be placing a GtkEntry on top of the playlist_name label (see data/PlaylistControls.ui). I would recommend having a GtkStack which would have as a child: the GtkLabel that we currently have (playlist_name) and the renaming box (a GtkEntry + the [DONE] GtkButton). [0] https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/music/wire-playlists.png
(In reply to Felipe Borges from comment #7) > Review of attachment 320816 [details] [review] [review]: > Firstly, where are you exposing the _Rename_ feature? I don't see an entry > in the playlist menu (playlistMenu at data/PlaylistControls.ui) (as it was > supposed to be according to the mockup[0]). I am so sorry. I added that change but forgot to commit it. My bad. Will add it in the next patch. > Secondly, You seem to be creating a specific dialog for that. According to > [0], you should be placing a GtkEntry on top of the playlist_name label (see > data/PlaylistControls.ui). > I would recommend having a GtkStack which would have as a child: the > GtkLabel that we currently have (playlist_name) and the renaming box (a > GtkEntry + the [DONE] GtkButton). > > [0] > https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/ > music/wire-playlists.png Ok. I'll create a new patch fixing the above. Thanks.
Yash if you already working on it please let me know i am also interested on working on this bug.
(In reply to Prashant Tyagi from comment #9) > Yash if you already working on it please let me know i am also interested > on working on this bug. Yes I am. I plan on submitting the patch today. :)
Created attachment 321719 [details] [review] Patch for playlist rename This is the updated patch according to the mockups.
Review of attachment 321719 [details] [review]: Thank you for your patch. I noticed that when you try to rename a playlist after had renamed another one, the GtkEntry has the previous playlist name within. Reset the GtkEtnry after Done is pressed. For now, that's all. ::: data/PlaylistControls.ui @@ +8,3 @@ </item> <item> + <attribute name="label" translatable="yes">_Rename</attribute> Please append the ellipses character at the end, in order to match the mockup. @@ +62,3 @@ + <property name="sensitive">True</property> + <style> + <property name="can_focus">False</property> use "suggested-action" instead.
Created attachment 321829 [details] [review] Patch for playlist rename Hi Felipe. I have made the changes you asked. For the GtkEntry, I used set_text(title) instead of clearing as it made more sense to set the input text/placeholder explicitly.
Yash Mehrotra, I tried you patch. However, after I pressed done, the GtkTreeView showing all playlists became empty.
(In reply to Chieh-Min Wang from comment #14) > Yash Mehrotra, I tried you patch. However, after I pressed done, the > GtkTreeView showing all playlists became empty. Yeah, you are right but it's inconsistent (ie. Sometimes it works fine, some times it shows the error) I think the error is due clearing and populating the SideView. I think a better approach would be to just rename the name of the playlist in the SideView. What do you suggest ?
Review of attachment 321829 [details] [review]: The sideview not populating right is definitely the major problem here. I don't think renaming in place is an option, since you are actually repopulating the list from tracker. I think you have to look at what's actually going on in those clear and repopulate calls what makes it fail.
Created attachment 350232 [details] [review] rebased patch A rebased patch. The sideview does not seem to have any problem as far as I can see.
Review of attachment 350232 [details] [review]: Thanks for picking this up. I think the reload problem might have been fixed by other commits to the playlist code. The button should be linked and default action, so you can finish the rename with hitting return. ::: gnomemusic/playlists.py @@ +340,2 @@ @log + def rename_playlist(self, item, new_name): needs a docstring, public function @@ +347,3 @@ + Query.rename_playlist(item.get_id(), new_name), GLib.PRIORITY_LOW, + None, update_callback, None + ) I prefer the closing the parenthesis behind the last argument here. ::: gnomemusic/views/playlistview.py @@ +89,3 @@ + self._playlist_rename_action = Gio.SimpleAction.new('playlist_rename', + None) indentation @@ +91,3 @@ + None) + self._playlist_rename_action.connect('activate', + self._on_rename_activate) indentation @@ +136,3 @@ self._songs_count = 0 + self._is_rename_active = False + self.pl_torename = None prepend with _ @@ +137,3 @@ + self._is_rename_active = False + self.pl_torename = None + self._pl_torename_index = None Is it used? @@ +513,3 @@ + self._name_stack.set_visible_child_name('renaming_dialog') + try: + self._rename_entry.set_text(self.pl_torename.get_title()) I don't think this throws an exception? @@ +515,3 @@ + self._rename_entry.set_text(self.pl_torename.get_title()) + except TypeError: + self._rename_entry.set_text("") I think setting it empty should be disallowed. ::: gnomemusic/window.py @@ +472,3 @@ and GLib.unichar_isprint(chr(key_unic)) and (event_and_modifiers == Gdk.ModifierType.SHIFT_MASK + or event_and_modifiers == 0) indent
Hi Marinus, Thanks for the review. I have got one question: When doing self.pl_torename.get_title() I got a TypeError exception if the current playlist is untitled. Thus the try and except. How should we handle it ?
(In reply to Jean Felder from comment #19) > I have got one question: > When doing self.pl_torename.get_title() I got a TypeError exception if the > current playlist is untitled. Thus the try and except. > How should we handle it ? I think you get a TypeError because you try to set a None value in set_text, but it requires a string. The question really is: why is the playlist title None? I don't think it should be.
(In reply to Marinus Schraal from comment #20) > (In reply to Jean Felder from comment #19) > > I have got one question: > > When doing self.pl_torename.get_title() I got a TypeError exception if the > > current playlist is untitled. Thus the try and except. > > How should we handle it ? > > I think you get a TypeError because you try to set a None value in set_text, > but it requires a string. > > The question really is: why is the playlist title None? I don't think it > should be. I think the playlist title None come from cue or m3u files ? I will try to reset tracker and take a deeper look at it.
Created attachment 350374 [details] [review] rebased patch v2 Here is a new version of the patch: - previous remarks should be fixed - renaming button and entry are disabled if you switch view while editing playlist name - new playlist name cannot be empty
Created attachment 357035 [details] [review] rebased patch v3 I've just understood the TypeError exception with the playlists without a name. I was directly calling 'self.pl_torename' but it should be 'utils.get_media_title(self.pl_torename)'. This is now fixed. I think that everything is ready now with this patch.
Review of attachment 357035 [details] [review]: Just some minor points left. One thing that I encountered during testing that if you switch playlists the rename stays active. It should be aborted at that point of course. ::: data/PlaylistControls.ui @@ +40,3 @@ + </child> + <child> + <object class="GtkBox"> set the style as linked https://wiki.gnome.org/HowDoI/Buttons @@ +72,3 @@ + <property name="name">renaming_dialog</property> + </packing> + </child> I noticed in the source file that the indentation here is with tabs instead of spaces, spaces is the way to go. ::: gnomemusic/views/playlistview.py @@ +510,3 @@ + def _stage_playlist_for_renaming(self): + _iter = self._pl_generic_view.get_selection().get_selected()[1] + self.pl_torename = self._playlists_model[_iter][5] This is the only place pl_torename is used right? Treat it as a function var then (so drop the self part). ::: gnomemusic/window.py @@ +506,3 @@ + # Disable renaming playlist it it was active when leaving Playlist view + if self.prev_view == self.views[3] and self.views[3].is_rename_active: + self.views[3].disable_rename_playlist() if (x and y):
Created attachment 357614 [details] [review] rebased patch v4 Previous remarks fixed. Everything should be in order.
Review of attachment 357614 [details] [review]: Pretty much there alright. ::: data/PlaylistControls.ui @@ +67,3 @@ + <class name="suggested-action"/> + </style> + </object> Still missing the linking part here, see for example PlaylistDialog.ui ::: gnomemusic/window.py @@ +506,3 @@ + # Disable renaming playlist it it was active when leaving Playlist view + if (self.prev_view == self.views[3] + and self.views[3].is_rename_active): The indent is like in _on_key_press function in the same file (other parts aren't converted yet).
Created attachment 357650 [details] [review] rebased patch v5 I had not understood the linked problem. Fixed.
Hi!I'm interested in solving this bug.Is this bug still open?Have the patches submitted above accepted or not?
Review of attachment 357650 [details] [review]: lgtm, rebase is needed for current master. Apologies for the delayed review. ::: gnomemusic/playlists.py @@ +112,3 @@ 'playlist-deleted': (GObject.SignalFlags.RUN_FIRST, None, (Grl.Media,)), 'playlist-updated': (GObject.SignalFlags.RUN_FIRST, None, (int,)), + 'playlist-renamed': (GObject.SignalFlags.RUN_FIRST, None, (Grl.Media,)), (pep8) Line too long, rebase to see current approach. @@ +339,3 @@ + @log + def rename_playlist(self, item, new_name): Just 'rename' would do. ::: gnomemusic/views/playlistview.py @@ +75,3 @@ + self._rename_entry = builder.get_object('playlist_rename_entry') + self._rename_entry.connect('changed', self._on_rename_entry_changed) + self._rename_done_button = builder.get_object('playlist_rename_done_button') Line too long(?) @@ +136,3 @@ self._pl_todelete_index = None self._songs_count = 0 + self.is_rename_active = False Make it a property. Can be called just 'rename_active'. @@ +402,3 @@ grilo.populate_playlist_songs(playlist, self._add_item) + # disable delete and rename buttons if current playlist is a smart playlist Unneeded comment (and too long, 72 chars in pep-8). @@ +517,3 @@ + + if not _iter: + return Why this check? Wouldn't it already error out before? ::: gnomemusic/window.py @@ +473,3 @@ and (event_and_modifiers == Gdk.ModifierType.SHIFT_MASK + or event_and_modifiers == 0) + and not self.views[3].is_rename_active): I'm not fond of this, but I can see why it's needed for now. window.py is an unholy mess.
Created attachment 365727 [details] [review] playlist: add the possibility to rename a playlist
Review of attachment 365727 [details] [review]: lgtm
Added with commit 9f6e386ccc3a36fedfecb32367036b997bcf3c03 , thanks for your work and keeping me on my toes. This problem has been fixed in the unstable development version. The fix will be available in the next major software release. You may need to upgrade your Linux distribution to obtain that newer version.