Why the left-hand side?

(In reply to comment #1)
> Why the left-hand side?

To distinguish it from the user menu, and to make the top bar visually balanced (I presume).

Created attachment 222684 [details] [review]
Panel: clean up button construction code

We already could build the right part of the panel declaratively according
to the session mode. Extend that to handle the left and center parts.
Also, move the mapping from the roles to the classes in panel.js, as it shared
by all modes.

Created attachment 222685 [details] [review]
UserMenu: move to left and show a padlock icon in the screenshield

When the screen is locked, the IM presence is forced to idle (away), so
it's not helpful. Show a padlock instead, to indicate that password
or other authentication method is required.
Also, move the user menu to the left of the panel when in the lock screen.

https://bugzilla.gnome.org/show_bug.cgi?id=681143

Review of attachment 222684 [details] [review]:

This is nice. Few nits, otherwise, cool.

I wonder if we could make this configurable with a GSetting :)

::: js/ui/panel.js
@@ +912,3 @@
+    'lockScreen': imports.ui.status.lockScreenMenu.Indicator,
+    'keyboard': imports.ui.status.keyboard.InputSourceIndicator,
+    'userMenu': imports.ui.userMenu.UserMenuButton

You forgot PowerMenu, used by GDM?

@@ +1126,3 @@
 
+    _addToPanelBox: function(role, indicator, position, box) {
+        // XXX: why this loop to find the right position?

Have you tried removing it? It seems really sketchy, you're right.

@@ +1160,3 @@
+
+        position = position || 0;
+        boxContainer = this['_' + (box || 'right') + 'Box'];

First of all, missing a "let". Second of all, ugh.

const boxes = {
    left: this._leftBox,
    center: this._centerBox,
    right: this._rightBox
};
let boxContainer = boxes[box] || this._rightBox;

Review of attachment 222685 [details] [review]:

Add yet another tally to the "I'm quite sure we want a session mode". Not a fan.

(In reply to comment #6)
> Review of attachment 222685 [details] [review]:
> 
> Add yet another tally to the "I'm quite sure we want a session mode". Not a
> fan.

Having a clean dynamic session mode support means much more than just hiding some actors.
What if the new session mode changes hasOverview? Do you make the current overview object a dummy?
What about hasExtensions? Do you unload them? What if the change was from an extension?
What about createSession? Do you want to recreate the session and register network, polkit, mountoperation, automount, etc. again?

ScreenShield.State is the "session sub-mode", in all respects. I can move the signals and getters to SessionMode, if you want.

Forgot to add, this depends on bug 682542 for the state handling.

(In reply to comment #7)
> (In reply to comment #6)
> > Review of attachment 222685 [details] [review] [details]:
> > 
> > Add yet another tally to the "I'm quite sure we want a session mode". Not a
> > fan.
> 
> Having a clean dynamic session mode support means much more than just hiding
> some actors.
> What if the new session mode changes hasOverview? Do you make the current
> overview object a dummy?
> What about hasExtensions? Do you unload them? What if the change was from an
> extension?

I don't think we need to implement everything out of the gate, just not having 10 different ways to track the current settings of the sessions would be better. If we run into a transition we don't need to handle right now, we'll just log it and move on.

> What about createSession? Do you want to recreate the session and register
> network, polkit, mountoperation, automount, etc. again?

I've never liked the createSession approach.

> ScreenShield.State is the "session sub-mode", in all respects. I can move the
> signals and getters to SessionMode, if you want.

Created attachment 222693 [details] [review]
Panel: clean up button construction code

We already could build the right part of the panel declaratively according
to the session mode. Extend that to handle the left and center parts.
Also, move the mapping from the roles to the classes in panel.js, as it shared
by all modes.

Review of attachment 222693 [details] [review]:

Yes.

Review of attachment 222693 [details] [review]:

::: js/ui/panel.js
@@ +1108,3 @@
+        this._initBox(panel.left, this._leftBox);
+        this._initBox(panel.center, this._centerBox);
+        this._initBox(panel.right, this._rightBox);

this._initBox('left');
this._initBox('center');
this._initBox('right');

@@ +1112,3 @@
+
+    _initBox: function(elements, box) {
+        for (let i = 0; i < elements.length; i++) {

let elements = Main.sessionMode.panel[box], boxActor = boxes[box];

@@ +1150,3 @@
+            right: this._rightBox
+        };
+        let boxContainer = boxes[box] || this._rightBox;

Wait, no, what you pass as the 'box' parameter is the actor. That's not going to work here.

Review of attachment 222693 [details] [review]:

Nevermind, I misread. Good to go.

Comment on attachment 222693 [details] [review]
Panel: clean up button construction code

Attachment 222693 [details] pushed as e5534e8 - Panel: clean up button construction code

Comment on attachment 222685 [details] [review]
UserMenu: move to left and show a padlock icon in the screenshield

This is obsoleted by bug 683156.

And this is fixed!