Created attachment 233067 [details] [review]
status/keyboard: Add input source switching per window

If the setting is enabled, we record the last activated input source
for the currently focused window and switch to it when focusing back
that window.

Created attachment 233068 [details] [review]
status/keyboard: Only change the current source setting if it changed

This avoids all the work that goes on in various processes when
switching input sources if the activated source if the currently
configured one.

Review of attachment 233068 [details] [review]:

Huh, I thought gsettings did this check for us.

(In reply to comment #3)
> Review of attachment 233068 [details] [review]:
> 
> Huh, I thought gsettings did this check for us.

Me too, but it doesn't.

Review of attachment 233067 [details] [review]:

I'm afraid of this because it's state that we lose if we crash the Shell. Should we set an Xorg property on the window so we can get it back? _GNOME_PRIV_INPUT_SOURCE ?

::: js/ui/status/keyboard.js
@@ +743,3 @@
+    _sourcesPerWindowChanged: function() {
+        this._sourcesPerWindow = this._settings.get_boolean('per-window');
+        if (this._sourcesPerWindow && this._focusWindowNotifyId == 0) {

I wonder if we should lose the input sources stored on each window?

Created attachment 233094 [details] [review]
status/keyboard: Only change the current source setting if it changed

--

Not asking the GSettings object looks tidier though.

Created attachment 233311 [details] [review]
status/keyboard: Add input source switching per window

--
* Added the Overview as another "window".

(In reply to comment #5)
> Review of attachment 233067 [details] [review]:
>
> I'm afraid of this because it's state that we lose if we crash the Shell.
> Should we set an Xorg property on the window so we can get it back?
> _GNOME_PRIV_INPUT_SOURCE ?

I think that's too much trouble for something that's basically
transient state which the user can easily get back to.

> ::: js/ui/status/keyboard.js
> @@ +743,3 @@
> +    _sourcesPerWindowChanged: function() {
> +        this._sourcesPerWindow = this._settings.get_boolean('per-window');
> +        if (this._sourcesPerWindow && this._focusWindowNotifyId == 0) {
>
> I wonder if we should lose the input sources stored on each window?

Yeah, I think it's better to do that indeed. Done here now.

Created attachment 233312 [details] [review]
display: Expose meta_display_list_windows

This is useful to quickly get at all the MetaWindow instances.
--

The shell patch depends on this. Did I miss a way to get at all the
windows already existing somewhere else?

global.get_window_actors().map(function(x) {
    return x.meta_window;
});

or similar is what we do already, which isn't really good. If you want to add MetaDisplay.list_windows(), I'm fine with it. At some point we should probably kill a lot of the code that uses get_window_actors to get a MetaWindow.

Review of attachment 233312 [details] [review]:

OK. Bonus points if you quickly swap out the quickly greppable instances of get_window_actors() in the shell codebase.

Review of attachment 233094 [details] [review]:

When could 'activate' get sent when it's already active?

(In reply to comment #11)
> Review of attachment 233094 [details] [review]:
> 
> When could 'activate' get sent when it's already active?

Clicking the corresponding panel menu entry or using the new AltTab-like switcher and choosing the same entry.

Review of attachment 233311 [details] [review]:

::: js/ui/status/keyboard.js
@@ +722,3 @@
+    },
+
+    _setPerWindowInputSource: function() {

Can we share some of this with _changePerWindowSource?

@@ +735,3 @@
+            window._inputSources = this._inputSources;
+            window._currentSource = this._currentSource;
+            return;

I'd prefer this to be:

    if (window._inputSources == null) {
        // ...
    } else if (window._inputSources == this._inputSources) {
        // ...
    } else {
        // ...
    }

Makes the behavior much more obvious.

@@ +782,3 @@
+        let window;
+        if (Main.overview.visible)
+            window = Main.overview;

Should we do it for the message tray, too? Modal dialogs?

    let focusable;
    if (global.display.focus_window != null)
        focusable = global.display.focus_window;
    else
        focusable = global.stage.key_focus;

The hard thing is to drop sources when the setting changes. One easy idea is to add a counter that increments whenever the setting changes, so if we detect that the focusable's counter is out of date, we don't respect the value.

(In reply to comment #13)
> @@ +782,3 @@
> +        let window;
> +        if (Main.overview.visible)
> +            window = Main.overview;
> 
> Should we do it for the message tray, too? Modal dialogs?

We probably should but I haven't come up with a solution for that yet. See below.

>     let focusable;
>     if (global.display.focus_window != null)
>         focusable = global.display.focus_window;
>     else
>         focusable = global.stage.key_focus;

This doesn't really work for 2 reasons. global.display.focus_window is not null when the input focus is on the COW and global.stage.key_focus is actually either the panel menu entry that the user is activating or the AltTab-like switcher item. Do you have suggestions to overcome this?

Comment on attachment 233312 [details] [review]
display: Expose meta_display_list_windows

Other places that are using global.get_window_actors() are relying on the stack ordering on that list so they can't be just ported to meta_display_list_windows() so I'm just going to use global.get_window_actors() here too instead of introducing another method needlessly.

Created attachment 233319 [details] [review]
status/keyboard: Add input source switching per window

--
(In reply to comment #13)
> +    _setPerWindowInputSource: function() {
>
> Can we share some of this with _changePerWindowSource?

Like this?

> I'd prefer this to be:
>
>     if (window._inputSources == null) {
>         // ...
>     } else if (window._inputSources == this._inputSources) {
>         // ...
>     } else {
>         // ...
>     }
>
> Makes the behavior much more obvious.

Ok.

Review of attachment 233319 [details] [review]:

OK.

Review of attachment 233094 [details] [review]:

OK.

Attachment 233094 [details] pushed as ca44977 - status/keyboard: Only change the current source setting if it changed
Attachment 233319 [details] pushed as 16bb9c1 - status/keyboard: Add input source switching per window