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 703255 - Add a toolbar for selection mode.
Add a toolbar for selection mode.
Status: RESOLVED FIXED
Product: gnome-music
Classification: Applications
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gnome-music-maint
gnome-music-maint
Depends on:
Blocks:
 
 
Reported: 2013-06-28 14:14 UTC by Sai Suman Prayaga
Modified: 2013-07-12 10:01 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Attachment to the patch. (8.75 KB, patch)
2013-06-29 18:05 UTC, Sai Suman Prayaga
needs-work Details | Review
Attachment to the patch. (14.21 KB, patch)
2013-07-03 19:19 UTC, Sai Suman Prayaga
needs-work Details | Review
Attachment to the patch. (23.20 KB, patch)
2013-07-09 19:06 UTC, Sai Suman Prayaga
none Details | Review
Attachment to the patch. (48.32 KB, patch)
2013-07-10 03:22 UTC, Sai Suman Prayaga
none Details | Review
Sorry for uploading the wrong patch. (24.15 KB, patch)
2013-07-10 03:37 UTC, Sai Suman Prayaga
needs-work Details | Review
Fix for the cancel button postion. (24.26 KB, patch)
2013-07-10 11:40 UTC, Sai Suman Prayaga
needs-work Details | Review
Attachment to the patch. (24.06 KB, patch)
2013-07-11 13:59 UTC, Sai Suman Prayaga
committed Details | Review
added headerbar and selection toolbar. (22.96 KB, patch)
2013-07-12 10:01 UTC, Vadim Rutkovsky
committed Details | Review

Description Sai Suman Prayaga 2013-06-28 14:14:08 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
Comment 1 Sai Suman Prayaga 2013-06-29 18:05:31 UTC
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.
Comment 2 Vadim Rutkovsky 2013-07-01 07:35:06 UTC
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)
Comment 3 Sai Suman Prayaga 2013-07-03 19:19:28 UTC
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.
Comment 4 Vadim Rutkovsky 2013-07-04 08:42:16 UTC
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
Comment 5 Sai Suman Prayaga 2013-07-09 19:06:48 UTC
Created attachment 248768 [details] [review]
Attachment to the patch.

Added the ui for headerbar and selection toolbar.
Incorporated the suggested improvements.
Comment 6 Sai Suman Prayaga 2013-07-10 03:22:10 UTC
Created attachment 248791 [details] [review]
Attachment to the patch.

Forgot to add the search button.
Comment 7 Sai Suman Prayaga 2013-07-10 03:37:03 UTC
Created attachment 248792 [details] [review]
Sorry for uploading the wrong patch.
Comment 8 Shivani Poddar 2013-07-10 06:21:03 UTC
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
Comment 9 Sai Suman Prayaga 2013-07-10 11:40:20 UTC
Created attachment 248827 [details] [review]
Fix for the cancel button postion.
Comment 10 Ignacio Casal Quinteiro (nacho) 2013-07-10 13:31:26 UTC
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
Comment 11 Arnel Borja 2013-07-10 15:11:47 UTC
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).
Comment 12 Sai Suman Prayaga 2013-07-11 13:59:23 UTC
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().
Comment 13 Vadim Rutkovsky 2013-07-12 10:00:55 UTC
Thanks, I also removed duplicate function

The following fix has been pushed:
6b5bfe0 added headerbar and selection toolbar.
Comment 14 Vadim Rutkovsky 2013-07-12 10:01:03 UTC
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.