Created attachment 251356 [details] [review]
popupMenu: Remove column widths

This code is too complicated to keep, and the last straw came after the
fixed width menu in the aggregate menu design.

This will break some existing popup menus that rely on the fixed width,
but this will soon be replaced with the aggregate menu. We'll also soon
clean this up further by replacing PopupBaseMenuItem's custom layout code
with an StBoxLayout.

Created attachment 251357 [details] [review]
panel: Use an hbox to lay out the app menu contents

This will make it easier to add the arrow to the app menu without
having to do the allocation logic ourselves.

Created attachment 251358 [details] [review]
status: Put arrow icons next to the separate status indicators

This is to indicate that it has a pulldown menu.

This is a part of the new system status design, see
https://wiki.gnome.org/GnomeShell/Design/Guidelines/SystemStatus/
for design details.

Created attachment 251359 [details] [review]
status: Port to a new SystemIndicator framework

We can't silently replace the old behavior of separate status
icons into a new system. Replace SystemStatusButton with a new
SystemIndicator class which will allow for the flexibility we
need. For now, make it a subclass of Button so that it mostly
feels the same, but we'll soon be swapping it out with a dummy
implementation that the aggregate menu will use.

I think the code cleanup here is worth it.

Created attachment 251360 [details] [review]
panel: Move statuses to the aggregate menu

Swap out the implementation of SystemIndicator with a dummy,
and build the aggregate menu. At the same time, remove the
poweroff and login screen menus, as those were fake aggregate
menus beforehand.

We lose some flexibility as we lose session-mode-based menu
layout, but as each component of the aggregate menu is supposed
to be "smart" in response to updating itself when session
state changes, I believe it's better than a declarative model.

Created attachment 251361 [details] [review]
panel: Align the arrows together in the status menus

To align the arrows, we need to allocate panel buttons the full
height of the tray. Fix up all of the panel buttons to support this,
and align the arrows in the middle.

Created attachment 251362 [details] [review]
system: Use the username if the user's name is too long

This is a part of the new system status design, see
https://wiki.gnome.org/GnomeShell/Design/Guidelines/SystemStatus/
for design details.

Created attachment 251363 [details] [review]
panelMenu: Show/hide the indicators box based on indicator visibility

This ensures that there's no empty space in the indicators box, and we don't
have to manage it manually.

Created attachment 251364 [details] [review]
status: Add new brightness slider widget

This is a simple slider that shows the current brightness of the
screen, and offers a way to change it.

This is a part of the new system status design, see
https://wiki.gnome.org/GnomeShell/Design/Guidelines/SystemStatus/
for design details.

Created attachment 251365 [details] [review]
status: Add new airplane mode indicator / menu

This is a simple indicator that shows if the user is currently in
airplane mode, and if they are, offers a way to turn it off. It
will not be shown if the user is not in airplane mode.

This is a part of the new system status design, see
https://wiki.gnome.org/GnomeShell/Design/Guidelines/SystemStatus/
for design details.

Created attachment 251366 [details] [review]
network: Remove superfluous intermediate section

Now that we're guaranteed this.menu is a section, this is
excessive and unnecessary.

Created attachment 251367 [details] [review]
power: Move the Power Off indicator to the power menu

It's only supposed to show if we have a battery, and hooking into
the power system is the easiest way of making that happen.

Created attachment 251368 [details] [review]
panelMenu: Remove the gicon parameter from addIndicator, and make private

There's only two uses of the parameter left, which can easily be added as a
separate line below. Since it's really a private interface meant for the
indicators, make it private as well so external users are less likely to
use it.

Created attachment 251369 [details] [review]
theme: Restrict the aggregate menu to 320px

Like in the designs. This also requires that we don't put a min-width
on sliders, as 15em is smaller than 320px.

Created attachment 251370 [details] [review]
popupMenu: Remove our custom allocation code

With support for column-based layout gone, simply use a box layout
and allow items to use their own layouts without any "framework".

Created attachment 251371 [details] [review]
system: Add an orientation lock action button

Review of attachment 251356 [details] [review]:

Ok (provided that the region panel is fixed, and that places-menu doesn't regress - a patch is missing for that)

Review of attachment 251357 [details] [review]:

::: js/ui/panel.js
@@ +214,2 @@
         this._label = new TextShadower();
+        this._label.actor.y_align = Clutter.ActorAlign.CENTER;

Actor property, not child property?

Review of attachment 251358 [details] [review]:

::: js/ui/status/keyboard.js
@@ +340,3 @@
+        this._hbox = new St.BoxLayout({ style_class: 'panel-status-menu-box' });
+        this._hbox.add_child(this._container);
+        this._hbox.add(new St.Label({ text: '\u25BE' }));

You're not consistent in .add(), .add_child() and .add_actor()...

Review of attachment 251359 [details] [review]:

I told you before, please leave SystemStatusButton alone, and add a new class SystemIndicator. Too many extensions are using SystemStatusButton, and I don't want to break them.

Review of attachment 251360 [details] [review]:

Ok

Review of attachment 251361 [details] [review]:

::: js/ui/panelMenu.js
@@ -93,3 @@
-            childBox.y1 = 0;
-            childBox.y2 = availHeight;
-        }

Isn't this code already supposed to center align stuff?

Review of attachment 251362 [details] [review]:

::: js/ui/status/system.js
@@ +144,3 @@
+        // a size cache issue or something. Moving this to be a layout
+        // manager would be a much better idea.
+        clutterText.get_allocation_box();

Did you try to investigate what's happening here?

@@ +202,3 @@
+        // the popup menu, and we can't easily connect on allocation-changed
+        // or notify::width without creating layout cycles, simply update the
+        // label whenever the menu is opened.

Isn't the width of the popup menu supposed to be constant?
Also, can't we query the geometry of the layout before allocating?

Review of attachment 251363 [details] [review]:

Ok

Review of attachment 251364 [details] [review]:

As already mentioned in IRC, please make this async.

Review of attachment 251365 [details] [review]:

Looks good.

Review of attachment 251366 [details] [review]:

Yay!

Review of attachment 251367 [details] [review]:

Ok

Review of attachment 251368 [details] [review]:

::: js/ui/panelMenu.js
@@ +253,3 @@
 
+    _addIndicator: function() {
+        let icon = new St.Icon({ style_class: 'system-status-icon' });

I'd rather have the icon_name at least, if you don't want a full GIcon, as we usually build the objects with the right properties immediately, rather than building and then immediately setting.

Review of attachment 251369 [details] [review]:

Ok

Review of attachment 251369 [details] [review]:

No wait, this in a way breaks with Large text (the menu doesn't resize, and I get my username instead of my real name, which was previous fitting). Is that the behavior we want?

Review of attachment 251370 [details] [review]:

1) Are we losing the alignment parameters? Does it mean that stuff is always left aligned?
2) Please add backward compatibily API for extensions, I know it's just a mechanical replace, but it becomes really hard when you use one version of the code for several major shell versions (and many authors do that)

::: js/ui/popupMenu.js
@@ +1047,3 @@
 
+        let expander = new St.Bin({ style_class: 'popup-menu-item-expander' });
+        this.actor.add(expander, { expand: true });

This really looks like a hack...

Review of attachment 251371 [details] [review]:

::: js/ui/status/system.js
@@ +107,3 @@
+                                        this._orentationExists = false;
+                                        this._updateOrientationLock();
+                                    }));

Wait, so the API is that the bus name appears and disappears?
Can't we have a property instead?

(In reply to comment #18)
> Actor property, not child property?

ClutterActor takes care of alignment when you allocate it more space than the actor requests. You don't need to use a "new-style" actor or be allocated by a layout manager to make the alignment properties work.

(In reply to comment #19)
> You're not consistent in .add(), .add_child() and .add_actor()...

This one slipped under the radar because we replace it later.

(In reply to comment #20)
> I told you before, please leave SystemStatusButton alone, and add a new class
> SystemIndicator. Too many extensions are using SystemStatusButton, and I don't
> want to break them.

No.

(In reply to comment #22)
> Isn't this code already supposed to center align stuff?

Yes, but only in its own local world. If the icons are 16px tall and the text is 24px tall, the arrows won't be aligned to each other.

(In reply to comment #23)
> Did you try to investigate what's happening here?

How else did you think I discovered that get_allocation_box() miraculously made the bug disappear?

> @@ +202,3 @@
> Isn't the width of the popup menu supposed to be constant?

Yes, but according to the theme. I suppose we could cache the behavior and reset it in a style-changed handler, causing it to relayout on the next open, but ugh. And it's not just the width of the menu, but the icons, the spacing of the box, the padding of the box pointer... feel free to suggest something better than just running the existing allocation code to determine all that.

> Also, can't we query the geometry of the layout before allocating?

We can cache the width of the layout, sure, but Pango already has a layout cache so it really isn't that expensive to re-measure.

(In reply to comment #29)
> I'd rather have the icon_name at least, if you don't want a full GIcon, as we
> usually build the objects with the right properties immediately, rather than
> building and then immediately setting.

"usually" here means twice out of five constructions.

(In reply to comment #31)
> No wait, this in a way breaks with Large text (the menu doesn't resize, and I
> get my username instead of my real name, which was previous fitting). Is that
> the behavior we want?

Yes.

(In reply to comment #32)
> Review of attachment 251370 [details] [review]:
> 
> 1) Are we losing the alignment parameters? Does it mean that stuff is always
> left aligned?

No.

> 2) Please add backward compatibily API for extensions, I know it's just a
> mechanical replace, but it becomes really hard when you use one version of the
> code for several major shell versions (and many authors do that)

No.

> This really looks like a hack...

It's what we do everywhere else we want things aligned on the left and right side of a box, which can be found in most modal dialogs and the looking glass. I can remove the style class, I suppose. I think I used to have a min-width there before realizing that spacing took care of it.

(In reply to comment #33)
> Wait, so the API is that the bus name appears and disappears?
> Can't we have a property instead?

Technically, either the bus will be there at g-s-d startup or it won't. g-s-d doesn't handle the hotplug case right now.

You can talk to Bastien about this, but I think it's a decent enough API.

Comment on attachment 251356 [details] [review]
popupMenu: Remove column widths

Attachment 251356 [details] pushed as c1fb1ba - popupMenu: Remove column widths


This has no API break, so I'm going to push it.

Created attachment 251404 [details] [review]
system: Fix showing the default avatar when the user has none

grr typos

Created attachment 251405 [details] [review]
system: Hide the Log Out / Switch User items in the lock screen

Created attachment 251406 [details] [review]
system: Add an orientation lock action button

Created attachment 251407 [details] [review]
system: Use the username if the user's name is too long

This is a part of the new system status design, see
https://wiki.gnome.org/GnomeShell/Design/Guidelines/SystemStatus/
for design details.

Created attachment 251408 [details] [review]
system: Remove rogue separator in lock screen

Hide the actions box when none are showing.

Review of attachment 251406 [details] [review]:

restoring old status as nothing has changed

Review of attachment 251407 [details] [review]:

restoring old status as nothing has changed

Created attachment 251409 [details] [review]
popupMenu: Remove our custom allocation code

With support for column-based layout gone, simply use a box layout
and allow items to use their own layouts without any "framework".

Created attachment 251410 [details] [review]
status: Add new airplane mode indicator / menu

This is a simple indicator that shows if the user is currently in
airplane mode, and if they are, offers a way to turn it off. It
will not be shown if the user is not in airplane mode.

This is a part of the new system status design, see
https://wiki.gnome.org/GnomeShell/Design/Guidelines/SystemStatus/
for design details.

Created attachment 251411 [details] [review]
status: Add new brightness slider widget

This is a simple slider that shows the current brightness of the
screen, and offers a way to change it.

This is a part of the new system status design, see
https://wiki.gnome.org/GnomeShell/Design/Guidelines/SystemStatus/
for design details.

Review of attachment 251404 [details] [review]:

Yes

Review of attachment 251405 [details] [review]:

Yes.

Review of attachment 251406 [details] [review]:

Yeah, ok.

Review of attachment 251407 [details] [review]:

In theory, we could have our own ClutterActor, implementing the fixed width with the theme width in get_preferred_*, and blocking the queue-relayout signal to avoid cycles.
This one is easier though.

::: js/ui/status/system.js
@@ +159,3 @@
+        // a size cache issue or something. Moving this to be a layout
+        // manager would be a much better idea.
+        clutterText.get_allocation_box();

Yeah, it would be better to have a longer explanation of what's happened.
"Probably a size cache issue or something" seems wild guessing to me, and won't help someone in the future looking at this code.

Review of attachment 251408 [details] [review]:

Yes.

Review of attachment 251409 [details] [review]:

Ok

Review of attachment 251410 [details] [review]:

Ok

Review of attachment 251411 [details] [review]:

acn because it's not a shell problem

::: js/ui/status/brightness.js
@@ +52,3 @@
+    _sliderChanged: function(slider, value) {
+        let percent = value * 100;
+        this._proxy.Brightness = percent;

I must say i'm not fond of the new property based API, because we get no notification of the new brightness (which can be different from the one we ask for due to rounding) until we get the signal, so the slider can jump around a little.

(In reply to comment #49)
> Review of attachment 251407 [details] [review]:
> 
> In theory, we could have our own ClutterActor, implementing the fixed width
> with the theme width in get_preferred_*, and blocking the queue-relayout signal
> to avoid cycles.

The better approach is to have a layout manager that allocates one of two labels depending on the box we're given and the size of both labels. But the problem is that it's hard to put that into the PopupSubMenu; I can't really subclass it effectively, and I can't make another class that's a submenu since we "instanceof PopupSubMenu" everywhere.

> ::: js/ui/status/system.js
> @@ +159,3 @@
> +        // a size cache issue or something. Moving this to be a layout
> +        // manager would be a much better idea.
> +        clutterText.get_allocation_box();

The problem is that it's impossible to even start debugging when simply trying to print out values fixes it.

(In reply to comment #53)
> I must say i'm not fond of the new property based API, because we get no
> notification of the new brightness (which can be different from the one we ask
> for due to rounding) until we get the signal, so the slider can jump around a
> little.

How would you prevent that, even without a property-based API? Don't update due to drags and simply update due to properties, so that it snaps to the rounded brightness? Only update for a property update if we're not dragging? Update the slider position when the menu is opened, and not elsewhere (we'd have to make sure the user can't press brightness keys while the menu is open)

Attachment 251357 [details] pushed as a45cc0a - panel: Use an hbox to lay out the app menu contents
Attachment 251358 [details] pushed as a347f02 - status: Put arrow icons next to the separate status indicators
Attachment 251359 [details] pushed as 5cca26a - status: Port to a new SystemIndicator framework
Attachment 251360 [details] pushed as 5c8c4e0 - panel: Move statuses to the aggregate menu
Attachment 251361 [details] pushed as 54bec54 - panel: Align the arrows together in the status menus
Attachment 251363 [details] pushed as 4ba6791 - panelMenu: Show/hide the indicators box based on indicator visibility
Attachment 251366 [details] pushed as c05627a - network: Remove superfluous intermediate section
Attachment 251367 [details] pushed as 37487c2 - power: Move the Power Off indicator to the power menu
Attachment 251368 [details] pushed as 73e1f23 - panelMenu: Remove the gicon parameter from addIndicator, and make private
Attachment 251369 [details] pushed as 978ab2c - theme: Restrict the aggregate menu to 320px
Attachment 251404 [details] pushed as 79dcb03 - system: Fix showing the default avatar when the user has none
Attachment 251405 [details] pushed as 5a06b34 - system: Hide the Log Out / Switch User items in the lock screen
Attachment 251406 [details] pushed as e76bcce - system: Add an orientation lock action button
Attachment 251407 [details] pushed as 3e4d095 - system: Use the username if the user's name is too long
Attachment 251408 [details] pushed as 52ec5cf - system: Remove rogue separator in lock screen
Attachment 251409 [details] pushed as 5148539 - popupMenu: Remove our custom allocation code
Attachment 251410 [details] pushed as 6bd2756 - status: Add new airplane mode indicator / menu
Attachment 251411 [details] pushed as cb09ae5 - status: Add new brightness slider widget


Pushing this for now. Please file any additional bugs you might find, or any complaints about this patch stack you might have.