Merge lp:~ntnk/gala/modifiers-only-input-source-switch into lp:gala

[Kirill Antonik]

This branch activates input source switch using "grp:" accelerator from xkb-options. It allows to use accelerator + key combos (Ctrl+Shift+T in Terminal) while input source can be switched with a modifiers only accelerator (Ctrl+Shift). It's the same thing Gnome Shell does actually. Works for me when switching between Russian and English layouts, may need more testing. Plus I'm not sure what place this code belongs to, so I just made a new file.

[Rico Tzschichholz (ricotz)]

Take a look at handle_switch_input_source() in WindowManager.vala

[Kirill Antonik (ntnk)]

How can handle_switch_input_source() be connected to the signal, if it has different number of parameters? Tried Signal.connect(), but it generates different C code, that doesn't work as intended. Sorry if I'm missing something, still new to Vala.

[Santiago (santileortiz)]

I think it will be better to just replace handle_switch_input_source() like this http://paste.debian.net/739520/, if we leave both then we will have conflicts when editing the 'current' key.

This will remove the possibility of configuring this keybinding through the Shortcuts tab on the keyboard plug, but I think no one really cares for non modifier-only keybindings for layout switching, except for our default Alt+Space which is still available through grp:alt_space_toggle.

[lp:~ntnk/gala/modifiers-only-input-source-switch]

527. By Kirill Antonik on 2016-06-15

Move the code to WindowManager.vala

528. By Kirill Antonik on 2016-06-15

Remove keybindings to switch input-sources

[Kirill Antonik (ntnk)]

Moved the code and removed switch-input-source keybindings.

[Rico Tzschichholz (ricotz)]

If this is meant to be the correct handling then it should be moved into KeyboardManager.vala
Take a close look at the documentation of modifiers_accelerator_activated signal and its boolean return value.

Also do not removed gschema-keys! If they are obsolete and not used anymore then mark them deprecated instead.

[lp:~ntnk/gala/modifiers-only-input-source-switch]

529. By Kirill Antonik on 2016-06-15

Move the code to KeyboardManager.vala and mark gschema-keys deprecated

[Santiago (santileortiz)]

Looking at the documentation it does seem like handle_switch_input_source() should return false, even though I don't see any difference in the behavior.

[Kirill Antonik (ntnk)]

Returning false calls unfreeze_keyboard() in mutter, while returning true just does nothing, making input unfreezing gala's problem, that's what ungrab_keyboard() is for. Following the documentation, ungrab_keyboard() should be called after handle_switch_input_source() returns true, but it seems to work great even if it's called before returning. So I'm not sure if we need ungrab_keyboard() and unfreeze_keyboard() (return false) or ungrab_keyboard() (return true) is enough.

[Santiago (santileortiz)]

Well I'm really not sure about this, by reading Mutter's source code on src/core/keybindings.c I see returning false will call XIAllowEvents() (through unfreeze_keyboard()) for the keyboard, while ungrab_keyboard() calls XIUngrabDevice() for the keyboard, both of them seem to work, but I really don't understand Mutter's code or the X11 protocol well enough to know what the state of the X server is at this point so I wouldn't know what is best. If someone knows then maybe that will tell us what is best.

Probably the safest choice is to return false (without calling ungrab_keyboard()) and let Mutter handle the cleanup after emitting the signal. After all, we are not doing anything besides knowing a modifier-only accelerator was activated.

[Kirill Antonik (ntnk)]

Returning false without calling ungrab_keyboard() breaks keybindings like Ctrl+Shift+T. I actually found that Gnome Shell calls ungrab_keyboard() before returning true, so the current code in this branch is fine.

[Rico Tzschichholz (ricotz)]

After a first review
https://paste.debian.net/plain/740271

review: Needs Fixing

[lp:~ntnk/gala/modifiers-only-input-source-switch]

530. By Kirill Antonik on 2016-06-20

Updated with review changes

[Kirill Antonik (ntnk)]

Thanks for suggestions, updated, works as expected.

[Rico Tzschichholz (ricotz)]

Some minor things
https://paste.debian.net/plain/740304

@santileortiz: Could you test the branch too again?

review: Needs Fixing

[Maxim Taranov (png2378)]

It works for me with latest revisions.

[Rico Tzschichholz (ricotz)]

review: Approve

=== modified file 'data/org.pantheon.desktop.gala.gschema.xml.in.in'
--- data/org.pantheon.desktop.gala.gschema.xml.in.in	2016-02-14 18:07:18 +0000
+++ data/org.pantheon.desktop.gala.gschema.xml.in.in	2016-06-20 14:14:24 +0000
@@ -134,10 +134,12 @@
 		<key type="as" name="switch-input-source">
 			<default><![CDATA[['<Alt>space']]]></default>
 			<_summary>Cycle to next keyboard layout</_summary>
+			<description>DEPRECATED: This key is deprecated and ignored.</description>
 		</key>
 		<key type="as" name="switch-input-source-backward">
 			<default><![CDATA[['']]]></default>
 			<_summary>Cycle to previous keyboard layout</_summary>
+			<description>DEPRECATED: This key is deprecated and ignored.</description>
 		</key>
 	</schema>
 	

=== modified file 'src/KeyboardManager.vala'
--- src/KeyboardManager.vala	2016-06-16 06:53:18 +0000
+++ src/KeyboardManager.vala	2016-06-20 14:14:24 +0000
@@ -22,11 +22,14 @@
 		static KeyboardManager? instance;
 		static VariantType sources_variant_type;
 
-		public static void init ()
+		public static void init (Meta.Display display)
 		{
-			if (instance == null) {
-				instance = new KeyboardManager ();
-			}
+			if (instance != null)
+				return;
+
+			instance = new KeyboardManager ();
+
+			display.modifiers_accelerator_activated.connect (instance.handle_modifiers_accelerator_activated);
 		}
 
 		static construct
@@ -53,6 +56,24 @@
 		}
 
 		[CCode (instance_pos = -1)]
+		bool handle_modifiers_accelerator_activated (Meta.Display display)
+		{
+			display.ungrab_keyboard (display.get_current_time ());
+
+			var sources = settings.get_value ("sources");
+			return_val_if_fail (sources.is_of_type (sources_variant_type), true);
+
+			var n_sources = (uint) sources.n_children ();
+			if (n_sources < 2)
+				return true;
+
+			var current = settings.get_uint ("current");
+			settings.set_uint ("current", (current + 1) % n_sources);
+
+			return true;
+		}
+
+		[CCode (instance_pos = -1)]
 		void set_keyboard_layout (GLib.Settings settings, string key)
 		{
 			if (!(key == "current" || key == "source" || key == "xkb-options"))

=== modified file 'src/WindowManager.vala'
--- src/WindowManager.vala	2016-05-24 13:00:35 +0000
+++ src/WindowManager.vala	2016-06-20 14:14:24 +0000
@@ -141,7 +141,7 @@
 			DBusAccelerator.init (this);
 #endif
 			WindowListener.init (screen);
-			KeyboardManager.init ();
+			KeyboardManager.init (display);
 
 			// Due to a bug which enables access to the stage when using multiple monitors
 			// in the screensaver, we have to listen for changes and make sure the input area
@@ -203,8 +203,6 @@
 			display.add_keybinding ("move-to-workspace-last", keybinding_schema, 0, (Meta.KeyHandlerFunc) handle_move_to_workspace_end);
 			display.add_keybinding ("cycle-workspaces-next", keybinding_schema, 0, (Meta.KeyHandlerFunc) handle_cycle_workspaces);
 			display.add_keybinding ("cycle-workspaces-previous", keybinding_schema, 0, (Meta.KeyHandlerFunc) handle_cycle_workspaces);
-			display.add_keybinding ("switch-input-source", keybinding_schema, 0, (Meta.KeyHandlerFunc) handle_switch_input_source);
-			display.add_keybinding ("switch-input-source-backward", keybinding_schema, 0, (Meta.KeyHandlerFunc) handle_switch_input_source);
 
 			display.overlay_key.connect (() => {
 				try {
@@ -364,27 +362,6 @@
 		}
 
 		[CCode (instance_pos = -1)]
-		void handle_switch_input_source (Meta.Display display, Meta.Screen screen, Meta.Window? window,
-			Clutter.KeyEvent event, Meta.KeyBinding binding)
-		{
-			var keyboard_input_settings = new GLib.Settings ("org.gnome.desktop.input-sources");
-
-			var n_sources = (uint) keyboard_input_settings.get_value ("sources").n_children ();
-			if (n_sources < 2)
-				return;
-
-			var new_index = 0U;
-			var current_index = keyboard_input_settings.get_uint ("current");
-
-			if (binding.get_name () == "switch-input-source")
-				new_index = (current_index + 1) % n_sources;
-			else
-				new_index = (current_index - 1 + n_sources) % n_sources;
-
-			keyboard_input_settings.set_uint ("current", new_index);
-		}
-
-		[CCode (instance_pos = -1)]
 		void handle_cycle_workspaces (Meta.Display display, Meta.Screen screen, Meta.Window? window,
 			Clutter.KeyEvent event, Meta.KeyBinding binding)
 		{