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.)

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

(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

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.

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

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

(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.

Created attachment 320784 [details] [review]
Patch updated. Sorry for the wrong commit earlier.

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.

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.

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 :)

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

Created attachment 333132 [details] [review]
playlist-dialog: add bottom entry

Fixed the pep8 issues.

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.

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...

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().

Created attachment 333197 [details] [review]
playlist-dialog: don't show static playlists

Fix a small style issue.

(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.

Review of attachment 333132 [details] [review]:

fine

Review of attachment 333197 [details] [review]:

good to go

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