GNOME Bugzilla – Bug 777342
Reduce dependency on Caribou
Last modified: 2017-05-15 12:52:49 UTC
I'm attaching some patches that eliminates the runtime dependency on the Caribou daemon. All it does is figuring out the current focus through accessibility events, which is pretty much 1:1 with the FocusCaretTracker implementation inside gnome-shell, we can just reuse this one and avoid spawning daemons to do the work for us. After these patches, we just use Caribou for 1) the keyboard models, and 2) the CaribouDisplayAdapter, which is poked by the model when we activate keys. For the latter we now use the same on both x11/wayland, which will use the internal virtual input device Clutter API.
Created attachment 343570 [details] [review] keyboard: Avoid runtime dependency on the Caribou daemon The caribou daemon only gives us focus tracking, which is almost 1:1 with our own FocusCaretTracker implementation. This means we can entirely replace the Caribou daemon inside gnome-shell, reducing the Caribou dependency to just libcaribou, and more specifically the CaribouKeyboardModel we pull the keyboard models from. As we still need underneath a CaribouDisplayAdapter to drive the keyboard, reuse the wayland one, which has been renamed to make it look generic, plus it will use the virtual input device API from mutter/clutter.
Created attachment 343571 [details] [review] keyboard: Remove Show/Hide calls And merge with the "internal" show/hide() ones. Those functions don't proxy dbus method calls anymore, so it makes no sense to expose these. Also, the timestamp is no longer needed as there is a single source for these events.
Created attachment 343572 [details] [review] keyboard: Drop dbus naming semantics from Set[Entry|Cursor]Location Those functions don't proxy dbus method calls anymore, so just drop the uppercase 'S'.
Created attachment 343652 [details] [review] keyboard: Avoid runtime dependency on the Caribou daemon The caribou daemon only gives us focus tracking, which is almost 1:1 with our own FocusCaretTracker implementation. This means we can entirely replace the Caribou daemon inside gnome-shell, reducing the Caribou dependency to just libcaribou, and more specifically the CaribouKeyboardModel we pull the keyboard models from. As we still need underneath a CaribouDisplayAdapter to drive the keyboard, reuse the wayland one, which has been renamed to make it look generic, plus it will use the virtual input device API from mutter/clutter.
Created attachment 343653 [details] [review] keyboard: Remove Show/Hide calls And merge with the "internal" show/hide() ones. Those functions don't proxy dbus method calls anymore, so it makes no sense to expose these. Also, the timestamp is no longer needed as there is a single source for these events.
Created attachment 343654 [details] [review] keyboard: Drop dbus naming semantics from Set[Entry|Cursor]Location Those functions don't proxy dbus method calls anymore, so just drop the uppercase 'S'.
Review of attachment 343652 [details] [review]: ::: js/ui/keyboard.js @@ +168,3 @@ + this._focusCaretTracker.connect('caret-moved', Lang.bind(this, this._onCaretMoved)); + this._focusCaretTracker.registerFocusListener(); + this._focusCaretTracker.registerCaretListener(); Atspi initialization used to trigger some pretty bad performance degradation under certain circumstances, so to minimize that impact it was deferred to when any actual listeners are registered (see bug 730118). Those listeners are now registered unconditionally, so does that mean that the performance hit will be back with this patch? @@ +211,3 @@ + GLib.source_remove(this._updateCaretPositionId); + this._updateCaretPositionId = GLib.idle_add(GLib.PRIORITY_DEFAULT_IDLE, Lang.bind(this, function() { + this._updateCaretPositionId = null; Mmmh, I do prefer 0 (or possibly undefined) for numerical properties, but it looks like this is consistent with other properties in this file. Would be good style to initialize it in _init() though. @@ +214,3 @@ + + let currentWindow = global.screen.get_display().focus_window; + if (!currentWindow) What about our own chrome (run dialog, overview search etc.)?
Review of attachment 343653 [details] [review]: LGTM
Review of attachment 343654 [details] [review]: Sure
(In reply to Florian Müllner from comment #7) > Review of attachment 343652 [details] [review] [review]: > > ::: js/ui/keyboard.js > @@ +168,3 @@ > + this._focusCaretTracker.connect('caret-moved', Lang.bind(this, > this._onCaretMoved)); > + this._focusCaretTracker.registerFocusListener(); > + this._focusCaretTracker.registerCaretListener(); > > Atspi initialization used to trigger some pretty bad performance degradation > under certain circumstances, so to minimize that impact it was deferred to > when any actual listeners are registered (see bug 730118). Those listeners > are now registered unconditionally, so does that mean that the performance > hit will be back with this patch? Uhmm, now that you mention I did notice devhelp search became slower, so it might be still noticeable, although not as much as when that bug was originally reported. I think it can be changed so we just enable focus tracking while the osk might be shown (i.e. there's touch events or it's toggled on), just might require to ensure the FocusCaretTracker emits events right after registration. That'd be most similar to how we did it through the Caribou daemon. > > @@ +211,3 @@ > + GLib.source_remove(this._updateCaretPositionId); > + this._updateCaretPositionId = > GLib.idle_add(GLib.PRIORITY_DEFAULT_IDLE, Lang.bind(this, function() { > + this._updateCaretPositionId = null; > > Mmmh, I do prefer 0 (or possibly undefined) for numerical properties, but it > looks like this is consistent with other properties in this file. Would be > good style to initialize it in _init() though. Sure, will change that :). > > @@ +214,3 @@ > + > + let currentWindow = global.screen.get_display().focus_window; > + if (!currentWindow) > > What about our own chrome (run dialog, overview search etc.)? the updateCaretPosition() function is only for windows, there's been for a long time the hooks to move/resize the focus window when showing the osk so the caret location isn't occluded. It would be nice to do that a once, just not in this bug I guess :).
Created attachment 344330 [details] [review] keyboard: Avoid runtime dependency on the Caribou daemon The caribou daemon only gives us focus tracking, which is almost 1:1 with our own FocusCaretTracker implementation. This means we can entirely replace the Caribou daemon inside gnome-shell, reducing the Caribou dependency to just libcaribou, and more specifically the CaribouKeyboardModel we pull the keyboard models from. As we still need underneath a CaribouDisplayAdapter to drive the keyboard, reuse the wayland one, which has been renamed to make it look generic, plus it will use the virtual input device API from mutter/clutter.
This new patch just registers the listeners when needed, and doesn't have as many idles, just one left in the updateCaretPosition function, as that turns out expensive given the many consecutive events we can get from the caret tracker. The other patches need minor updates on top of this one, which I have locally.
Review of attachment 344330 [details] [review]: LGTM
Thanks for the reviews :). Pushed this to master. Attachment 343653 [details] pushed as c324395 - keyboard: Remove Show/Hide calls Attachment 343654 [details] pushed as 41baf0f - keyboard: Drop dbus naming semantics from Set[Entry|Cursor]Location Attachment 344330 [details] pushed as aecd1c1 - keyboard: Avoid runtime dependency on the Caribou daemon