Created attachment 238366 [details] [review]
Patch for gsd-keyboard-manager.c

The patch can launch the second input method too.

Review of attachment 238366 [details] [review]:

Thanks, this looks useful but I think it needs some further work.

::: plugins/keyboard/gsd-keyboard-manager.c
@@ +216,3 @@
 }
 
+#ifdef HAVE_IBUS

This section of code is already inside a #ifdef HAVE_IBUS

@@ +218,3 @@
+#ifdef HAVE_IBUS
+static void
+fetch_ibus_preload_engines_result (GObject            *object,

There's nothing useful here so we can just pass NULL for the async callback below and then this isn't needed.

@@ +262,3 @@
+                                     n_sources - 1);
+                goto exit;
+        }

None of this initial part is needed as it's already checked by apply_input_sources_settings().

@@ +267,3 @@
+                if (next == current) {
+                        continue;
+                }

The rest of the code avoids braces for single line if statements.

@@ +293,3 @@
+        g_free (names[0]);
+        g_free (names[1]);
+        g_free (names);

Use g_strfreev()

@@ +349,2 @@
         apply_input_sources_settings (priv->input_sources_settings, NULL, 0, manager);
+#ifdef HAVE_IBUS

Not needed here.

@@ +349,3 @@
         apply_input_sources_settings (priv->input_sources_settings, NULL, 0, manager);
+#ifdef HAVE_IBUS
+        preload_ibus_engine (manager);

So, here's a more general comment on this. I think we should use g_timeout_add_seconds(), say with 30 seconds or something, so that we don't create needless I/O during login and slow everything down even further.

Created attachment 238553 [details] [review]
Patch for gsd-keyboard-manager.c

OK, revised the patch.

ibus 1.5.2 is released.

Could you review the patch?

Created attachment 255603 [details] [review]
Patch for gsd-keyboard-manager.c

Updated the patch for the latest HEAD.

The code is moved to gnome-shell.

Created attachment 289711 [details] [review]
Patch for ibusManager.js

Revised the patch.

Review of attachment 289711 [details] [review]:

This doesn't preload just second but all configured engines right? Please fix the commit subject and message accordingly.

::: js/misc/ibusManager.js
@@ +178,3 @@
     },
+
+    preloadEngines: function(ids, callback) {

The callback isn't being used right? We can add it later if we find that we need it.

@@ +179,3 @@
+
+    preloadEngines: function(ids, callback) {
+        if (!this._ibus.preload_engines_async)

All public methods in this class need to check that we have the IBus import loaded and that we have successfully connected to the daemon so

if (!IBus || !this._ready)
    return;

@@ +183,3 @@
+
+        this._ibus.preload_engines_async(ids, this._MAX_INPUT_SOURCE_ACTIVATION_TIME,
+                                         null, callback);

If we don't need the callback we don't need the dbus timeout either.

::: js/ui/status/keyboard.js
@@ +390,3 @@
+                break;
+            }
+        }

this._ibusManager.preloadEngines(Object.keys(this._ibusSources));

(In reply to comment #2)
> So, here's a more general comment on this. I think we should use
> g_timeout_add_seconds(), say with 30 seconds or something, so that we don't
> create needless I/O during login and slow everything down even further.

Oh, and this still applies.

Created attachment 290080 [details] [review]
Patch for ibusManager.js

Updated the patch.
I added a version check function because I think IBus.Bus.preload_engines_async is not accessible in GJS without the instance.

Created attachment 290081 [details] [review]
Patch for ibusManager.js

Fixed a typo.

Created attachment 290132 [details] [review]
Patch for ibusManager.js

s/_MAX_PRELOAD_DELAY_TIME/_PRELOAD_ENGINES_DELAY_TIME/

"keyboard" is the component for the on-screen keyboard. Reassigning to "general" until it's triaged.

Review of attachment 290132 [details] [review]:

Almost there.

s/Execute/Preload/ and s/run/preload/ would be more accurate. Also, no period at the end of the subject

::: js/misc/ibusManager.js
@@ +10,3 @@
     var IBus = imports.gi.IBus;
+    if (checkVersion(1, 5, 2) < 0)
+        throw "IBus requires 1.5.2 or later";

This is a "private" function so prefix it with underscore, i.e. _checkIBusVersion()

Also, passing the minimum version like this doesn't seem very useful. I'd just write this as:

_checkIBusVersion();

then see below

@@ +21,3 @@
+function checkVersion(major, minor, micro) {
+    if (IBus.MAJOR_VERSION == major && IBus.MINOR_VERSION == minor &&
+        IBus.MICRO_VERSION == micro)

This check is pointless, the one below is enough.

@@ +27,3 @@
+        (IBus.MAJOR_VERSION == major && IBus.MINOR_VERSION > minor) ||
+        (IBus.MAJOR_VERSION == major && IBus.MINOR_VERSION == minor &&
+         IBus.MICRO_VERSION > micro))

Removing the check above means this has to be MICRO_VERSION >= micro

@@ +30,3 @@
+        return 1;
+
+    return -1;

I'd throw right here. Also, please put the minimum required version in variables and then use that to format the error message like

throw "Found IBus version %d.%d.%d but required is %d.%d.%d".format(IBus.MAJOR_VERSION, ..., ..., requiredMajor, ... , ...);

@@ +196,3 @@
     },
+
+    preloadEngines: function(ids) {

This will be called unconditionally from outside so we need to check

if (!IBus || !this._ibus) return;

@@ +203,3 @@
+
+        this._preloadEnginesId =
+            Mainloop.timeout_add(this._PRELOAD_ENGINES_DELAY_TIME,

Since this is a relatively long time that doesn't need millisecond precision, it should use timeout_add_seconds()

::: js/ui/status/keyboard.js
@@ +386,3 @@
+        // when users switch the input sources.
+        let ibusSources = Object.keys(this._ibusSources);
+        if (ibusSources.length > 0)

The IBus code seems to handle empty lists just fine so this check seems pointless. No big deal though

Created attachment 290324 [details] [review]
Patch for ibusManager.js

Revised the patch.

Created attachment 290326 [details] [review]
Patch for ibusManager.js

s/.$// in the commit subject.

Review of attachment 290326 [details] [review]:

Looks fine, thanks for the patch.

In fact, it seems I was wrong about ibus handling empty lists fine so I'm going to amend the patch before pushing. It seems the server side ibus code does handle it but the client library has this check https://github.com/ibus/ibus/blob/master/src/ibusbus.c#L2092 .