GNOME Bugzilla – Bug 703255
Add a toolbar for selection mode.
Last modified: 2013-07-12 10:01:03 UTC
A toolbar needs to be added to implement selection mode as shown in mockups. https://raw.github.com/gnome-design-team/gnome-mockups/master/music/wire-playlists.png
Created attachment 248057 [details] [review] Attachment to the patch. Created the selection toolbar according to the design. Selection toolbar is now visible in selection mode.
Review of attachment 248057 [details] [review]: Thanks, looks good, but needs some improvement: 1) breaks this.player.discoverer - all items become disable 2) needs 'Click on items to select them' label (not sure if this is a part of the patch)
Created attachment 248346 [details] [review] Attachment to the patch. Fixed the issues with player discoverer. Added "Click on items to select them" as header bar title in selection mode. Added Cancel button.
Review of attachment 248346 [details] [review]: Thanks, toolbar works, though mockups show that there should be a down arrow after the label. Please contact designers on its purpose. Also, a couple of nitpicks ::: src/toolbar.js @@ +108,3 @@ if (this._selectionMode) { + this.custom_title = null; + this.title = "Click on items to select them." Not sure if this will be translated, possibly we should store this in .ui file? @@ +142,3 @@ + _addCancelButton: function() { + this._cancelButton = new Gd.HeaderSimpleButton({ label: _("Cancel") }); Enhancement: move this declarations to .ui ::: src/view.js @@ +146,3 @@ + this.view.connect('view-selection-changed',Lang.bind(this,function(){ + let items = this.view.get_selection(); + if(items.length > 0) I think this 'if' can be simplified: this.selection_toolbar._add_to_playlist_button.sensitive = items.length > 0 ::: src/widgets.js @@ +291,3 @@ + this.view.connect('view-selection-changed',Lang.bind(this,function(){ + let items = this.view.get_selection(); + if(items.length > 0) Same here - can be simplified ::: src/window.js @@ +148,2 @@ this.toolbar.show(); +// this.selection_toolbar.eventbox.show_all(); Please remove commented code from the patch
Created attachment 248768 [details] [review] Attachment to the patch. Added the ui for headerbar and selection toolbar. Incorporated the suggested improvements.
Created attachment 248791 [details] [review] Attachment to the patch. Forgot to add the search button.
Created attachment 248792 [details] [review] Sorry for uploading the wrong patch.
Review of attachment 248792 [details] [review]: The toolbar shows up great . You might want to look at interchanging the places of the search button and the Cancel button in it. The size of the cancel button also needs to be tweaked to match the mockups here https://raw.github.com/gnome-design-team/gnome-mockups/master/music/wire-playlists.png. This is what I get after applying the patch -> http://ctrlv.in/212530 The difference between the UI in the mockups to that what the patch renders can be found here -> http://img.ctrlv.in/img/51dcf9d604f9c.png
Created attachment 248827 [details] [review] Fix for the cancel button postion.
Review of attachment 248827 [details] [review]: See comments inline. ::: data/Headerbar.ui @@ +64,3 @@ + <property name="sensitive">True</property> + <style> + <class name="image-button"/> wrong indentation @@ +70,3 @@ + <property name="visible">True</property> + <property name="can_focus">False</property> + <property name="icon-name">folder-saved-search-symbolic</property> I think this is not the correct search image, check gitg for the right one @@ +113,3 @@ + <class name="suggested-action"/> + </style> + </object> wrong indentation ::: src/window.js @@ +147,3 @@ } + this.toolbar._closeButton.connect("clicked",Lang.bind(this,function(){ + this.destroy(); this is not correct. This patch should be applied on top of https://git.gnome.org/browse/gnome-music/commit/?h=wip/gtkmaster
Review of attachment 248827 [details] [review]: Looks great, I just have some suggestions. ::: data/Headerbar.ui @@ +1,2 @@ +<?xml version="1.0" encoding="UTF-8"?> +<interface> Please set the translation domain (attribute "domain" of "interface" element) to "gnome-music". More info at https://bugzilla.gnome.org/show_bug.cgi?id=703601 @@ +79,3 @@ + </packing> + </child> + <child> Wrong indentation. @@ +125,3 @@ + <property name="sensitive">True</property> + <style> + <class name="image-button"/> Wrong indentation. ::: data/SelectionToolbar.ui @@ +1,2 @@ +<?xml version="1.0" encoding="UTF-8"?> +<interface> Please set the translation domain to "gnome-music". More info at https://bugzilla.gnome.org/show_bug.cgi?id=703601 @@ +10,3 @@ + <property name="vexpand">False</property> + <child> + <object class="GtkBox" id="box1"> I think a GtkButtonBox is more appropriate. @@ +17,3 @@ + <property name="halign">start</property> + <property name="valign">center</property> + <property name="margin_left">20</property> This margin is too large compared to the top and bottom margin. The mockup suggests that all margins are the same. Also, could you set the right margin too? So that it will be symmetric, in case we have to add a new button to the right in the future. It won't affect the "Add to Playlist" because it is left-aligned. @@ +27,3 @@ + <property name="receives_default">True</property> + <property name="double_buffered">False</property> + <property name="margin_top">10</property> Maybe you could move these two margins into the box so that all margins will be in the same place? ::: src/player.js @@ +817,1 @@ Signals.addSignalMethods(Player.prototype); Please move "Signals.addSignalMethods(Player.prototype);" before "const SelectionToolbar ..." so that it won't get separated from its prototype. Though I think SelectionToolbar is better suited to be in toolbar.js, or in its own source file (but it's too short, so maybe it should be in toolbar.js).
Created attachment 248928 [details] [review] Attachment to the patch. Fixed Indentation errors, updated the interface domain to "gnome-music" and used _closesButton.get_top_level.close().
Thanks, I also removed duplicate function The following fix has been pushed: 6b5bfe0 added headerbar and selection toolbar.
Created attachment 248994 [details] [review] added headerbar and selection toolbar. added search button. deleted: 0001-added-headerbar-and-selection-toolbar.patch cancel button position fix. Selection toolbar and Headerbar patch.