Created attachment 230226 [details] [review]
sessionMode: Add support for external mode definitions

Currently adding a new session mode requires patching the sources.
As defining custom modes can be desirable in some circumstances
(for instance for administrators of kiosk setups), load additional
modes from JSON files.

Created attachment 230227 [details] [review]
overview: Handle sessionMode.hasOverview changes

Currently we assume that either the initial sessionMode will have
the overview or none of the pushed modes - starting without the
overview and pushing a mode that adds it fails spectacularly.
However this is exactly what we are going to do when loading external
modes asynchronously - we'll initially use the default mode while
the modes are loading, and switch to the mode passed on the command
line when finished. So make sure that the overview UI gets initialized
properly in that case.

Created attachment 230228 [details] [review]
main: Stop setting custom keybinding handlers conditionally

Currently we call wm.setCustomKeybindingHandler() on session mode
changes for the panel-menu and run-dialog keybindings, passing
either the proper callback or null depending on the corresponding
session mode properties.
However as meta_keybindings_set_custom_handler() only allows to
replace existing handlers, setting it to NULL will disable the
binding permanently. To fix, set the custom handler unconditionally
and move the check for the session mode property into the handler.

Created attachment 230229 [details] [review]
sessionMode: Make loading of external modes asynchronous

Blocking IO on startup is bad m'kay ...

Review of attachment 230226 [details] [review]:

I thought about doing this, but there's things (like UnlockDialog or createSession) which aren't JSON values.

::: js/ui/sessionMode.js
@@ +159,3 @@
+                let evalProps = ['unlockDialog'];
+                if (evalProps.indexOf(prop) > -1)
+                    modes[modeName][prop] = eval(newMode[prop]);

No. I'd prefer a dispatch table of unlock dialogs, or not allowing to specify this in the mode.

Review of attachment 230227 [details] [review]:

OK.

Review of attachment 230228 [details] [review]:

I assume "disable the binding permanently" means "disable the binding until the next time set_custom_handler is called", in which case, wouldn't the session updated do the right thing currently? If that's not the case, that's a mutter bug.

Review of attachment 230229 [details] [review]:

::: js/ui/sessionMode.js
@@ +111,3 @@
+    let loadState = { loadedModes: _modes,
+                      numLoading: 0,
+                      modesLoadedCallback: modesLoadedCallback };

Hello, another case for Deferred/gatherResults.

@@ +154,1 @@
+            loadState.loadedModes[modeName] = {};

Just want to make sure you're aware of the race condition here where the values can be random if we load two files with the same mode name from two separate dirs.

Created attachment 230232 [details] [review]
sessionMode: Add support for external mode definitions

Minor cleanup

Created attachment 230233 [details] [review]
sessionMode: Make loading of external modes asynchronous

Minor fix.

(In reply to comment #7)
> Review of attachment 230228 [details] [review]:
> 
> I assume "disable the binding permanently" means "disable the binding until the
> next time set_custom_handler is called"

No. As explained in the commit message, meta_keybindings_set_custom_handler() only allows replacing *existing* handlers. Which translates to a check for NULL in C, so setting the handler to NULL removes it completely.

(In reply to comment #11)
> No. As explained in the commit message, meta_keybindings_set_custom_handler()
> only allows replacing *existing* handlers. Which translates to a check for NULL
> in C, so setting the handler to NULL removes it completely.

Yeah, I think this is a case of a bad API that we should fix in mutter. I can imagine this tripping up people with extensions, too.

Comment on attachment 230228 [details] [review]
main: Stop setting custom keybinding handlers conditionally

OK, found the real bug, pushed as 5ecc204d570140 ...

Comment on attachment 230227 [details] [review]
overview: Handle sessionMode.hasOverview changes

Attachment 230227 [details] pushed as e00eb06 - overview: Handle sessionMode.hasOverview changes

Created attachment 230237 [details] [review]
sessionMode: Add support for external mode definitions

(In reply to comment #5)
> No. I'd prefer [...] not allowing to specify this in the mode.

Yeah, I had considered that before, but that was before I wrote the parentMode patch, so I needed some way to support that property for a mode similar to the normal user one.
Now that we can just inherit from the user mode, let's go with the simple backlist approach ...

Review of attachment 230237 [details] [review]:

Sure.

Created attachment 230325 [details] [review]
remoteSearch: Factor out collectFromDatadirsAsync() utility function

Processing files from a subdirectory of various share/gnome-shell
directories asynchronously is a common enough pattern to share
the common code.

Created attachment 230326 [details] [review]
extensionUtils: Load extensions asynchronously

We shouldn't use blocking I/O on session start, so make extension
loading asynchronous using the new collectFromDatadirsAsync()
function.

Created attachment 230327 [details] [review]
sessionMode: Make loading of external modes asynchronous

Blocking IO on startup is bad m'kay ...

Review of attachment 230325 [details] [review]:

::: js/misc/fileUtils.js
@@ +49,3 @@
+}
+
+function collectFromDatadirsAsync(subdir, processFile, params) {

Seems weird to pass one callback as a parameter, and the other in params. Perhaps we should just use params entirely?

::: js/ui/remoteSearch.js
@@ +36,3 @@
+        loadRemoteSearchProvider,
+        { loadedCallback: remoteProvidersLoaded,
+          data: { loadedProviders: [],

let data = { loadedProviders: [],

...

{ loadedCallback: remoteProvidersLoaded,
  data: data }

@@ -45,3 @@
-        dir.query_info_async('standard:type', Gio.FileQueryInfoFlags.NONE,
-            GLib.PRIORITY_DEFAULT, null,
-                function(object, res) {

Indentation is a bit wacky. Wrap the function on the last line.

Review of attachment 230326 [details] [review]:

::: js/misc/extensionUtils.js
@@ +180,3 @@
+                                           Lang.bind(this, this._loadExtension),
+                                           { includeUserdir: true,
+                                             data: extensions });

data isn't used. Remove.

Review of attachment 230325 [details] [review]:

::: js/misc/fileUtils.js
@@ +29,3 @@
+    dir.query_info_async('standard:type', Gio.FileQueryInfoFlags.NONE,
+        GLib.PRIORITY_DEFAULT, null,
+            function(object, res) {

And I meant to put the indentation comment here, even though it's existing.

@@ +32,3 @@
+                try {
+                    object.query_info_finish(res);
+                } catch (e) {

Oh, we should probably test that the error is actually something we care about rather than drop all errors on the floor.

(In reply to comment #20)
> Seems weird to pass one callback as a parameter, and the other in params.
> Perhaps we should just use params entirely?

The idea is to use params for optional parameters - not running a callback after the entire operation has finished seems perfectly valid, iterating over all files in a couple of directories to not do anything does not.

(In reply to comment #22)
> Oh, we should probably test that the error is actually something we care about
> rather than drop all errors on the floor.

I don't think aborting on any error is "dropping all errors on the floor". Any particular errors you suggest to ignore?

(In reply to comment #24)
> I don't think aborting on any error is "dropping all errors on the floor". Any
> particular errors you suggest to ignore?

Logging about Permissions Denied or other errors may help administrators investigate why their modes aren't being used.

(In reply to comment #23)
> The idea is to use params for optional parameters - not running a callback
> after the entire operation has finished seems perfectly valid, iterating over
> all files in a couple of directories to not do anything does not.

We use Params for required params elsewhere, so I don't see why this should be anything different.

(In reply to comment #25)
> We use Params for required params elsewhere, so I don't see why this should be
> anything different.

I'd consider that a bug if we don't (or can't) provide a meaningful default value at the same time.

Created attachment 230346 [details] [review]
remoteSearch: Factor out collectFromDatadirsAsync() utility function

I still think processFile doesn't make sense as optional parameter, but whatever ...

Created attachment 230347 [details] [review]
extensionUtils: Load extensions asynchronously

Created attachment 230348 [details] [review]
sessionMode: Make loading of external modes asynchronous

Blocking IO on startup is bad m'kay ...

Can the GDM mode be implemented in the same way?

(In reply to comment #30)
> Can the GDM mode be implemented in the same way?

It'd require a bit of code separation, but generally it shouldn't be too hard to use an external mode + extension and move it to the gdm module. I don't mind having support for a part of the core experience like the login screen in gnome-shell core, but maybe you are right and the move makes sense anyway ...

Extension? No.

I'd say that if anything, we should have a separate binary for "gdm" with a shared "base compositor" library, which contains most of the js/ui/ stuff. This is why I've been wanting to reorg the js/ directories so bad.

(In reply to comment #32)
> I'd say that if anything, we should have a separate binary for "gdm" with a
> shared "base compositor" library, which contains most of the js/ui/ stuff.

But gdm and normal shell differ only in js - so how does that make any sense?

(In reply to comment #33)
> But gdm and normal shell differ only in js - so how does that make any sense?

Stuff like ShellEntry and ModalDialog should be shared, but AppDisplay is clearly shell-only. I'm imagining that those shared components go into a "Widgets" library that can be used by gdm.

Review of attachment 230346 [details] [review]:

::: js/misc/fileUtils.js
@@ +76,3 @@
+    };
+
+    if (params.includeUserdir) {

This seems like it could introduce capitalization typos: 'includeUserDir'. 'includeUser' or 'includeHome' might be better.

There's also the question of whether the scan for homedir should go before or after system dirs. I don't think we need this flexibility quite yet.

@@ +78,3 @@
+    if (params.includeUserdir) {
+        let path = GLib.build_filenamev([global.userdatadir, subdir]);
+        let dir = Gio.file_new_for_path(path);

Gio.File.new_for_path; functions on interfaces are deprecated and may be removed in the future.

@@ +81,3 @@
+
+        loadState.loadedCallback = collectFromSystemDirs;
+        _collectFromDirectoryAsync(dir, loadState);

eek. I'd prefer a "collectFromDirs", and we do:

    let dirs = GLib.get_system_data_dirs();
    if (params.includeWhatever) {
        dirs.unshift(dir);
    }

    collectFromDirsAsync(dirs);

That would remove the ugly munging of the loadState callback, too, and we can just set it directly.

Review of attachment 230347 [details] [review]:

This won't help that much, considering that loading the extension script itself will still use blocking IO, and that the extensions will execute synchronously as well.

It's a nice commit, but with an extremely misleading commit message. I don't think we can have real asynchronous extension loading until we get better JS APIs, which will only happen when (if???) js187 comes out.

::: js/misc/extensionUtils.js
@@ +164,3 @@
 
+        let extension;
+        let perUserDir = Gio.File.new_for_path(global.userdatadir);

I wonder if we can save this file on the state to prevent recreating it. Oh well, microoptimization.

(In reply to comment #35)
> eek. I'd prefer a "collectFromDirs", and we do:
> 
>     let dirs = GLib.get_system_data_dirs();
>     if (params.includeWhatever) {
>         dirs.unshift(dir);
>     }
> 
>     collectFromDirsAsync(dirs);
> 
> That would remove the ugly munging of the loadState callback, too, and we can
> just set it directly.

That's more or less what I had first, but I rewrote it because I assumed that the extension system guarantees that extensions in home take precedence over extensions in system directories. If you are OK with possible randomness there, I'm fine with changing the code back.

Review of attachment 230348 [details] [review]:

Fine by me.

(In reply to comment #34)
> Stuff like ShellEntry and ModalDialog should be shared, but AppDisplay is
> clearly shell-only. I'm imagining that those shared components go into a
> "Widgets" library that can be used by gdm.

I'm just not sure any advantage of doing that outweighs the necessary duplication (at least large parts of main.c and main.js) and justifies moving the shell towards the bag-of-bits-build-your-own-on-top platform.

Any comments on comment #37?

(In reply to comment #37)
> That's more or less what I had first, but I rewrote it because I assumed that
> the extension system guarantees that extensions in home take precedence over
> extensions in system directories. If you are OK with possible randomness there,
> I'm fine with changing the code back.

I'm fine with it. It's not ideal, but when we fix it we should fix it for all users, not just home vs. system.

Attachment 230237 [details] pushed as 8a17f51 - sessionMode: Add support for external mode definitions
Attachment 230346 [details] pushed as c528401 - remoteSearch: Factor out collectFromDatadirsAsync() utility function
Attachment 230347 [details] pushed as 6b40c39 - extensionUtils: Load extensions asynchronously
Attachment 230348 [details] pushed as 92083ea - sessionMode: Make loading of external modes asynchronous

(In reply to comment #36)
> It's a nice commit, but with an extremely misleading commit message.

OK, changed to something less promising.


> ::: js/misc/extensionUtils.js
> I wonder if we can save this file on the state to prevent recreating it. Oh
> well, microoptimization.

Yeah, did that as well


(In reply to comment #41)
> I'm fine with it. It's not ideal, but when we fix it we should fix it for all
> users, not just home vs. system.

OK, I'll add this to the nice-to-fix-later list then :-)