GNOME Bugzilla – Bug 766826
Restore input source when logging in
Last modified: 2016-05-27 17:48:59 UTC
This seems to be a regression from commit 8589bfb62ea4fa494bba99f1a58bf648bedbaacc and https://bugzilla.gnome.org/show_bug.cgi?id=736435. When logging in, the first input source is restored as opposed to the one that was current when the session was terminated. There is a "current" GSetting in the input-sources schema, but the Shell ignores it at the moment. See attached patches.
Created attachment 328415 [details] [review] ibusManager: signal readyness when there's no IBus We're always ready in that case, since operations are no-ops.
Created attachment 328416 [details] [review] keyboard: save the current source when it's switched And restore it when reloading the current list of sources, if appropriate.
I can't find it anywhere now, but the reasons to stop saving the current input source were: 1) the original use for the current key was IPC between g-s-d and gnome-shell and since we moved all that functionality into gnome-shell we didn't need it anymore 2) stop writing a setting to disk in particular during a session cold start and in cases where switching is frequent such as when you have per-window input sources and alt+tab between 2 different windows often 3) have a more predictable start state since during run time we have an alt+tab like switcher (MRU), just saving the current source isn't enough If you think we should really save this, I'd prefer that we a) save the whole MRU list b) only save it when the session ends
(In reply to Rui Matos from comment #3) > I can't find it anywhere now, but the reasons to stop saving the current > input source were: > > 1) the original use for the current key was IPC between g-s-d and > gnome-shell and since we moved all that functionality into gnome-shell we > didn't need it anymore OK - I guess we should also remove the code that reads that key from gnome-control-center then. > 2) stop writing a setting to disk in particular during a session cold start > and in cases where switching is frequent such as when you have per-window > input sources and alt+tab between 2 different windows often I agree with both of these points: - we should avoid writing the setting on a session cold start - I don't think it makes sense to save anything when settings are per-window > 3) have a more predictable start state since during run time we have an > alt+tab like switcher (MRU), just saving the current source isn't enough > > If you think we should really save this, I'd prefer that we > > a) save the whole MRU list > b) only save it when the session ends I can try to change the code to save the whole MRU list instead. About saving it only when the session ends, I think that as long as your comments above are addressed, it should not matter much. I will try to provide an updated patchset.
I filed https://bugzilla.gnome.org/show_bug.cgi?id=766846 and https://bugzilla.gnome.org/show_bug.cgi?id=766847 to complete the deprecation of the "current" GSettings key.
Created attachment 328459 [details] [review] ibusManager: signal readyness when there's no IBus We're always ready in that case, since operations are no-ops.
Created attachment 328460 [details] [review] keyboard: remove unused code Nothing ever sets those attributes on Main.overview, so there's no point in deleting them here.
Created attachment 328461 [details] [review] keyboard: split out a function We're going to use this in a later commit.
Created attachment 328462 [details] [review] keyboard: save the MRU input sources list when switching And restore it when reloading the current list of sources, if appropriate.
Created attachment 328463 [details] [review] input-sources: add a GSettings key for the MRU input sources The shell will save the MRU list here when switching input source.
Review of attachment 328459 [details] [review]: Why do you need this? ::: js/misc/ibusManager.js @@ +48,3 @@ + GLib.idle_add(Lang.bind(this, function() { + // If we have no IBus, signal we're ready + this.emit('ready', true); this should probably be false
Review of attachment 328460 [details] [review]: we do, see _getCurrentWindow() and its callers
Review of attachment 328462 [details] [review]: ::: js/ui/status/keyboard.js @@ +435,3 @@ + // If input sources are per-window, no point in saving the setting + if (this._sourcesPerWindow) + return; this shouldn't be needed with my suggestion below @@ +459,3 @@ } + this._updateCurrentSourceSettings(); we won't avoid writing the gsetting on startup this way, right? my suggestion would be to add an argument to the InputSource::activate signal and the method that triggers it, and only the callers that are a result of explicit user action would set it in order to cause _activateInputSource() (the signal handler) to call this save method @@ +587,3 @@ + this._mruSources.push(mruSource); + } + } this function is becoming a little too long, can you move this hunk to a different function?
Review of attachment 328461 [details] [review]: ok
Review of attachment 328463 [details] [review]: sure
Created attachment 328521 [details] [review] keyboard: split out a function to udpate the MRU list We're going to add saving of the MRU list in the function in a later commit.
Created attachment 328522 [details] [review] keyboard: add an interactive argument to input source activation This is useful to differentiate between a change due to user interaction or automatic loading.
Created attachment 328523 [details] [review] keyboard: save the MRU input sources list when switching And restore it when reloading the current list of sources, if appropriate.
Thanks for the review, Rui. I attached a new set of patches that should address your comments.
Review of attachment 328522 [details] [review]: I'd prefer that the .activate() calls in _inputSourcesChanged() and _setPerWindowInputSource() are changed to have an explicit 'false' argument with that fixed, looks good ::: js/ui/status/keyboard.js @@ +439,2 @@ this._ibusManager.setEngine(engine, KeyboardManager.releaseKeyboard); + this._currentInputSourceChanged(is, interactive); there's no need to propagate this further into _currentInputSourceChanged(), we can handle the settings update here.
Review of attachment 328523 [details] [review]: Ootherwise looks good, thanks for the patches! ::: js/ui/status/keyboard.js @@ +456,3 @@ + if (interactive) + this._updateMruSettings(); This could move to _activateInputSource() as mentioned on the other patch
Review of attachment 328521 [details] [review]: ok
Comment on attachment 328463 [details] [review] input-sources: add a GSettings key for the MRU input sources Attachment 328463 [details] pushed as b866971 - input-sources: add a GSettings key for the MRU input sources
Thanks for the review, I pushed these to master with your suggestions. Attachment 328461 [details] pushed as 9aa3d86 - keyboard: split out a function Attachment 328521 [details] pushed as f818877 - keyboard: split out a function to udpate the MRU list Attachment 328522 [details] pushed as 5c0eba7 - keyboard: add an interactive argument to input source activation Attachment 328523 [details] pushed as 2ea6ae0 - keyboard: save the MRU input sources list when switching