After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 744834 - Remove 'New Playlist' entry, introduce a textbox and a button
Remove 'New Playlist' entry, introduce a textbox and a button
Status: RESOLVED FIXED
Product: gnome-music
Classification: Applications
Component: general
3.15.x
Other Linux
: Normal normal
: ---
Assigned To: gnome-music-maint
gnome-music-maint
Depends on:
Blocks:
 
 
Reported: 2015-02-19 23:59 UTC by Vadim Rutkovsky
Modified: 2016-08-17 06:10 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Mockup (21.04 KB, image/png)
2016-01-05 18:47 UTC, Vadim Rutkovsky
  Details
Remove 'New Playlist' entry, introduce a textbox (114.16 KB, patch)
2016-02-05 21:43 UTC, Prachi Agrawal
needs-work Details | Review
Patch updated. Sorry for the wrong commit earlier. (2.15 KB, patch)
2016-02-10 11:39 UTC, Prachi Agrawal
none Details | Review
friendlyNewPlaylist: adds entry at bottom of add to playlist dialog (12.23 KB, patch)
2016-03-27 04:56 UTC, Billy Barrow
none Details | Review
playlist-dialog: add bottom entry (12.09 KB, patch)
2016-08-09 22:47 UTC, Georges Basile Stavracas Neto
none Details | Review
playlist-dialog: add bottom entry (12.22 KB, patch)
2016-08-11 19:46 UTC, Georges Basile Stavracas Neto
committed Details | Review
playlist-dialog: don't show static playlists (2.37 KB, patch)
2016-08-11 19:46 UTC, Georges Basile Stavracas Neto
none Details | Review
playlist-dialog: don't show static playlists (2.24 KB, patch)
2016-08-12 20:58 UTC, Georges Basile Stavracas Neto
none Details | Review
playlist-dialog: don't show static playlists (2.24 KB, patch)
2016-08-12 21:05 UTC, Georges Basile Stavracas Neto
committed Details | Review

Description Vadim Rutkovsky 2015-02-19 23:59:38 UTC
See Contacts / Empathy for similar idea of 'type a new name or select from the list'
Comment 1 Allan Day 2016-01-05 18:01:14 UTC
It's hard to tell what is being proposed here. Can you provide some more details, Vadim?

(Removing target-milestone while I'm at it.)
Comment 2 Vadim Rutkovsky 2016-01-05 18:47:17 UTC
Created attachment 318274 [details]
Mockup

See attached mockup. It seems that current playlist selection dialog is too cryptic for new users, iirc Contacts handles similar situation better
Comment 3 Allan Day 2016-01-06 12:41:49 UTC
(In reply to Vadim Rutkovsky from comment #2)
...
> See attached mockup. It seems that current playlist selection dialog is too
> cryptic for new users, iirc Contacts handles similar situation better

Ah right, yes I agree that that would be better. I've done some mockups for this myself:

https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/music/wire-add-to-playlist-dialog.png
Comment 4 Prachi Agrawal 2016-02-05 21:43:58 UTC
Created attachment 320515 [details] [review]
Remove 'New Playlist' entry, introduce a textbox

Mentioned in the Bug 744834 of gnome-music, need for textbox inplace of "New Playlist" entry.

In the current version of gnome-music, to create a new playlist, "New Playlist" clickable entry is present, which needs to be replaced by a textbox and a button.

For this to be done, changes have been made in the "_add_list_renderers" function in the "PlaylistDialog" class in widgets.py file.
A "CallRendererCombo" has been used in place of "styledtextrenderer", that enables users to type the name of the playlist as they want and hit enter key to create the playlist.
Comment 5 Felipe Borges 2016-02-08 12:26:40 UTC
Review of attachment 320515 [details] [review]:

your patch rewrites the whole files that you have changed. Could you please patch just the changes you made?

Thanks
Comment 6 Felipe Borges 2016-02-08 12:26:42 UTC
Review of attachment 320515 [details] [review]:

your patch rewrites the whole files that you have changed. Could you please patch just the changes you made?

Thanks
Comment 7 Saiful B. Khan 2016-02-08 12:42:47 UTC
(In reply to Prachi Agrawal from comment #4)
> Created attachment 320515 [details] [review] [review]
> Remove 'New Playlist' entry, introduce a textbox
> 
> Mentioned in the Bug 744834 of gnome-music, need for textbox inplace of "New
> Playlist" entry.
> 
> In the current version of gnome-music, to create a new playlist, "New
> Playlist" clickable entry is present, which needs to be replaced by a
> textbox and a button.
> 
> For this to be done, changes have been made in the "_add_list_renderers"
> function in the "PlaylistDialog" class in widgets.py file.
> A "CallRendererCombo" has been used in place of "styledtextrenderer", that
> enables users to type the name of the playlist as they want and hit enter
> key to create the playlist.

Hey Prachi, I was working on this same bug but have yet to complete what I aim to do. However, it seems you have submitted your patch first that I believe intends to follow on Vadim's original design (attachment 318274 [details]). The patch I have in mind tries to bring in Allan's idea  of the playlist dialog. Would it be fine if I continued working on it?
If you want to discuss the issue you can reply here as well as the irc. I'm 'khan' on #gnome-music.
Comment 8 Prachi Agrawal 2016-02-10 11:39:17 UTC
Created attachment 320784 [details] [review]
Patch updated. Sorry for the wrong commit earlier.
Comment 9 Felipe Borges 2016-02-17 13:34:56 UTC
Review of attachment 320784 [details] [review]:

thanks for your patch.

::: gnomemusic/widgets.py
@@ +792,3 @@
         cols.set_cell_data_func(type_renderer, self._on_list_text_render)
         self.view.append_column(cols)
+        '''

please, do not leave commented code in the patch.

@@ +797,3 @@
     def populate(self):
         self.add_playlist_iter = self.model.append()
+       # self.model.set(self.add_playlist_iter, [0, 1], [_("New Playlist"), True])

ditto.
Comment 10 Billy Barrow 2016-03-27 04:56:40 UTC
Created attachment 324827 [details] [review]
friendlyNewPlaylist: adds entry at bottom of add to playlist dialog

Fix for bug 744834 - Remove 'New Playlist' entry, introduce a textbox an button

Adds a gtk entry at the bottom of the PlaylistDialog and removes the 'New Playlist' entry from the playlist selection.

When a new playlist is created, the song(s) are automatically added to the new playlist without the need for any more user input.

I am a newbie here so please let me know if I have done anything wrong, thank you.
Comment 11 Georges Basile Stavracas Neto 2016-08-09 22:47:19 UTC
Created attachment 333034 [details] [review]
playlist-dialog: add bottom entry

I managed to update the previous patch. Hope the previous author won't get angry :)
Comment 12 Marinus Schraal 2016-08-10 14:11:05 UTC
Review of attachment 333034 [details] [review]:

Looks pretty good.

One thing that needs fixing is the dialog showing playlists that shouldn't be altered, like the auto-generated playlists & static (on-disk) playlists. Tracker allows it, but it doesn't make much sense.

Also the usual pep8 & docstring fixes.

::: gnomemusic/widgets.py
@@ +813,3 @@
         self._select_button.connect('clicked', self._on_selection)
 
+

pep-8 -blank line

@@ +817,3 @@
+        self._new_playlist_button.connect('clicked', self._on_editing_done)
+        self._new_playlist_entry = self.ui.get_object('new-playlist-entry')
+        self._new_playlist_entry.connect('changed', self._on_new_playlist_entry_changed)

pep-8 linelength

@@ +819,3 @@
+        self._new_playlist_entry.connect('changed', self._on_new_playlist_entry_changed)
+        self._new_playlist_entry.connect('activate', self._on_editing_done)
+        self._new_playlist_entry.connect('focus-in-event', self._on_new_playlist_entry_focused)

pep8 linelength
Comment 13 Georges Basile Stavracas Neto 2016-08-11 19:46:13 UTC
Created attachment 333132 [details] [review]
playlist-dialog: add bottom entry

Fixed the pep8 issues.
Comment 14 Georges Basile Stavracas Neto 2016-08-11 19:46:38 UTC
Created attachment 333133 [details] [review]
playlist-dialog: don't show static playlists

Static playlists are managed by GNOME Music and shouldn't
be exposed as available playlists in the playlist dialog,
even if adding to them works.

Fix that by adding a new method to check whether a playlist
is static or not, and teaching the playlists dialog to not
add static playlists.
Comment 15 Marinus Schraal 2016-08-12 14:21:58 UTC
Review of attachment 333133 [details] [review]:

I noticed StaticPlaylists has get_protected_ids(), cant this be used instead for is_static_playlist?

I know everything else in there uses your method as well, but still...
Comment 16 Georges Basile Stavracas Neto 2016-08-12 20:58:37 UTC
Created attachment 333196 [details] [review]
playlist-dialog: don't show static playlists

Not feasible/good to use get_protected_ids() instead of is_static_playlist(), but I could've used INSIDE is_static_playlists().
Comment 17 Georges Basile Stavracas Neto 2016-08-12 21:05:25 UTC
Created attachment 333197 [details] [review]
playlist-dialog: don't show static playlists

Fix a small style issue.
Comment 18 Marinus Schraal 2016-08-13 21:59:44 UTC
(In reply to Georges Basile Stavracas Neto from comment #16)
> Not feasible/good to use get_protected_ids() instead of
> is_static_playlist(), but I could've used INSIDE is_static_playlists().

That's actually what I meant to say, my bad.

There's some odd bits that could use fixing, but that are general bugs as they are and not directly related to this patch. I'm gonna open up some bugs on them.
Comment 19 Marinus Schraal 2016-08-13 22:00:46 UTC
Review of attachment 333132 [details] [review]:

fine
Comment 20 Marinus Schraal 2016-08-13 22:01:39 UTC
Review of attachment 333197 [details] [review]:

good to go
Comment 21 Georges Basile Stavracas Neto 2016-08-17 06:09:57 UTC
Thanks for all the reviews. I'm happy that these changes are landing :)

I'm closing this bug as FIXED. Lets eventually open separate issues for the other 'odd bits'.

Attachment 333132 [details] pushed as ea71c1e - playlist-dialog: add bottom entry
Attachment 333197 [details] pushed as 6bb7fbf - playlist-dialog: don't show static playlists