Created attachment 222032 [details] [review]
Exposing percentage label

As I said on the comment, ideally we want to expose two different labels (this also had become a need for other bugs too). But while I work on that, I think that at least exposing the percentage level will be enough (we already have some context as that percentage is exposed on the power menu)

Created attachment 222034 [details] [review]
Proper state for items with a dot

This state is used to indicate that that item is checked, so "selected". The poster boy here is the current wifi selected. Without this patch the only difference between the selected wifi and the rest of available wifis are that current wifi is grayed out.

Review of attachment 222034 [details] [review]:

Yes.

Review of attachment 222032 [details] [review]:

Sure.

After all the changes on the UI I have found some more ui-elements to review:

 * The new element at the end of the Dash (replacement for the All programs tab) doesn't have a label.
 * Notifications are now keyboard navigable, but also lack a label (but I will manage that on bug 677229)

Created attachment 222347 [details] [review]
Expose toggle role if button is in toggle mode

It seems that StButton has a toggle mode now. It would be convenient to expose ATK_ROLE_TOGGLE_BUTTON if it is in that mode.

Created attachment 222348 [details] [review]
set a proper label for the show applications dash button

This was easy because the button already had a string with the name.

Review of attachment 222347 [details] [review]:

Sure - on a side note, the toggle mode is not at all a new addition, looks like we just overlooked it earlier :-)

::: src/st/st-button.c
@@ +785,3 @@
+    atk_object_set_role (accessible, ATK_ROLE_TOGGLE_BUTTON);
+  else
+    atk_object_set_role (accessible, ATK_ROLE_PUSH_BUTTON);

atk_object_set_role (accessible,
                                 button->priv->is_toggle ? ATK_ROLE_TOGGLE_BUTTON
                                                                       : ATK_ROLE_PUSH_BUTTON);
would be slightly more concise, but OK as-is of course

Review of attachment 222348 [details] [review]:

::: js/ui/dash.js
@@ +237,3 @@
     _init: function() {
         this.parent();
+        this.title = _("Show Applications");

Why is title public?

@@ +252,3 @@
         this.setChild(this.toggleButton);
         this.setHover(false);
+        this.toggleButton.label_actor = new St.Label({ text: this.title });

I wonder - would doing
  this.toggleButton.label_actor = this.label;
work?

(In reply to comment #9)
> @@ +252,3 @@
>          this.setChild(this.toggleButton);
>          this.setHover(false);
> +        this.toggleButton.label_actor = new St.Label({ text: this.title });
> 
> I wonder - would doing
>   this.toggleButton.label_actor = this.label;
> work?

Answering my own question: yes, you can do that, so let's go with that.

(In reply to comment #8)
> Review of attachment 222347 [details] [review]:
> 
> Sure - on a side note, the toggle mode is not at all a new addition, looks like
> we just overlooked it earlier :-)
> 
> ::: src/st/st-button.c
> @@ +785,3 @@
> +    atk_object_set_role (accessible, ATK_ROLE_TOGGLE_BUTTON);
> +  else
> +    atk_object_set_role (accessible, ATK_ROLE_PUSH_BUTTON);
> 
> atk_object_set_role (accessible,
>                                  button->priv->is_toggle ?
> ATK_ROLE_TOGGLE_BUTTON
>                                                                        :
> ATK_ROLE_PUSH_BUTTON);
> would be slightly more concise, but OK as-is of course

Committed using this change.

(In reply to comment #10)
> (In reply to comment #9)
> > @@ +252,3 @@
> >          this.setChild(this.toggleButton);
> >          this.setHover(false);
> > +        this.toggleButton.label_actor = new St.Label({ text: this.title });
> > 
> > I wonder - would doing
> >   this.toggleButton.label_actor = this.label;
> > work?
> 
> Answering my own question: yes, you can do that, so let's go with that.

I pushed a commit using that suggestion. Thanks.

This bug was opened as a general review, that was done, and all patches comitted. Other stuff are already being tracked by individual bugs.

Closing as FIXED.

(In reply to comment #13)
> This bug was opened as a general review, that was done, and all patches
> comitted. Other stuff are already being tracked by individual bugs.
> 
> Closing as FIXED.

But I didn't close it. Closing it now. Sorry for the noise.