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

Proposed by Kirill Antonik on 2016-06-14
Status: Merged
Merged at revision: 527
Proposed branch: lp:~ntnk/gala/modifiers-only-input-source-switch
Merge into: lp:gala
Diff against target: 114 lines (+28/-28)
3 files modified
data/org.pantheon.desktop.gala.gschema.xml.in.in (+2/-0)
src/KeyboardManager.vala (+25/-4)
src/WindowManager.vala (+1/-24)
To merge this branch: bzr merge lp:~ntnk/gala/modifiers-only-input-source-switch
Reviewer Review Type Date Requested Status
Rico Tzschichholz 2016-06-14 Approve on 2016-06-22
Review via email: mp+297373@code.launchpad.net

Description of the Change

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.

To post a comment you must log in.
Rico Tzschichholz (ricotz) wrote :

Take a look at handle_switch_input_source() in WindowManager.vala

Kirill Antonik (ntnk) wrote :

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) wrote :

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.

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) wrote :

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

Rico Tzschichholz (ricotz) wrote :

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.

529. By Kirill Antonik on 2016-06-15

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

Santiago (santileortiz) wrote :

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) wrote :

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) wrote :

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) wrote :

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) wrote :
review: Needs Fixing
530. By Kirill Antonik on 2016-06-20

Updated with review changes

Kirill Antonik (ntnk) wrote :

Thanks for suggestions, updated, works as expected.

Rico Tzschichholz (ricotz) wrote :

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

@santileortiz: Could you test the branch too again?

review: Needs Fixing
Maxim Taranov (png2378) wrote :

It works for me with latest revisions.

Rico Tzschichholz (ricotz) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'data/org.pantheon.desktop.gala.gschema.xml.in.in'
2--- data/org.pantheon.desktop.gala.gschema.xml.in.in 2016-02-14 18:07:18 +0000
3+++ data/org.pantheon.desktop.gala.gschema.xml.in.in 2016-06-20 14:14:24 +0000
4@@ -134,10 +134,12 @@
5 <key type="as" name="switch-input-source">
6 <default><![CDATA[['<Alt>space']]]></default>
7 <_summary>Cycle to next keyboard layout</_summary>
8+ <description>DEPRECATED: This key is deprecated and ignored.</description>
9 </key>
10 <key type="as" name="switch-input-source-backward">
11 <default><![CDATA[['']]]></default>
12 <_summary>Cycle to previous keyboard layout</_summary>
13+ <description>DEPRECATED: This key is deprecated and ignored.</description>
14 </key>
15 </schema>
16
17
18=== modified file 'src/KeyboardManager.vala'
19--- src/KeyboardManager.vala 2016-06-16 06:53:18 +0000
20+++ src/KeyboardManager.vala 2016-06-20 14:14:24 +0000
21@@ -22,11 +22,14 @@
22 static KeyboardManager? instance;
23 static VariantType sources_variant_type;
24
25- public static void init ()
26+ public static void init (Meta.Display display)
27 {
28- if (instance == null) {
29- instance = new KeyboardManager ();
30- }
31+ if (instance != null)
32+ return;
33+
34+ instance = new KeyboardManager ();
35+
36+ display.modifiers_accelerator_activated.connect (instance.handle_modifiers_accelerator_activated);
37 }
38
39 static construct
40@@ -53,6 +56,24 @@
41 }
42
43 [CCode (instance_pos = -1)]
44+ bool handle_modifiers_accelerator_activated (Meta.Display display)
45+ {
46+ display.ungrab_keyboard (display.get_current_time ());
47+
48+ var sources = settings.get_value ("sources");
49+ return_val_if_fail (sources.is_of_type (sources_variant_type), true);
50+
51+ var n_sources = (uint) sources.n_children ();
52+ if (n_sources < 2)
53+ return true;
54+
55+ var current = settings.get_uint ("current");
56+ settings.set_uint ("current", (current + 1) % n_sources);
57+
58+ return true;
59+ }
60+
61+ [CCode (instance_pos = -1)]
62 void set_keyboard_layout (GLib.Settings settings, string key)
63 {
64 if (!(key == "current" || key == "source" || key == "xkb-options"))
65
66=== modified file 'src/WindowManager.vala'
67--- src/WindowManager.vala 2016-05-24 13:00:35 +0000
68+++ src/WindowManager.vala 2016-06-20 14:14:24 +0000
69@@ -141,7 +141,7 @@
70 DBusAccelerator.init (this);
71 #endif
72 WindowListener.init (screen);
73- KeyboardManager.init ();
74+ KeyboardManager.init (display);
75
76 // Due to a bug which enables access to the stage when using multiple monitors
77 // in the screensaver, we have to listen for changes and make sure the input area
78@@ -203,8 +203,6 @@
79 display.add_keybinding ("move-to-workspace-last", keybinding_schema, 0, (Meta.KeyHandlerFunc) handle_move_to_workspace_end);
80 display.add_keybinding ("cycle-workspaces-next", keybinding_schema, 0, (Meta.KeyHandlerFunc) handle_cycle_workspaces);
81 display.add_keybinding ("cycle-workspaces-previous", keybinding_schema, 0, (Meta.KeyHandlerFunc) handle_cycle_workspaces);
82- display.add_keybinding ("switch-input-source", keybinding_schema, 0, (Meta.KeyHandlerFunc) handle_switch_input_source);
83- display.add_keybinding ("switch-input-source-backward", keybinding_schema, 0, (Meta.KeyHandlerFunc) handle_switch_input_source);
84
85 display.overlay_key.connect (() => {
86 try {
87@@ -364,27 +362,6 @@
88 }
89
90 [CCode (instance_pos = -1)]
91- void handle_switch_input_source (Meta.Display display, Meta.Screen screen, Meta.Window? window,
92- Clutter.KeyEvent event, Meta.KeyBinding binding)
93- {
94- var keyboard_input_settings = new GLib.Settings ("org.gnome.desktop.input-sources");
95-
96- var n_sources = (uint) keyboard_input_settings.get_value ("sources").n_children ();
97- if (n_sources < 2)
98- return;
99-
100- var new_index = 0U;
101- var current_index = keyboard_input_settings.get_uint ("current");
102-
103- if (binding.get_name () == "switch-input-source")
104- new_index = (current_index + 1) % n_sources;
105- else
106- new_index = (current_index - 1 + n_sources) % n_sources;
107-
108- keyboard_input_settings.set_uint ("current", new_index);
109- }
110-
111- [CCode (instance_pos = -1)]
112 void handle_cycle_workspaces (Meta.Display display, Meta.Screen screen, Meta.Window? window,
113 Clutter.KeyEvent event, Meta.KeyBinding binding)
114 {

Subscribers

People subscribed via source and target branches