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 676156 - generalize --gdm-mode to 'kiosk mode'
generalize --gdm-mode to 'kiosk mode'
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
Depends on:
Blocks:
 
 
Reported: 2012-05-16 12:35 UTC by Matthias Clasen
Modified: 2012-05-22 17:50 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
main: Add generic --mode switch (1.87 KB, patch)
2012-05-18 16:09 UTC, Florian Müllner
accepted-commit_now Details | Review
global: Add session-mode property (4.99 KB, patch)
2012-05-18 16:10 UTC, Florian Müllner
accepted-commit_now Details | Review
Delegate mode information to a dedicated object (7.97 KB, patch)
2012-05-18 16:10 UTC, Florian Müllner
needs-work Details | Review
global: Remove session_type property (4.07 KB, patch)
2012-05-18 16:10 UTC, Florian Müllner
accepted-commit_now Details | Review
sessionMode: Add hasOverview property (4.53 KB, patch)
2012-05-18 16:10 UTC, Florian Müllner
reviewed Details | Review
sessionMode: Add hasAppMenu property (1.71 KB, patch)
2012-05-18 16:10 UTC, Florian Müllner
accepted-commit_now Details | Review
sessionMode: Add showCalendarEvents property (3.10 KB, patch)
2012-05-18 16:10 UTC, Florian Müllner
accepted-commit_now Details | Review
sessionMode: Add allowSettings property (4.08 KB, patch)
2012-05-18 16:10 UTC, Florian Müllner
reviewed Details | Review
sessionMode: Add allowExtensions property (2.29 KB, patch)
2012-05-18 16:10 UTC, Florian Müllner
accepted-commit_now Details | Review
sessionMode: Add allowKeybindingsWhenModal property (2.78 KB, patch)
2012-05-18 16:10 UTC, Florian Müllner
reviewed Details | Review
sessionMode: Add hasRunDialog property (2.73 KB, patch)
2012-05-18 16:10 UTC, Florian Müllner
accepted-commit_now Details | Review
sessionMode: Add hasWorkspaces property (3.62 KB, patch)
2012-05-18 16:10 UTC, Florian Müllner
reviewed Details | Review
main: Remove _initUserSession() (1.54 KB, patch)
2012-05-18 16:10 UTC, Florian Müllner
accepted-commit_now Details | Review
sessionMode: Add createSession() hook (3.01 KB, patch)
2012-05-18 16:11 UTC, Florian Müllner
needs-work Details | Review
sessionMode: Add getExtraStylesheets() hook (3.04 KB, patch)
2012-05-18 16:11 UTC, Florian Müllner
needs-work Details | Review
sessionMode: Add getStatusArea() hook (6.72 KB, patch)
2012-05-18 16:11 UTC, Florian Müllner
needs-work Details | Review
Remove SessionType type (2.12 KB, patch)
2012-05-18 16:11 UTC, Florian Müllner
accepted-commit_now Details | Review
main: Add a --list-modes command line option (3.95 KB, patch)
2012-05-18 16:11 UTC, Florian Müllner
needs-work Details | Review
main: Add generic --mode switch (1.87 KB, patch)
2012-05-18 22:49 UTC, Florian Müllner
committed Details | Review
global: Add session-mode property (4.99 KB, patch)
2012-05-18 22:49 UTC, Florian Müllner
committed Details | Review
Delegate mode information to a dedicated object (7.96 KB, patch)
2012-05-18 22:50 UTC, Florian Müllner
committed Details | Review
main: Make sure that --mode parameter is valid (2.83 KB, patch)
2012-05-18 22:50 UTC, Florian Müllner
committed Details | Review
global: Remove session_type property (4.08 KB, patch)
2012-05-18 22:50 UTC, Florian Müllner
committed Details | Review
sessionMode: Add hasOverview property (4.29 KB, patch)
2012-05-18 22:53 UTC, Florian Müllner
committed Details | Review
sessionMode: Add hasAppMenu property (1.44 KB, patch)
2012-05-18 22:53 UTC, Florian Müllner
committed Details | Review
sessionMode: Add showCalendarEvents property (2.83 KB, patch)
2012-05-18 22:53 UTC, Florian Müllner
committed Details | Review
sessionMode: Add allowSettings property (3.78 KB, patch)
2012-05-18 22:56 UTC, Florian Müllner
committed Details | Review
sessionMode: Add allowExtensions property (1.96 KB, patch)
2012-05-18 22:56 UTC, Florian Müllner
committed Details | Review
sessionMode: Add allowKeybindingsWhenModal property (2.40 KB, patch)
2012-05-18 22:56 UTC, Florian Müllner
committed Details | Review
sessionMode: Add hasRunDialog property (2.39 KB, patch)
2012-05-18 22:56 UTC, Florian Müllner
committed Details | Review
sessionMode: Add hasWorkspaces property (3.28 KB, patch)
2012-05-18 23:03 UTC, Florian Müllner
committed Details | Review
main: Remove _initUserSession() (1.54 KB, patch)
2012-05-18 23:04 UTC, Florian Müllner
committed Details | Review
sessionMode: Add createSession() hook (2.53 KB, patch)
2012-05-18 23:04 UTC, Florian Müllner
none Details | Review
sessionMode: Add extraStylesheet property (2.24 KB, patch)
2012-05-18 23:04 UTC, Florian Müllner
committed Details | Review
sessionMode: Add statusArea property (7.29 KB, patch)
2012-05-18 23:07 UTC, Florian Müllner
needs-work Details | Review
Remove SessionType type (2.01 KB, patch)
2012-05-18 23:07 UTC, Florian Müllner
committed Details | Review
main: Add a --list-modes command line option (4.38 KB, patch)
2012-05-18 23:07 UTC, Florian Müllner
committed Details | Review
sessionMode: Add createSession() hook (2.97 KB, patch)
2012-05-18 23:34 UTC, Florian Müllner
committed Details | Review
sessionMode: Add statusArea property (7.01 KB, patch)
2012-05-18 23:35 UTC, Florian Müllner
committed Details | Review

Description Matthias Clasen 2012-05-16 12:35:13 UTC
On the login screen, we use the shell in a 'non-personalized' and single-application mode, where it 
- does not show the user menu
- does not have the overview
- does not show app menus

The single app that it runs on the login screen is the greeter (though not technically a separate application).

There are other situations where the non-personalized and single-application aspects of this mode make sense, minus the greeter. One of them is the initial-setup work that is happening for 3.6. Another one is using GNOME for customized point-of-sale or similar kiosk-style applications - GNOME 2 was fairly amenable to this kind of customization, and people are using it in this way.

The --gdm-mode commandline switch should be turned into a slightly more general --mode='gdm' which would allow for other modes like --mode='kiosk' that would do away with the greeter.
Comment 1 Jasper St. Pierre (not reading bugmail) 2012-05-16 14:24:31 UTC
The first idea I have is to remove the flag-like approach and have separate JS files that hook up the various components.

So, the shell could ship a shellLayout.js that hooks up the message tray, panel, status area, applications menu button, etc.

gdm could ship a gdmLayout.js that has a few other things removed.

The biggest issue that I see with some of this is that a lot of components reference "imports.ui.main" as a sort of global name. Perhaps we can have some magic that injects a Main around. Ideally, of course, we stop using globals everywhere and pass around everything we need, but that's not going to happen anytime soon without a major restructuring.
Comment 2 Matthias Clasen 2012-05-16 14:30:27 UTC
As I said to florian in irc earlier: I'm not sure how far we want to go from shell as 'app with modes' to 'shell as platform'. Or if we want to go in that direction at all, in fact. I just need --mode=kiosk to work.
Comment 3 Florian Müllner 2012-05-18 16:09:55 UTC
Created attachment 214370 [details] [review]
main: Add generic --mode switch

We are going to need "special" modes other than GDM, for instance
for the initial setup[0]. Rather than adding one command line switch
per mode (and logic to deal with multiple switches being given at
the same time), add a generic --mode switch to specify the mode as
string.

[0] https://live.gnome.org/GnomeOS/Design/Whiteboards/InitialSetup
Comment 4 Florian Müllner 2012-05-18 16:10:00 UTC
Created attachment 214371 [details] [review]
global: Add session-mode property

Add a session-mode property on ShellGlobal which corresponds to the
new --mode switch. Make the existing ShellGlobal:session-type property
readonly and base it on ShellGlobal:session-mode to avoid conflicts.
Comment 5 Florian Müllner 2012-05-18 16:10:04 UTC
Created attachment 214372 [details] [review]
Delegate mode information to a dedicated object

Rather than accessing global.session_type / global.session_mode
all over the place, delegate mode information to a dedicated
sessionMode object. While not very useful for now, we will replace
checks for a particular mode with checks for particular properties
that sessionMode defines based on global.session_mode.
Comment 6 Florian Müllner 2012-05-18 16:10:09 UTC
Created attachment 214373 [details] [review]
global: Remove session_type property

ShellGlobal:session-type is now unsed, remove it.
Comment 7 Florian Müllner 2012-05-18 16:10:14 UTC
Created attachment 214374 [details] [review]
sessionMode: Add hasOverview property

Add a sessionMode.hasOverview property, which determines whether
the mode should give access to the overview or not.
Comment 8 Florian Müllner 2012-05-18 16:10:18 UTC
Created attachment 214375 [details] [review]
sessionMode: Add hasAppMenu property

Add a sessionMode.hasAppMenu property, which determines whether
the top bar should contain a menu for the active application or
not.
Comment 9 Florian Müllner 2012-05-18 16:10:23 UTC
Created attachment 214376 [details] [review]
sessionMode: Add showCalendarEvents property

Add a sessionMode.showCalendarEvents property, which determines
whether the calendar menu should contain an events section or not.
Comment 10 Florian Müllner 2012-05-18 16:10:28 UTC
Created attachment 214377 [details] [review]
sessionMode: Add allowSettings property

Add a sessionMode.allowSettings property, which determines whether
menus in the top bar should allow access to System Settings or not.
Comment 11 Florian Müllner 2012-05-18 16:10:33 UTC
Created attachment 214378 [details] [review]
sessionMode: Add allowExtensions property

Add a sessionMode.allowExtensions property, which determines whether
installed and enabled extensions should be loaded or ignored.
Comment 12 Florian Müllner 2012-05-18 16:10:38 UTC
Created attachment 214379 [details] [review]
sessionMode: Add allowKeybindingsWhenModal property

Add a sessionMode.allowKeybindingsWhenModal property, which determines
whether keybindings should still be handled while a modal dialog is
up or not.
Comment 13 Florian Müllner 2012-05-18 16:10:43 UTC
Created attachment 214380 [details] [review]
sessionMode: Add hasRunDialog property

Add a sessionMode.hasRunDialog property, which determines whether
the keyboard shortcut to access the run dialog should be active
or not.
Comment 14 Florian Müllner 2012-05-18 16:10:48 UTC
Created attachment 214381 [details] [review]
sessionMode: Add hasWorkspaces property

Add a sessionMode.hasWorkspaces property, which determines whether
the concept of workspaces should be exposed in the interface or not.
Comment 15 Florian Müllner 2012-05-18 16:10:53 UTC
Created attachment 214382 [details] [review]
main: Remove _initUserSession()

In Shell.SessionType.USER mode, two separate setup functions were
used during startup. With the new feature-based checks, the second
one is now almost empty, so move its remaining code into the first
function and remove it.
Comment 16 Florian Müllner 2012-05-18 16:11:00 UTC
Created attachment 214383 [details] [review]
sessionMode: Add createSession() hook

Add a sessionMode.createSession() hook, which is executed during
startup to setup mode-specific stuff.
Comment 17 Florian Müllner 2012-05-18 16:11:10 UTC
Created attachment 214384 [details] [review]
sessionMode: Add getExtraStylesheets() hook

Add a sessionMode.getExtraStylesheets() hook, which is called to
load mode-specific stylesheets (if any).
Comment 18 Florian Müllner 2012-05-18 16:11:16 UTC
Created attachment 214385 [details] [review]
sessionMode: Add getStatusArea() hook

Add a sessionMode.getStatusArea() hook, which is called to determine
the list of items in the top bar's status area.
Comment 19 Florian Müllner 2012-05-18 16:11:21 UTC
Created attachment 214386 [details] [review]
Remove SessionType type

SessionType has now completely been replaced by the finer-grained
session-mode, so remove the remaining type definition.
Comment 20 Florian Müllner 2012-05-18 16:11:26 UTC
Created attachment 214387 [details] [review]
main: Add a --list-modes command line option

Add a command line option to list available modes, i.e. possible
arguments for the --mode switch.
Comment 21 Florian Müllner 2012-05-18 16:34:09 UTC
(In reply to comment #1)
> The first idea I have is to remove the flag-like approach and have separate JS
> files that hook up the various components.

I don't think that approach would work too well; there are no real differences in the actual layout (except of course for gdm's login dialog, but that one is already well separated from the rest of the code base), rather a bunch of tweaks and adjustments to existing components.

So with your suggestion, we'll probably end up adding extra parameters to components (think "hideSettingItems" for PopupMenus) and duplicate the "normal" layout with a few of those parameters tweaked (with all the maintenance problems that will pop up when the duplicated versions start differing).

Therefore I went with a modified flag approach here - rather than having checks for particular modes all over the place, each mode is represented by series of "feature" properties (e.g. whether there should be an overview / app menu, what status icons to show etc.), so the components now only use checks for particular behavior tweaks that concern them. The actual mapping from mode to behavior is done centrally now, which should be a lot cleaner than the original approach.
Comment 22 Jasper St. Pierre (not reading bugmail) 2012-05-18 17:24:34 UTC
    function loginMode() {
        Main.panel.leftBox.add(new Panel.ActivitiesButton());
        Main.panel.centerBox.add(new DateMenu.DateMenu());
        Main.panel.addStatusMenu(new A11yMenu.A11yMenu());
        Main.panel.addStatusMenu(new VolumeMenu.VolumeMenu());
        // ...
        Main.layoutManager.addChrome(new Gdm.GdmLoginScreen());
    }

I don't think that's too difficult.
Comment 23 Florian Müllner 2012-05-18 17:27:48 UTC
(In reply to comment #22)
>     function loginMode() {
>         Main.panel.leftBox.add(new Panel.ActivitiesButton());
>         Main.panel.centerBox.add(new DateMenu.DateMenu());
>         Main.panel.addStatusMenu(new A11yMenu.A11yMenu());
>         Main.panel.addStatusMenu(new VolumeMenu.VolumeMenu());
>         // ...
>         Main.layoutManager.addChrome(new Gdm.GdmLoginScreen());
>     }
> 
> I don't think that's too difficult.

Except that it should be:

  function loginMode() {
        Main.panel.leftBox.add(new Panel.ActivitiesButton({ disableSettings: true }));
         Main.panel.centerBox.add(new DateMenu.DateMenu({ disableSettings: true }));
         Main.panel.addStatusMenu(new A11yMenu.A11yMenu({ disableSettings: true }));
         Main.panel.addStatusMenu(new VolumeMenu.VolumeMenu({ disableSetting: true }));
         // ...
         Main.layoutManager.addChrome(new Gdm.GdmLoginScreen());
  }

  function userMode() {
        Main.panel.leftBox.add(new Panel.ActivitiesButton());
         Main.panel.centerBox.add(new DateMenu.DateMenu());
         Main.panel.addStatusMenu(new A11yMenu.A11yMenu());
         Main.panel.addStatusMenu(new VolumeMenu.VolumeMenu());
         // ...
  }


Which is what I call code duplication :-)
Comment 24 Florian Müllner 2012-05-18 17:29:30 UTC
Note that I *did* add a SessionMode.createSession() hook, which should be used to set up specifics; I just don't think it is practical to have modes set up *everything* there.
Comment 25 Jasper St. Pierre (not reading bugmail) 2012-05-18 18:17:27 UTC
Review of attachment 214370 [details] [review]:

Sure.
Comment 26 Jasper St. Pierre (not reading bugmail) 2012-05-18 18:21:38 UTC
Review of attachment 214383 [details] [review]:

::: js/ui/sessionMode.js
@@ +54,3 @@
+
+    createSession: function() {
+        if (typeof this._createSession == "function")

We don't explicitly check for functions anywhere else throughout gnome-shell -- we expect people to pass the correct thing at the correct time. Why here?

(Also, is there a reason you didn't move Main.createUserSession/Main.createGDMsession into here?)
Comment 27 Jasper St. Pierre (not reading bugmail) 2012-05-18 18:22:55 UTC
Review of attachment 214384 [details] [review]:

::: js/ui/sessionMode.js
@@ +19,3 @@
              hasWorkspaces: false,
              createSession: Main.createGDMSession,
+             getExtraStylesheetsFunc: function() {

Why is this a function rather than:

    extraStylesheets: [global.datadir + '/theme/gdm.css']

? And why is it a list?
Comment 28 Jasper St. Pierre (not reading bugmail) 2012-05-18 18:25:54 UTC
Review of attachment 214385 [details] [review]:

::: js/ui/panel.js
@@ +957,3 @@
         /* right */
+        let [order, implementation] = Main.sessionMode.getStatusArea();
+        this._status_area_order = order;

While we're here, can we fix up the bad variable name style here?

::: js/ui/sessionMode.js
@@ +56,3 @@
                  return [global.datadir + '/theme/gdm.css'];
              },
+             getStatusArea: function() {

Why is this a function?

@@ +117,3 @@
+            if (typeof o == "object" && typeof o.length == "number" &&
+                typeof i == "object")
+                return [o, i];

Again, why are we doing type checks here?
Comment 29 Jasper St. Pierre (not reading bugmail) 2012-05-18 18:26:20 UTC
Review of attachment 214386 [details] [review]:

Sure.
Comment 30 Jasper St. Pierre (not reading bugmail) 2012-05-18 18:29:15 UTC
Review of attachment 214387 [details] [review]:

::: src/main.c
@@ +280,3 @@
+   * We don't *really* need anything in GTK+, so just mute those - otherwise
+   * we would still get warnings when executed from the CLI. */
+  g_log_set_default_handler (shut_up, NULL);

Ew. No. Just initialize GTK+.

@@ +290,3 @@
+  script = "imports.ui.environment.init();"
+           "print(Object.getOwnPropertyNames(imports.ui.sessionMode._modes));";
+  gjs_context_eval (context, script, strlen (script), "<main>", &status, NULL);

Do we want to start the precedent of accessing private members? Also, the list of modes isn't very pretty:

gnome-shell,gdm

Consider adding a prettyListModes() function to sessionMode that prints each on its own line.
Comment 31 Jasper St. Pierre (not reading bugmail) 2012-05-18 18:31:07 UTC
Review of attachment 214374 [details] [review]:

::: js/ui/main.js
@@ +231,3 @@
+
+    if (sessionMode.hasOverview) {
+        Meta.keybindings_set_custom_handler('panel-main-menu', function () {

Is there a reason the overview can't handle this itself?
Comment 32 Jasper St. Pierre (not reading bugmail) 2012-05-18 18:31:38 UTC
Review of attachment 214375 [details] [review]:

.
Comment 33 Jasper St. Pierre (not reading bugmail) 2012-05-18 18:34:09 UTC
Review of attachment 214376 [details] [review]:

My first thought is how it would interact with Nothing to Do:

    https://extensions.gnome.org/extension/153/nothingtodo/
    https://extensions.gnome.org/review/974

It looks fine, otherwise.
Comment 34 Jasper St. Pierre (not reading bugmail) 2012-05-18 18:35:43 UTC
Review of attachment 214377 [details] [review]:

::: js/ui/popupMenu.js
@@ +890,2 @@
     addSettingsAction: function(title, desktopFile) {
         // Don't allow user settings to get edited unless we're in a user session

Can you fix the comment?
Comment 35 Jasper St. Pierre (not reading bugmail) 2012-05-18 18:36:58 UTC
Review of attachment 214378 [details] [review]:

::: js/ui/sessionMode.js
@@ +36,3 @@
         this.showCalendarEvents = params.showCalendarEvents;
         this.allowSettings = params.allowSettings;
+        this.allowExtensions = params.allowExtensions;

OK, this is getting a bit out of hand. Lang.copyProperties?
Comment 36 Jasper St. Pierre (not reading bugmail) 2012-05-18 18:42:42 UTC
Review of attachment 214379 [details] [review]:

::: js/ui/main.js
@@ +565,3 @@
 
+    if (!sessionMode.allowKeybindingsWhenModal) {
+        if (modalCount > (sessionMode.hasOverview && overview.visible ? 1 : 0))

Won't overview.visible always be false if we don't have an overview?
Comment 37 Jasper St. Pierre (not reading bugmail) 2012-05-18 18:43:40 UTC
Review of attachment 214380 [details] [review]:

.
Comment 38 Jasper St. Pierre (not reading bugmail) 2012-05-18 18:46:34 UTC
Review of attachment 214381 [details] [review]:

::: js/ui/main.js
@@ +615,2 @@
             wm.actionMoveWorkspaceUp();
             return true;

I would prefer it if you put the check in windowManager.
Comment 39 Florian Müllner 2012-05-18 18:47:38 UTC
(In reply to comment #26)
> (Also, is there a reason you didn't move
> Main.createUserSession/Main.createGDMsession into here?)

Main.createUserSession() initializes globals in main, so I don't want to move it. I did consider to move Main.createGDMSession to imports.gdm.main.createGDMSession or something though.

(In reply to comment #28)
> +        this._status_area_order = order;
> 
> While we're here, can we fix up the bad variable name style here?

Sure, I'll do a separate patch though.


(In reply to comment #30)
> ::: src/main.c
> @@ +280,3 @@
> +   * We don't *really* need anything in GTK+, so just mute those - otherwise
> +   * we would still get warnings when executed from the CLI. */
> +  g_log_set_default_handler (shut_up, NULL);
> 
> Ew. No. Just initialize GTK+.

Sure, I can do both (I don't want the warnings GTK+ spills when gtk_init_check() fails on the console)


> @@ +290,3 @@
> Consider adding a prettyListModes() function to sessionMode that prints each on
> its own line.

Sure, can do ...


(In reply to comment #33)
> My first thought is how it would interact with Nothing to Do:

If an extension breaks, it breaks. Up to the author (or anyone for what's worth) to fix it. Note that in this particular case, there shouldn't be a problem - I expect sessionMode.allowExtensions to be false for anything other than the normal user session (which *does* show events normally).


(In reply to comment #36)
> Won't overview.visible always be false if we don't have an overview?

Good point. I didn't check, but I'd say it's a bug if it's not the case ...
Comment 40 Jasper St. Pierre (not reading bugmail) 2012-05-18 18:48:56 UTC
Review of attachment 214371 [details] [review]:

Looks fine.
Comment 41 Jasper St. Pierre (not reading bugmail) 2012-05-18 18:54:16 UTC
Review of attachment 214372 [details] [review]:

I'm a bit surprised at what seems to be a low number of places we're not using session-type.

::: js/ui/sessionMode.js
@@ +20,3 @@
+        let params = _modes[global.session_mode];
+
+        params = Params.parse(params, _modes[DEFAULT_MODE]);

Why are we using the params framework? It doesn't seem to help us. In fact, it seems to hurt us. Passing an invalid session mode will silently give us a user session:

    > imports.misc.params.parse(undefined, { a: 1 });
    r(0) = [object Object]
    > JSON.stringify(_);
    r(1) = { a: 1 }

We need to crash fast when passed an invalid session mode.
Comment 42 Jasper St. Pierre (not reading bugmail) 2012-05-18 18:54:57 UTC
Review of attachment 214373 [details] [review]:

Fine to squash with the previous patch.
Comment 43 Jasper St. Pierre (not reading bugmail) 2012-05-18 18:55:17 UTC
Review of attachment 214382 [details] [review]:

Neato.
Comment 44 Florian Müllner 2012-05-18 22:48:41 UTC
(In reply to comment #41)
> I'm a bit surprised at what seems to be a low number of places we're not using
> session-type.

Uhm - it is used in 4 out of 105 js files, so ~96% doesn't exactly feel like a "low number of places" to me ;-)


> ::: js/ui/sessionMode.js
> @@ +20,3 @@
> +        params = Params.parse(params, _modes[DEFAULT_MODE]);
> 
> Why are we using the params framework? It doesn't seem to help us.

It does allow for more concise definitions of "modes". It's not obvious now with only two modes (and all available options based on the differences between those), but I expect it to make a difference once we start adding more modes.

Once the default values are no longer relevant as fallback (see below), we can pick them based on commonality - still, I'd like to wait for more modes to actually pop up before making guesses at good defaults (though some stuff like "allowExtensions" is rather obvious).


> Passing an invalid session mode will silently give us a user
> session: [...]
> We need to crash fast when passed an invalid session mode.

I'm not that convinced that we *need* to crash. In fact, the only case I can think of where that seems desirable is a sysadmin rolling out a faulty kiosk configuration without any testing. In all other cases we'd crash on the developer (gdm, first-boot, ...) or sysadmin (kiosk, ...) doing tests, making it harder for them to fix the problem.

I was actually considering using a locked-down fallback, but changed my mind because it'll be most likely less obvious that something is wrong.

But anyway, while I don't agree with crashing, I'm fine with validating the mode early (read: before registering as the wm) and making a clean exit if it does not exist. I'll attach a separate patch for that.
Comment 45 Florian Müllner 2012-05-18 22:49:08 UTC
Created attachment 214403 [details] [review]
main: Add generic --mode switch

We are going to need "special" modes other than GDM, for instance
for the initial setup[0]. Rather than adding one command line switch
per mode (and logic to deal with multiple switches being given at
the same time), add a generic --mode switch to specify the mode as
string.

[0] https://live.gnome.org/GnomeOS/Design/Whiteboards/InitialSetup
Comment 46 Florian Müllner 2012-05-18 22:49:16 UTC
Created attachment 214404 [details] [review]
global: Add session-mode property

Add a session-mode property on ShellGlobal which corresponds to the
new --mode switch. Make the existing ShellGlobal:session-type property
readonly and base it on ShellGlobal:session-mode to avoid conflicts.
Comment 47 Florian Müllner 2012-05-18 22:50:19 UTC
Created attachment 214405 [details] [review]
Delegate mode information to a dedicated object

Unchanged, see comment #44 - next patch will introduce validation for the mode parameter.
Comment 48 Florian Müllner 2012-05-18 22:50:27 UTC
Created attachment 214406 [details] [review]
main: Make sure that --mode parameter is valid

Instead of falling back to a set of default values or crashing the
window manager when an invalid mode is specified, check the value
of the ShellGlobal:session-mode property before taking over as WM
and make a clean exit if it cannot be resolved to an existent mode.
Comment 49 Florian Müllner 2012-05-18 22:50:34 UTC
Created attachment 214407 [details] [review]
global: Remove session_type property

ShellGlobal:session-type is now unsed, remove it.
Comment 50 Florian Müllner 2012-05-18 22:53:35 UTC
Created attachment 214408 [details] [review]
sessionMode: Add hasOverview property

(In reply to comment #31)
> Review of attachment 214374 [details] [review]:
> 
> Is there a reason the overview can't handle this itself?

It could. On the other hand, we have handle all keybindings in main.js/windowManager.js, probably worth not spreading it further. Or not, dunno ...
Comment 51 Florian Müllner 2012-05-18 22:53:45 UTC
Created attachment 214409 [details] [review]
sessionMode: Add hasAppMenu property

Add a sessionMode.hasAppMenu property, which determines whether
the top bar should contain a menu for the active application or
not.
Comment 52 Florian Müllner 2012-05-18 22:53:53 UTC
Created attachment 214410 [details] [review]
sessionMode: Add showCalendarEvents property

Add a sessionMode.showCalendarEvents property, which determines
whether the calendar menu should contain an events section or not.
Comment 53 Florian Müllner 2012-05-18 22:56:07 UTC
Created attachment 214411 [details] [review]
sessionMode: Add allowSettings property

(In reply to comment #34)
>      addSettingsAction: function(title, desktopFile) {
>          // Don't allow user settings to get edited unless we're in a user
> session
> 
> Can you fix the comment?

Actually, "if (!allowSettings)" should be clear enough to not need a comment, so I just removed it.
Comment 54 Florian Müllner 2012-05-18 22:56:26 UTC
Created attachment 214412 [details] [review]
sessionMode: Add allowExtensions property

Add a sessionMode.allowExtensions property, which determines whether
installed and enabled extensions should be loaded or ignored.
Comment 55 Florian Müllner 2012-05-18 22:56:41 UTC
Created attachment 214413 [details] [review]
sessionMode: Add allowKeybindingsWhenModal property

Add a sessionMode.allowKeybindingsWhenModal property, which determines
whether keybindings should still be handled while a modal dialog is
up or not.
Comment 56 Florian Müllner 2012-05-18 22:56:48 UTC
Created attachment 214414 [details] [review]
sessionMode: Add hasRunDialog property

Add a sessionMode.hasRunDialog property, which determines whether
the keyboard shortcut to access the run dialog should be active
or not.
Comment 57 Florian Müllner 2012-05-18 23:03:53 UTC
Created attachment 214417 [details] [review]
sessionMode: Add hasWorkspaces property

(In reply to comment #38)
>              wm.actionMoveWorkspaceUp();
>              return true;
> 
> I would prefer it if you put the check in windowManager.

Mmh, there is a subtle difference though: right now we don't consume the event unless we handle the keybinding, while we would do so unconditionally if the check is moved into windowManager. I'm leaving it for now as it's consistent with the current code, but I don't feel strongly about it - I can't really think of a cse where this would cause actual problems, so I'll move it if you still want me to :)
Comment 58 Florian Müllner 2012-05-18 23:04:03 UTC
Created attachment 214418 [details] [review]
main: Remove _initUserSession()

In Shell.SessionType.USER mode, two separate setup functions were
used during startup. With the new feature-based checks, the second
one is now almost empty, so move its remaining code into the first
function and remove it.
Comment 59 Florian Müllner 2012-05-18 23:04:09 UTC
Created attachment 214419 [details] [review]
sessionMode: Add createSession() hook

Add a sessionMode.createSession() hook, which is executed during
startup to setup mode-specific stuff.
Comment 60 Florian Müllner 2012-05-18 23:04:20 UTC
Created attachment 214420 [details] [review]
sessionMode: Add extraStylesheet property

Add a sessionMode.extraStylesheet property, which may be used for
mode-specific style information.
Comment 61 Florian Müllner 2012-05-18 23:07:33 UTC
Created attachment 214421 [details] [review]
sessionMode: Add statusArea property

(In reply to comment #28)
> +        this._status_area_order = order;
> 
> While we're here, can we fix up the bad variable name style here?

sessionMode now uses a property for statusArea (rather than a function), so I just use that one directly instead of copying it into separate Panel properties.
Comment 62 Florian Müllner 2012-05-18 23:07:41 UTC
Created attachment 214422 [details] [review]
Remove SessionType type

SessionType has now completely been replaced by the finer-grained
session-mode, so remove the remaining type definition.
Comment 63 Florian Müllner 2012-05-18 23:07:47 UTC
Created attachment 214423 [details] [review]
main: Add a --list-modes command line option

Add a command line option to list available modes, i.e. possible
arguments for the --mode switch.
Comment 64 Jasper St. Pierre (not reading bugmail) 2012-05-18 23:19:36 UTC
Review of attachment 214421 [details] [review]:

::: js/ui/sessionMode.js
@@ +94,3 @@
+    createSession: function() {
+        if (this._createSession)
+            this._createSession();

Bad rebase.
Comment 65 Jasper St. Pierre (not reading bugmail) 2012-05-18 23:23:10 UTC
(In reply to comment #39)
> If an extension breaks, it breaks. Up to the author (or anyone for what's
> worth) to fix it. Note that in this particular case, there shouldn't be a
> problem - I expect sessionMode.allowExtensions to be false for anything other
> than the normal user session (which *does* show events normally).

I wasn't thinking about breaking things, I was thinking about providing an API for the extension to use. The extension is fairly simple as-is, though, but duplicated effort and all that. Not worth it, I guess.
Comment 66 Florian Müllner 2012-05-18 23:24:17 UTC
(In reply to comment #64)
> Review of attachment 214421 [details] [review]:
> 
> ::: js/ui/sessionMode.js
> @@ +94,3 @@
> +    createSession: function() {
> +        if (this._createSession)
> +            this._createSession();
> 
> Bad rebase.

Why?
Comment 67 Jasper St. Pierre (not reading bugmail) 2012-05-18 23:25:20 UTC
(In reply to comment #66)
> Why?

Uh, because it's in the statusArea patch?
Comment 68 Florian Müllner 2012-05-18 23:34:12 UTC
Created attachment 214424 [details] [review]
sessionMode: Add createSession() hook

(In reply to comment #67)
> (In reply to comment #66)
> > Why?
> 
> Uh, because it's in the statusArea patch?

Woops, gotcha ...  there was actually another problem introduced by the use of Lang.copyProperties that should be fixed here.
Comment 69 Florian Müllner 2012-05-18 23:35:11 UTC
Created attachment 214425 [details] [review]
sessionMode: Add statusArea property

Add a sessionMode.statusArea property, which specifies both the order
and shell implementation of the status area items for the mode.
Comment 70 Jasper St. Pierre (not reading bugmail) 2012-05-18 23:40:27 UTC
Review of attachment 214420 [details] [review]:

Much better.
Comment 71 Jasper St. Pierre (not reading bugmail) 2012-05-18 23:43:37 UTC
Review of attachment 214423 [details] [review]:

What's this mutter typelib dir stuff?

::: src/main.c
@@ +279,3 @@
+   * don't really care if it fails though (e.g. when running from a tty),
+   * so we mute all warnings */
+  g_log_set_default_handler (shut_up, NULL);

The reason I wanted GTK+ to be initialized was so that we wouldn't have to do this. I don't want all logging ever to be thrown out.

I don't really see the utility in --list-modes beyond debugging, so I think that emitting warnings when from a TTY is fine.
Comment 72 Jasper St. Pierre (not reading bugmail) 2012-05-18 23:45:17 UTC
Review of attachment 214425 [details] [review]:

Much cleaner.

::: js/ui/panel.js
@@ +1151,3 @@
 
+        this._insertStatusItem(box,
+                               Main.sessionMode.statusArea.order.indexOf(role));

Not sure what the purpose of the line wrap is. We're gaining what, five characters?
Comment 73 Jasper St. Pierre (not reading bugmail) 2012-05-18 23:46:08 UTC
Review of attachment 214424 [details] [review]:

Looks good.

::: js/ui/sessionMode.js
@@ +46,3 @@
         params = Params.parse(params, _modes[DEFAULT_MODE]);
+
+        this._createSession = params.createSession;

Might need a comment.
Comment 74 Jasper St. Pierre (not reading bugmail) 2012-05-18 23:48:04 UTC
Review of attachment 214417 [details] [review]:

Gotcha.
Comment 75 Jasper St. Pierre (not reading bugmail) 2012-05-18 23:48:16 UTC
Review of attachment 214413 [details] [review]:

Yep.
Comment 76 Jasper St. Pierre (not reading bugmail) 2012-05-18 23:51:19 UTC
Review of attachment 214411 [details] [review]:

::: js/ui/userMenu.js
@@ +626,2 @@
         item = new IMStatusChooserItem();
+        if (Main.sessionMode.allowSettings)

I'm not sure this is necessary. Will we ever be using the user menu component in a kiosk setting?
Comment 77 Jasper St. Pierre (not reading bugmail) 2012-05-18 23:51:59 UTC
Review of attachment 214405 [details] [review]:

Sure.
Comment 78 Jasper St. Pierre (not reading bugmail) 2012-05-18 23:56:13 UTC
Review of attachment 214406 [details] [review]:

Yuck. Can't we have Main.start return something instead?
Comment 79 Florian Müllner 2012-05-19 01:19:52 UTC
(In reply to comment #71)
> What's this mutter typelib dir stuff?

We obviously need it for imports.gi.Meta. In the normal code path the typelib path is added my meta_init(), but that function does way too much in case where we have no intention to becoming the window manager (in particular replacing an existing one).


> ::: src/main.c
> I don't want all logging ever to be thrown out.

I think doing it milliseconds before calling exit() is perfectly fine, but *shrug*


> I don't really see the utility in --list-modes beyond debugging

It is the last patch in the series for a reason :-)
Still, I think it is a bit impolite to expose a --foo=<something> switch without documenting the possible values of <something> anywhere, so I added it (it was certainly more fun than just editing the --mode help string). It's not required by anything, so I'm fine with leaving the patch out.


(In reply to comment #72)
> Not sure what the purpose of the line wrap is. We're gaining what, five
> characters?

Staying below 80 characters per line (a.k.a. "the one true terminal config"). I like lying to myself that I'm not religious about that limit, so I joined the lines again :-)


(In reply to comment #76)
> ::: js/ui/userMenu.js
> @@ +626,2 @@
>          item = new IMStatusChooserItem();
> +        if (Main.sessionMode.allowSettings)
> 
> I'm not sure this is necessary. Will we ever be using the user menu component
> in a kiosk setting?

Unlikely to say the least, but I still don't like to make that assumption given that the cost is a simple if() clause - "dont-show-settings-in-menus-unless-you-use-the-wrong-menu" just makes for a terrible option ...


(In reply to comment #78)
> Yuck. Can't we have Main.start return something instead?

That's almost as good as crashing. My main use case here is launching gnome-shell from an existing session while working on $mode. With the current patch, "gnome-shell --replace --mode=koisk" fails with an error message without affecting the running session, unlike any code after meta_run(). Also note that returning from Main.start() does not exit the shell, but leaves it in a running-but-broken state.
Comment 80 Jasper St. Pierre (not reading bugmail) 2012-05-21 23:11:17 UTC
Does this need anything else for me before it lands?
Comment 81 Florian Müllner 2012-05-22 17:48:52 UTC
Attachment 214403 [details] pushed as 5617f91 - main: Add generic --mode switch
Attachment 214404 [details] pushed as 940ddb1 - global: Add session-mode property
Attachment 214405 [details] pushed as 3d26224 - Delegate mode information to a dedicated object
Attachment 214406 [details] pushed as a7a46bb - main: Make sure that --mode parameter is valid
Attachment 214407 [details] pushed as 19318a1 - global: Remove session_type property
Attachment 214408 [details] pushed as 122bca4 - sessionMode: Add hasOverview property
Attachment 214409 [details] pushed as 6bee51e - sessionMode: Add hasAppMenu property
Attachment 214410 [details] pushed as ba92cfa - sessionMode: Add showCalendarEvents property
Attachment 214411 [details] pushed as a3fcb8c - sessionMode: Add allowSettings property
Attachment 214412 [details] pushed as ab31734 - sessionMode: Add allowExtensions property
Attachment 214413 [details] pushed as de69c71 - sessionMode: Add allowKeybindingsWhenModal property
Attachment 214414 [details] pushed as 5264f39 - sessionMode: Add hasRunDialog property
Attachment 214417 [details] pushed as ed17418 - sessionMode: Add hasWorkspaces property
Attachment 214418 [details] pushed as f6a2c92 - main: Remove _initUserSession()
Attachment 214420 [details] pushed as b5b1332 - sessionMode: Add extraStylesheet property
Attachment 214422 [details] pushed as e49b946 - Remove SessionType type
Attachment 214423 [details] pushed as ecff2fa - main: Add a --list-modes command line option
Attachment 214424 [details] pushed as c25e7f3 - sessionMode: Add createSession() hook
Attachment 214425 [details] pushed as e4f1572 - sessionMode: Add statusArea property


(In reply to comment #80)
> Does this need anything else for me before it lands?

I didn't address everything that came up in review (as noted in the earlier comments), but as this is still early in the cycle there's plenty of time to adjust as necessary, so I've pushed the series now.

Thanks for the quick review!