Created attachment 253155 [details] [review]
topbar: Rename to Cancel and remove emphasis on Done button

This is to follow the new design for Content Selection Pattern.

This was changed from 'Cancel' to 'Done' as part of https://bugzilla.gnome.org/show_bug.cgi?id=674671 :-/

(In reply to comment #2)
> This was changed from 'Cancel' to 'Done' as part of
> https://bugzilla.gnome.org/show_bug.cgi?id=674671 :-/

That was 2012, 2013 came with the new style. :)

Review of attachment 253155 [details] [review]:

Please try to keep the commit's subject line under 50 characters (you can explain in the details part) and link to new design in commit log details would be nice.

With the new design, we also want to "Activating an action from the bottom toolbar exits selection mode and performs the action on the selected content." as it says on the design page (At the bottom, just above "Development Tracking" heading).

::: src/topbar.vala
@@ -113,3 @@
-        done_btn = selection_toolbar.add_button (null, _("D_one"), false) as Gtk.Button;
-        done_btn.use_underline = true;
-        // workaround for libgd bug #698289

Have you checked if this workaround is really not needed? If so, please mark that bug as fixed.

Thanks for reviewing it! :D

(In reply to comment #4)
> Review of attachment 253155 [details] [review]:
> 
> Please try to keep the commit's subject line under 50 characters (you can
> explain in the details part) and link to new design in commit log details would
> be nice.

OK.

> With the new design, we also want to "Activating an action from the bottom
> toolbar exits selection mode and performs the action on the selected content."
> as it says on the design page (At the bottom, just above "Development Tracking"
> heading).

OK.

> ::: src/topbar.vala
> @@ -113,3 @@
> -        done_btn = selection_toolbar.add_button (null, _("D_one"), false) as
> Gtk.Button;
> -        done_btn.use_underline = true;
> -        // workaround for libgd bug #698289
> 
> Have you checked if this workaround is really not needed? If so, please mark
> that bug as fixed.

The bug was not fixed, but other applications like Documents don't use underline for Cancel button, so I removed this part.

I'll attach a new patch later then.

I sense we weren't so clear communicating Done vs Cancel. Depends whether the actual action dismisses the selection mode. When you're likely to perform multiple actions such as grouping songs into playlists, the selection mode is only dismissed explicitly by the user with a Done button. When it's more likely the action will be performed on the selection once, the selection mode is dismissed after the action has been performed, thus the Cancel button. Looks like the latter will be way more common.

(In reply to comment #6)
> I sense we weren't so clear communicating Done vs Cancel. Depends whether the
> actual action dismisses the selection mode. When you're likely to perform
> multiple actions such as grouping songs into playlists, the selection mode is
> only dismissed explicitly by the user with a Done button. When it's more likely
> the action will be performed on the selection once, the selection mode is
> dismissed after the action has been performed, thus the Cancel button. Looks
> like the latter will be way more common.

So it should be latter for Boxes too then IIUC?

> So it should be latter for Boxes too then IIUC?

Right now it leaves the selection mode, so with the actions implemented I see cancel being appropriate and the bug title being appropriate. Once we have collection management, we might want to revisit. But the label goes with the behavior.

Ack, didn't notice that the bug for the new design is still open at bug #699911, so I'll just set this one as blocking that said bug.

The current selection toolbar is still an OSD toolbar. Should I fix that too?

(In reply to comment #10)
> The current selection toolbar is still an OSD toolbar. Should I fix that too?

That would be great!

(In reply to comment #10)
> The current selection toolbar is still an OSD toolbar. Should I fix that too?

Yes but please update the existing patch here first cause I need to make changes to the same code and I assume you wouldn't like to rebase. :)

Created attachment 253262 [details] [review]
topbar: Rename and don't emphasize Done button

The Done button should be renamed to Cancel, and it shouldn't be
emphasized.

This is to follow the new design for Content Selection Pattern at
https://wiki.gnome.org/GnomeOS/Design/Whiteboards/SelectionPattern


Here's the reworded patches, I haven't fixed the other two issues yet.
And it's okay if I have to rebase :D

Review of attachment 253262 [details] [review]:

::: src/topbar.vala
@@ -116,3 @@
-        done_btn = selection_toolbar.add_button (null, _("D_one"), false) as Gtk.Button;
-        done_btn.use_underline = true;
-        // workaround for libgd bug #698289

This is again missing. We do want the accelerator..

Created attachment 253399 [details] [review]
topbar: Follow new selection pattern design

Renamed Done button to Cancel, and no longer emphasized. Added text-button
style class. Removed other unnecessary property changes.

Added text-button style class to New button instead of setting a custom
size.

This is to follow the new design for Content Selection Pattern at
https://wiki.gnome.org/GnomeOS/Design/Whiteboards/SelectionPattern

Created attachment 253400 [details] [review]
selectionbar: Follow new selection pattern design

Use a standard toolbar at the bottom instead of an overlaid toolbar. Close
selection mode once an action is performed. Remove use of stock items which
are now deprecated. Add padding between buttons.

This is to follow the new design for Content Selection Pattern at
https://wiki.gnome.org/GnomeOS/Design/Whiteboards/SelectionPattern

Created attachment 253401 [details] [review]
selectionbar: Fix selection bug on favorites

Selecting a machine not in favorites, while there is a machine that is in
favorites and selected, adds all selected items to favorites. This happens
because the active property is changed during the check for selection,
emitting a "clicked" signal.

This will block the "clicked" handler for favorite button while changing
this property.

(In reply to comment #14)
> Review of attachment 253262 [details] [review]:
> 
> ::: src/topbar.vala
> @@ -116,3 @@
> -        done_btn = selection_toolbar.add_button (null, _("D_one"), false) as
> Gtk.Button;
> -        done_btn.use_underline = true;
> -        // workaround for libgd bug #698289
> 
> This is again missing. We do want the accelerator..

Sorry, I just noticed this. Is "_Cancel" fine? Documents and other core apps don't use an accelerator for this though.

Also, I noticed that there is a bug in selection, I tried to fix that, but seems like there are other issues that I haven't found yet. Before, when I randomly toggle the selection with machines with some machines set as a favorite, they all set as favorites after some toggling. I used signal blocking to fix that, but now, when I click the favorites button, the selection changes and the favorite attribute is applied to random items.

Review of attachment 253400 [details] [review]:

This is a few different changes:

1. Use a standard toolbar at the bottom instead of an overlaid toolbar
2. Close selection mode once an action is performed
3. Remove use of stock items which are now deprecated.
4. Add padding between buttons

While 1 and 2 can go in the same patch, 2 and 3 doesn't belong in this patch so please separate them out.

Does it work? I'm puzzled as to where/how you are handling transparency though?

::: src/app.vala
@@ -869,3 @@
         notificationbar.display_for_action (message, Gtk.Stock.UNDO, (owned) undo, (owned) really_remove);
-
-        // go out of selection mode if there are no more boxes

Why are you removing these lines?

Review of attachment 253399 [details] [review]:

This also feels like it needs to be divided into separate patches (even though the goal is the same). Could you please divide them into separate patches? "_Cancel" is fine btw.

Not really a blocker or anything but usual commit log convention is to use present tense rather than past for the changes in the patch, e.g 'Added' -> 'Add'.

::: src/topbar.vala
@@ -116,3 @@
-        done_btn = selection_toolbar.add_button (null, _("D_one"), false) as Gtk.Button;
-        done_btn.use_underline = true;
-        // workaround for libgd bug #698289

Dude, how many times I need to remind you of this workaround being needed? This also means you didn't test this patch. :)

Review of attachment 253401 [details] [review]:

This will block -> This patch will block.

Good otherwise.

Comment on attachment 253401 [details] [review]
selectionbar: Fix selection bug on favorites

Only saw your comment now. I think this is a separate bug anyways. Could you file one?

(In reply to comment #20)
> Dude, how many times I need to remind you of this workaround being needed? This
> also means you didn't test this patch. :)

I tested it without an accelerator, but I only noticed your comment after attaching the new patches. It is already past midnight and I have class next day, so I just decided to postpone including it.

I'll work on an update now :D

(In reply to comment #19)
> Review of attachment 253400 [details] [review]:
> ::: src/app.vala
> @@ -869,3 @@
>          notificationbar.display_for_action (message, Gtk.Stock.UNDO, (owned)
> undo, (owned) really_remove);
> -
> -        // go out of selection mode if there are no more boxes
> 
> Why are you removing these lines?

It will now immediately get out of selection mode before removing any items, so this one is no longer unnecessary.(In reply to comment #20)

> Review of attachment 253399 [details] [review]:
> Not really a blocker or anything but usual commit log convention is to use
> present tense rather than past for the changes in the patch, e.g 'Added' ->
> 'Add'.

Sorry I always forget this XD (I sometimes mix up past and present tense).

Correct component, 'windows' is for Microsoft Windows installation-related.

Created attachment 253535 [details] [review]
topbar: Rename Done button and other fixes

Rename Done button to Cancel, and remove emphasis. Add text-button style
class.

This is to follow the new design for Content Selection Pattern at
https://wiki.gnome.org/GnomeOS/Design/Whiteboards/SelectionPattern

Created attachment 253536 [details] [review]
topbar: Use text-button style class on New button

This style class should be used instead of setting a custom size.

Created attachment 253537 [details] [review]
selectionbar: Use a standard toolbar

Use a standard toolbar at the bottom instead of an overlaid toolbar.
Add a revealer to slide up and down the toolbar.

This is to follow the new design for Content Selection Pattern at
https://wiki.gnome.org/GnomeOS/Design/Whiteboards/SelectionPattern

Created attachment 253538 [details] [review]
selectionbar: Hide after an action

Close selection mode once an action is performed. Always show
selectionbar even if there are no selected items.

This is to follow the new design for Content Selection Pattern at
https://wiki.gnome.org/GnomeOS/Design/Whiteboards/SelectionPattern

Created attachment 253539 [details] [review]
selectionbar: Remove usage of stock items

Remove usage of stock items which are now deprecated.

Created attachment 253540 [details] [review]
selectionbar: Use text label for Pause button

Pause is an action item and string labels are now prefered for these
items.

This is to follow the new design for Content Selection Pattern at
https://wiki.gnome.org/GnomeOS/Design/Whiteboards/SelectionPattern

Created attachment 253541 [details] [review]
selectionbar: Add button style classes and spacing

Add "image-button" and "text-button" style classes to the buttons to
fix padding of buttons. Add spacing between buttons.

This is to follow the new design for Content Selection Pattern at
https://wiki.gnome.org/GnomeOS/Design/Whiteboards/SelectionPattern

Selection bug is on #707076

Thanks! Now i can review each change separately. :)

Review of attachment 253535 [details] [review]:

Looks good.

Review of attachment 253536 [details] [review]:

ACK

Review of attachment 253537 [details] [review]:

Yay! Removing code is always good. :)

Review of attachment 253538 [details] [review]:

The commit log summary is misleading, we are exiting the selection mode all together and not just hiding the selectionbar on action. How about: selectionbar: Exist selection mode after an action ?

Also I can only spot you quiting the selection-mode on box deletion. Are you actually hiding it on other actions as well and I'm just being blind?

::: src/selectionbar.vala
@@ +142,2 @@
                 sensitive = false;
+                break;

unrelated fix.

Review of attachment 253539 [details] [review]:

Again, the commit log is misleading. We are not just simply removing usage of deprecated API but also changing the buttons to use text labels rather than images, latter being the more important part of this patch actually.

Change itself is good but please do change commit log before pushing.

Review of attachment 253540 [details] [review]:

ACK

Review of attachment 253541 [details] [review]:

I don't see you adding any space as the commit log says. I guess the style takes care of that? If so, just update the commit log before pushing.

Created attachment 253602 [details] [review]
Break loop if already insensitive

(In reply to comment #38)
> Review of attachment 253538 [details] [review]:
> 
> The commit log summary is misleading, we are exiting the selection mode all
> together and not just hiding the selectionbar on action. How about:
> selectionbar: Exist selection mode after an action ?
> 

OK. Here's the new commit log:

commit 647d8ac879928590828895127cffea80db24321b
Author: Arnel A. Borja <arnelborja@src.gnome.org>
Date:   Thu Aug 29 23:50:38 2013 +0800

    selectionbar: Exit selection mode after an action
    
    Close selection mode once an action is performed. Always show
    selectionbar even if there are no selected items.
    
    This is to follow the new design for Content Selection Pattern at
    https://wiki.gnome.org/GnomeOS/Design/Whiteboards/SelectionPattern
    
    https://bugzilla.gnome.org/show_bug.cgi?id=706818

I will no longer attach it here since it is just a commit message change.

> Also I can only spot you quiting the selection-mode on box deletion. Are you
> actually hiding it on other actions as well and I'm just being blind?
> 

They are here:

@@ -25,6 +25,8 @@ public Selectionbar () {
                    continue;
                machine.config.set_category ("favorite", favorite_btn.active);
            }
+
+           App.app.selection_mode = false;
         });
 
         pause_btn = new Gtk.Button ();
@@ -45,6 +47,7 @@ public Selectionbar () {
            }
 
            pause_btn.sensitive = false;
+           App.app.selection_mode = false;
         });
 
         remove_btn = new Gtk.Button.from_stock (Gtk.Stock.DELETE);

> ::: src/selectionbar.vala
> @@ +142,2 @@
>                  sensitive = false;
> +                break;
> 
> unrelated fix.

I'll move it to a separate patch then. (This attachment)


(In reply to comment #39)
> Review of attachment 253539 [details] [review]:
> 
> Again, the commit log is misleading. We are not just simply removing usage of
> deprecated API but also changing the buttons to use text labels rather than
> images, latter being the more important part of this patch actually.
> 
> Change itself is good but please do change commit log before pushing.

It was already using text labels before though, only the Pause button has this change.


(In reply to comment #41)
> Review of attachment 253541 [details] [review]:
> 
> I don't see you adding any space as the commit log says. I guess the style
> takes care of that? If so, just update the commit log before pushing.

Oops, I switched to Gtk.HeaderBar just like how it is done in gnome-documents, I'll remove that in the commit message.

New commit log:

commit 9b445d6ea3a8a4cbe334d4266b1387d29871e88c
Author: Arnel A. Borja <arnelborja@src.gnome.org>
Date:   Fri Aug 30 00:11:13 2013 +0800

    selectionbar: Add button style classes
    
    Add "image-button" and "text-button" style classes to the buttons to
    fix padding of buttons.
    
    This is to follow the new design for Content Selection Pattern at
    https://wiki.gnome.org/GnomeOS/Design/Whiteboards/SelectionPattern
    
    https://bugzilla.gnome.org/show_bug.cgi?id=706818

I will no longer attach it here since it is just a commit message change.

Review of attachment 253602 [details] [review]:

ack

(In reply to comment #42)
> Created an attachment (id=253602) [details] [review]
> Break loop if already insensitive
> 
> (In reply to comment #38)
> > Review of attachment 253538 [details] [review] [details]:
> > 
> > The commit log summary is misleading, we are exiting the selection mode all
> > together and not just hiding the selectionbar on action. How about:
> > selectionbar: Exist selection mode after an action ?
> > 
> 
> OK. Here's the new commit log:
> 
> commit 647d8ac879928590828895127cffea80db24321b
> Author: Arnel A. Borja <arnelborja@src.gnome.org>
> Date:   Thu Aug 29 23:50:38 2013 +0800
> 
>     selectionbar: Exit selection mode after an action
> 
>     Close selection mode once an action is performed. Always show
>     selectionbar even if there are no selected items.
> 
>     This is to follow the new design for Content Selection Pattern at
>     https://wiki.gnome.org/GnomeOS/Design/Whiteboards/SelectionPattern
> 
>     https://bugzilla.gnome.org/show_bug.cgi?id=706818
> 
> I will no longer attach it here since it is just a commit message change.

Looks good. Sure thing.

> > Also I can only spot you quiting the selection-mode on box deletion. Are you
> > actually hiding it on other actions as well and I'm just being blind?
> > 
> 
> They are here:
> 
> @@ -25,6 +25,8 @@ public Selectionbar () {
>                     continue;
>                 machine.config.set_category ("favorite", favorite_btn.active);
>             }
> +
> +           App.app.selection_mode = false;
>          });
> 
>          pause_btn = new Gtk.Button ();
> @@ -45,6 +47,7 @@ public Selectionbar () {
>             }
> 
>             pause_btn.sensitive = false;
> +           App.app.selection_mode = false;
>          });
> 
>          remove_btn = new Gtk.Button.from_stock (Gtk.Stock.DELETE);

Ah ok, missed one of them.

> > ::: src/selectionbar.vala
> > @@ +142,2 @@
> >                  sensitive = false;
> > +                break;
> > 
> > unrelated fix.
> 
> I'll move it to a separate patch then. (This attachment)

Thanks. You want to resubmit the patch you took this out from (for the record).

> (In reply to comment #39)
> > Review of attachment 253539 [details] [review] [details]:
> > 
> > Again, the commit log is misleading. We are not just simply removing usage of
> > deprecated API but also changing the buttons to use text labels rather than
> > images, latter being the more important part of this patch actually.
> > 
> > Change itself is good but please do change commit log before pushing.
> 
> It was already using text labels before though, only the Pause button has this
> change.

1/2 is 50% :) I won't nitpick this to death but either you can update the log or split the unrelated change. You choice. :)
 
> (In reply to comment #41)
> > Review of attachment 253541 [details] [review] [details]:
> > 
> > I don't see you adding any space as the commit log says. I guess the style
> > takes care of that? If so, just update the commit log before pushing.
> 
> Oops, I switched to Gtk.HeaderBar just like how it is done in gnome-documents,
> I'll remove that in the commit message.
> 
> New commit log:
> 
> commit 9b445d6ea3a8a4cbe334d4266b1387d29871e88c
> Author: Arnel A. Borja <arnelborja@src.gnome.org>
> Date:   Fri Aug 30 00:11:13 2013 +0800
> 
>     selectionbar: Add button style classes
> 
>     Add "image-button" and "text-button" style classes to the buttons to
>     fix padding of buttons.
> 
>     This is to follow the new design for Content Selection Pattern at
>     https://wiki.gnome.org/GnomeOS/Design/Whiteboards/SelectionPattern
> 
>     https://bugzilla.gnome.org/show_bug.cgi?id=706818

Looks good now.

Created attachment 253607 [details] [review]
selectionbar: Exit selection mode after an action

(In reply to comment #44)
> > > ::: src/selectionbar.vala
> > > @@ +142,2 @@
> > >                  sensitive = false;
> > > +                break;
> > > 
> > > unrelated fix.
> > 
> > I'll move it to a separate patch then. (This attachment)
> 
> Thanks. You want to resubmit the patch you took this out from (for the record).

Attaching it here then.

Review of attachment 253607 [details] [review]:

ack

(In reply to comment #18)
> Also, I noticed that there is a bug in selection, I tried to fix that, but
> seems like there are other issues that I haven't found yet. Before, when I
> randomly toggle the selection with machines with some machines set as a
> favorite, they all set as favorites after some toggling. I used signal blocking
> to fix that, but now, when I click the favorites button, the selection changes
> and the favorite attribute is applied to random items.

Sorry, this one actually works. I was sleepy back then :D

Comment on attachment 253535 [details] [review]
topbar: Rename Done button and other fixes

Committed as 2fa9caa

Comment on attachment 253536 [details] [review]
topbar: Use text-button style class on New button

Committed as d38b8ab

Comment on attachment 253537 [details] [review]
selectionbar: Use a standard toolbar

Committed as b55b7cd

Comment on attachment 253607 [details] [review]
selectionbar: Exit selection mode after an action

Committed as 647d8ac

Comment on attachment 253602 [details] [review]
Break loop if already insensitive

Committed as 74b9cd2

Comment on attachment 253539 [details] [review]
selectionbar: Remove usage of stock items

Committed as 4524a5f

Comment on attachment 253540 [details] [review]
selectionbar: Use text label for Pause button

Committed as cb958a7

Comment on attachment 253541 [details] [review]
selectionbar: Add button style classes and spacing

Committed as e7199f3

Committed all of the patches. Thanks for reviewing them!

Just for reference, Matthias Clasen and Javier Jardón approved freeze break here:
https://mail.gnome.org/archives/release-team/2013-August/msg00090.html