GNOME Bugzilla – Bug 772089
[playlist dialog] implement empty state dialog
Last modified: 2018-01-10 14:56:02 UTC
Implement the empty state dialog as seen in the design here: https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/music/wire-add-to-playlist-dialog.png (top left).
Created attachment 342478 [details] [review] playlistdialog: Add firstPlaylistDialog query: add query to count no. of non-static playlists This is a raw patch.
Created attachment 342479 [details] [review] playlistdialog, query, PlaylistDialog: Add new dialog box when creating first playlist
Created attachment 342480 [details] [review] playlistdialog,query: Add new dialog box when creating first playlist
Created attachment 342645 [details] [review] playlistdialog,query: Add new dialog box when creating first playlist. -Raw patch
Created attachment 342697 [details] [review] playlistdialog,query: Add new dialog box when creating first playlist. -Raw patch
Review of attachment 342697 [details] [review]: Looks pretty good overall. Let's get into details when the bigger problems are fixed. ::: data/PlaylistDialog.ui @@ +13,3 @@ <property name="can_focus">False</property> <property name="receives_default">False</property> + <property name="use_underline">True</property> I know this is a bit irritating with glade, but this is not something you touch so don't change it (and several other places like it). @@ +54,3 @@ + <property name="transition_duration">250</property> + <child> + <object class="GtkDialog" id="dialog1"> This looks the wrong way around and probably the reason of the criticals/warnings, the dialog contains a stack. Not a stack containing a dialog. ::: gnomemusic/query.py @@ +143,3 @@ + 'RECENTLY_PLAYED', 'RECENTLY_ADDED', 'FAVORITES')) + } + """.replace('\n', ' ').strip() The query counts on-disk playlists as well. ::: gnomemusic/widgets/playlistdialog.py @@ +44,3 @@ self.ui = Gtk.Builder() self.ui.add_from_resource('/org/gnome/Music/PlaylistDialog.ui') + self.add_playlist_stack = self.ui.get_object('add_playlist_stack') self._add_playlist_stack @@ +48,3 @@ + cursor = tracker.query(Query.all_non_static_playlists_count(), None) + if cursor is not None and cursor.next(): + count = cursor.get_integer(0) This should be done through grilo for cleanliness like https://git.gnome.org/browse/gnome-music/commit/?id=77be4ead5cec6b4aa66c8ec4a168bed06888e953 . Also should be async. @@ +51,3 @@ + + if count > 0: + self.dialog_box = self.ui.get_object('dialog1') self._dialog_box
Created attachment 343487 [details] [review] playlistdialog,query: Add new dialog box when creating first playlist.
Review of attachment 343487 [details] [review]: Getting there, good work. ::: data/PlaylistDialog.ui @@ +279,3 @@ + </object> + </child> + </object> About the whole playlistdialog ui: you should be able to add the dialog boxes (1&2, but better name them something useful like 'empty_state' & 'normal_state' or something) as childobjects of the stack, saves you placing them manually in playlistdialog.py, @@ +286,3 @@ <child> <object class="GtkButton" id="cancel-button"> + <property name="label" translatable="yes">Cancel</property> This shouldn't be changed. @@ +298,3 @@ <child> <object class="GtkButton" id="select-button"> + <property name="label" translatable="yes">Add</property> This shouldn't be changed. ::: gnomemusic/query.py @@ +143,3 @@ + 'RECENTLY_PLAYED', 'RECENTLY_ADDED', 'FAVORITES')) + } + """.replace('\n', ' ').strip() This query is still broken, listing local playlists. Also, I don't like the fact that the query contains the names of the static playlists (most played, etc). This query works for me (not counting yet, but just as an example): SELECT tracker:id(?pl) ?pl nie:title(?pl) { ?pl a nmm:Playlist . OPTIONAL { ?pl nao:hasTag ?tag } FILTER (!BOUND(nfo:belongsToContainer(?pl))) FILTER ( !BOUND(?tag)) } ::: gnomemusic/widgets/playlistdialog.py @@ +28,3 @@ from gnomemusic.grilo import grilo from gnomemusic.playlists import Playlists +from gnomemusic.query import Query This is not needed, it works through grilo.py now. @@ +102,3 @@ + self.add_playlist_stack.set_visible_child(self.box_dialog2) + + self._setup_view() Why setup the view here? The treeview is not gonna be used. @@ +112,3 @@ + self._on_new_playlist_entry_changed) + self._new_playlist_entry.connect('activate', + self._on_editing_done) Indent too far. @@ +121,1 @@ + grilo.playlists_available(playlists_available_cb) 2 notes here on the whole _setup_dialog function. * Most lines are too long (pep8). * There's serious duplication in available/unavailable cases. Afaics the only thing you need to do is to get the objects specific for the stack page (_new_pl_{button,entry}), everything else is the same. ::: gnomemusic/window.py @@ +571,3 @@ return add_to_playlist = PlaylistDialog(self) Maybe name add_to_playlist more obvious. playlist_dialog should do. Makes the run & destroy more logical.
Created attachment 343541 [details] [review] playlistdialog,query: Add new dialog box when creating first playlist.
Created attachment 343554 [details] [review] playlistdialog,query: Add new dialog box when creating first playlist.
(In reply to Yash from comment #10) > Created attachment 343554 [details] [review] [review] > playlistdialog,query: Add new dialog box when creating first playlist. Did major cleanup and removed duplicated code. Hopefully now we can start improving the UI.
Review of attachment 343554 [details] [review]: I still see this warning on the empty dialog part: (gnome-music:28212): Gtk-WARNING **: Allocating size to GtkDialog 0x2e51170 without calling gtk_widget_get_preferred_width/height(). How does the code know the size to allocate? Also overall I think it should only show the editable playlists (so user-created). Those are the ones we count, but the dialog shows all playlist (static ones excluded). This should be a separate patch. ::: data/PlaylistDialog.ui @@ +95,3 @@ + <child> + <object class="GtkButton" id="create-first-playlist-button"> + <property name="label" translatable="yes">Create</property> Make it a mnemonic, 'C_reate' & add use_underline property. The R b/c the C is taken for cancel iirc. @@ +122,3 @@ + <child> + <object class="GtkButtonBox"> + <property name="name">internal action_area</property> i think you mean internal_action_area? Is this name needed at all? @@ +132,3 @@ + <child> + <placeholder/> + </child> are these placeholders needed @@ +147,3 @@ + <property name="name">page0</property> + <property name="title" translatable="yes">page0</property> + </packing> Is this packing needed and certainly we don't need a translatable 'page0' string? @@ +267,3 @@ + <child> + <placeholder/> + </child> are these placeholders needed @@ +302,3 @@ <property name="can_focus">False</property> <property name="receives_default">False</property> + <property name="use_underline">True</property> this still unneeded reordered @@ +314,3 @@ <property name="can_focus">False</property> <property name="receives_default">False</property> + <property name="use_underline">True</property> see above ::: gnomemusic/widgets/playlistdialog.py @@ +40,3 @@ self.ui = Gtk.Builder() self.ui.add_from_resource('/org/gnome/Music/PlaylistDialog.ui') + self.add_playlist_stack = self.ui.get_object('add_playlist_stack') self._add_playlist_stack @@ +45,3 @@ + self._dialog_box.set_transient_for(parent) + self.normal_state = self.ui.get_object('normal_state') + self.empty_state = self.ui.get_object('empty_state') self.{_normal_state,_empty_state} @@ +79,3 @@ + if available: + self.add_playlist_stack.set_visible_child(self.normal_state) + I think there's a bit too much blank lines going on in the next section. Try to make it into logical chunks. I think this can be thought of setting dialog state and as such the sections in the if/else parts can be considered one block. @@ +108,1 @@ + self.playlist = Playlists.get_default() self._playlist, but I rather see you create this in __init__. It looks out of place here.
Created attachment 345657 [details] [review] playlistdialog,query: Add new dialog box when creating first playlist.
Review of attachment 345657 [details] [review]: lgtm ::: gnomemusic/query.py @@ +140,3 @@ + OPTIONAL { ?pl nao:hasTag ?tag } . + FILTER (!BOUND(nfo:belongsToContainer(?pl))) + FILTER ( !BOUND(?tag)) Remove the space before !, it's a bit odd 2 identical lines doing different things
Created attachment 345667 [details] [review] playlistdialog,query: Add new dialog box when creating first playlist.
Uhm lets keep it open to track some minor issues that need fixing. * the warning mentioned earlier needs figuring out * I think the dialog can be brought in line with the mock-up in regards to its looks (details, details) * anything that pops up
-- 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/74.