GNOME Bugzilla – Bug 682315
input sources: add an OSD for cycling through engines
Last modified: 2013-02-16 09:28:31 UTC
Takao demonstrated the OSD-approach that ibus uses for switching engines during the input sources bof at guadec, and we agreed that this is a good idea.
Created attachment 231357 [details] [review] keybindings: Give dynamic keybindings a keybindings action -- This is a patch from Florian needed for the shell side. I massaged it a bit to apply to current master.
Created attachment 231358 [details] [review] main: Add an ALL value to KeybindingMode
Created attachment 231359 [details] [review] main: Init WindowManager before Panel This allows us to register keybindings from the panel or any status indicator.
Created attachment 231360 [details] [review] windowManager: Return the KeyBindingAction value from addKeybinding()
Created attachment 231361 [details] [review] Add the new switch-input-source keybinding
Created attachment 231362 [details] [review] status/keyboard: Keep a list of input sources in MRU order This will allows us to provide an Alt+Tab like input source switcher.
Created attachment 231363 [details] [review] status/keyboard: Add an Alt+Tab like input source switcher
Patch "status/keyboard: Keep IBus properties per engine" in bug 682318 is a requirement for this.
Review of attachment 231359 [details] [review]: Any reason the WM isn't first? Then we can register keybindings from anywhere.
Review of attachment 231357 [details] [review]: Why did you remove #MetaKeyBindingAction? ::: src/core/keybindings.c @@ +663,3 @@ * @mask: Event mask * + * Get the keybinding action bound to %keycode. Builtin keybindings just noticed, but this documentation is incorrect.
Review of attachment 231358 [details] [review]: A better commit message would be nice.
Review of attachment 231360 [details] [review]: A better commit message would be nice. ::: js/ui/windowManager.js @@ +193,3 @@ addKeybinding: function(name, settings, flags, modes, handler) { + let action; + action = global.display.add_keybinding(name, settings, flags, handler); let action =
Review of attachment 231361 [details] [review]: OK.
Review of attachment 231362 [details] [review]: Sure.
Review of attachment 231363 [details] [review]: ::: js/ui/status/keyboard.js @@ +251,3 @@ + _initialSelection: function(backward, binding) { + if (binding == 'switch-input-source') { + if (backward) I think we need to fix these shenanigans, but maybe now is not the time. @@ +340,3 @@ + Main.wm.addKeybinding('switch-input-source-backward', + new Gio.Settings({ schema: "org.gnome.shell.keybindings" }), + Meta.KeyBindingFlags.REVERSES, Isn't there also a REVERSED flag you need to add?
Review of attachment 231362 [details] [review]: Typo in the commit message: either 'this will allow us' or 'this allows us'
hmm, couldn't get this to apply to master - am I missing some other patches ?
(In reply to comment #10) > Why did you remove #MetaKeyBindingAction? I guess that's because the function isn't returning a MetaKeyBindingAction anymore but a plain guint. > ::: src/core/keybindings.c > @@ +663,3 @@ > * @mask: Event mask > * > + * Get the keybinding action bound to %keycode. Builtin keybindings > > just noticed, but this documentation is incorrect. What's wrong with it?
(In reply to comment #11) > A better commit message would be nice. Got any suggestions? It seems really straightforward to me.
Created attachment 231454 [details] [review] main: Initialize WindowManager earlier -- (In reply to comment #9) > Any reason the WM isn't first? Then we can register keybindings from anywhere. Without suffling more code around this is as earlier as we can go. Only Overview, CtrlAltTabManager, XdndHandler and LayoutManager are before WM now. From those, I think only the Overview could make sense to be after WM but since the Overview already has a 2 step init process exactly because of other places wanting to hook to it on their own _init()s I think it's ok to leave it like this.
Created attachment 231456 [details] [review] status/keyboard: Add an Alt+Tab like input source switcher -- (In reply to comment #15) > + _initialSelection: function(backward, binding) { > + if (binding == 'switch-input-source') { > + if (backward) > > I think we need to fix these shenanigans, but maybe now is not the time. I agree. As far as I'm concened we could get rid of all the *-backward keybindings. REVERSES should be enough. As it is you can have e.g.: '<Super>space' on switch-input-source '<Super>i' on switch-input-source-backward 1.a) Super+space goes forward 1.b) Shift+Super+space goes backward 2.a) Super+i goes backward 2.b) Shift+Super+i goes forward And the same thing for all the other *-backward keybindings... > @@ +340,3 @@ > + Main.wm.addKeybinding('switch-input-source-backward', > + new Gio.Settings({ schema: > "org.gnome.shell.keybindings" }), > + Meta.KeyBindingFlags.REVERSES, > > Isn't there also a REVERSED flag you need to add? Yes, thanks for catching.
Fixed the other comments and typos locally.
Created attachment 231469 [details] Screenshot (In reply to comment #17) > hmm, couldn't get this to apply to master - am I missing some other patches ? All the patches here should go on top of the ones in bug 682318. The correct order should be: * main: Add an ALL value to KeybindingMode * main: Initialize WindowManager earlier * windowManager: Return the KeyBindingAction value from addKeybinding() * Add the new switch-input-source keybinding * status/keyboard: Keep a list of input sources in MRU order * status/keyboard: Add an Alt+Tab like input source switcher Anyway, here's a screenshot of how it looks.
(In reply to comment #18) > (In reply to comment #10) > > Why did you remove #MetaKeyBindingAction? > > I guess that's because the function isn't returning a MetaKeyBindingAction > anymore but a plain guint. Ah right, it's an enum and introspection can't handle that. > > ::: src/core/keybindings.c > > @@ +663,3 @@ > > * @mask: Event mask > > * > > + * Get the keybinding action bound to %keycode. Builtin keybindings > > > > just noticed, but this documentation is incorrect. > > What's wrong with it? It should be @keycode
Review of attachment 231454 [details] [review]: OK.
Review of attachment 231456 [details] [review]: OK.
(In reply to comment #24) > > > ::: src/core/keybindings.c > > > @@ +663,3 @@ > > > * @mask: Event mask > > > * > > > + * Get the keybinding action bound to %keycode. Builtin keybindings > > > > > > just noticed, but this documentation is incorrect. > > > > What's wrong with it? > > It should be @keycode Gotcha, thanks. Is this patch a-c-n with this amended?
Review of attachment 231357 [details] [review]: Yes
Attachment 231358 [details] pushed as a42d35d - main: Add an ALL value to KeybindingMode Attachment 231360 [details] pushed as b767849 - windowManager: Return the KeyBindingAction value from addKeybinding() Attachment 231361 [details] pushed as 88a9b76 - Add the new switch-input-source keybinding Attachment 231362 [details] pushed as 6eef830 - status/keyboard: Keep a list of input sources in MRU order Attachment 231454 [details] pushed as a9ec8a3 - main: Initialize WindowManager earlier Attachment 231456 [details] pushed as f615482 - status/keyboard: Add an Alt+Tab like input source switcher
Attachment 231357 [details] pushed as d78de37 - keybindings: Give dynamic keybindings a keybindings action