GNOME Bugzilla – Bug 746509
Fixes to search panel vs shell interaction
Last modified: 2015-03-27 20:15:55 UTC
The first one might be good to backport to 3.14 too.
Created attachment 299916 [details] [review] Search: watch for changes to enabled as well When introducing support for "default disabled" search providers this part was overlooked, so enabling a default disabled provider would be ignored until the next login.
Created attachment 299917 [details] [review] Search: read all keys after connecting to change notifications In recent glib, change notifications don't actually happen unless all keys have been read, in an effort to reduce unnecessary dbus traffic for shortlived GSettings object and avoid AddMatch calls. But we care about changes here, so make sure we are subscribed.
Review of attachment 299916 [details] [review]: LGTM
Review of attachment 299917 [details] [review]: ::: js/ui/search.js @@ +429,3 @@ + // about) + for (let key of ['disabled', 'enabled', 'disable-external', 'sort-order']) + this._searchSettings.get_value(key); Ugh, this is ugly. Would it be better to add this._searchSettings as parameter to loadRemoteSearchProviders() and let the reading happen "naturally"?
Created attachment 299987 [details] [review] Search: use the same settings object for loading search providers In recent glib, change notifications don't actually happen unless all keys have been read, in an effort to reduce unnecessary dbus traffic for shortlived GSettings object and avoid AddMatch calls. But we care about changes here, so we need to make sure we're subscribed, and an easy way to do so is to reuse the same object to watch for changes and to load the active providers at startup.
Created attachment 299988 [details] [review] AppDisplay: use global settings object We don't need a different GSettings object for each app or favorite item. While it practice it does not change much (AddMatch is still obviously sent out), it minimally reduces the overhead on changes, and makes for cleaner code.
Created attachment 299989 [details] [review] ExtensionPrefs: remove unused GSettings object Each ExtensionRow has its own
Created attachment 299990 [details] [review] Background: add a comment for questionable GSettings usage BackgroundSource relies on Background to watch its own settings. Add a comment to explain that.
Created attachment 299991 [details] [review] Magnifier: invert connection to settings changes and initial read If there is no signal connected to changed for a key, get_value() will not subscribe to changes.
Created attachment 299992 [details] [review] status/a11y: invert connection to changes and initial read If there is no signal connected to changed, get_value() will not subscribe to notifications and we might miss changes.
Created attachment 299993 [details] [review] dash: use global settings instead of own settings object Makes it easier to reason about change notifications
Created attachment 299994 [details] [review] System: remove unused GSettings
Created attachment 299995 [details] [review] System: update lock screen at construction Will make sure that change notifications are enabled by reading the value at least once
Created attachment 299996 [details] [review] WorkspacesView: remove unused GSettings
Created attachment 299997 [details] [review] prefs: connect to changed:: before reading the value of a setting Otherwise glib might skip registering to change notifications
Sorry I hijacked this bug to clean up GSettings usage. Some of these are real bugs (the keybindings one probably is, at least for some schemas), some probably still work by chance (the magnifier one, on the account that appsettings changes when settings changes, maybe a11y too works?), some are just cleanups.
Review of attachment 299987 [details] [review]: OK
Review of attachment 299988 [details] [review]: Sure
Review of attachment 299989 [details] [review]: Yes
Review of attachment 299991 [details] [review]: OK
Review of attachment 299993 [details] [review]: Yes
Review of attachment 299994 [details] [review]: You should remove now unused SCHEMA consts as well
Review of attachment 299996 [details] [review]: OK
Review of attachment 299995 [details] [review]: OK
Review of attachment 299992 [details] [review]: Sure
Review of attachment 299997 [details] [review]: LGTM
Review of attachment 299990 [details] [review]: OK
By the way, given that the vast majority of the patches are cleanups, I don't think this should be on the 3.16 blocker list.
yeah, lets take this for 3.16.1
Yeah, this was never meant for 3.16.0, and easy workarounds exist for people who are bitten by this. I just happened to discover it now.
Attachment 299916 [details] pushed as fd6ef48 - Search: watch for changes to enabled as well Attachment 299987 [details] pushed as 048a14f - Search: use the same settings object for loading search providers Attachment 299988 [details] pushed as 1f2e53d - AppDisplay: use global settings object Attachment 299989 [details] pushed as 36d9cc3 - ExtensionPrefs: remove unused GSettings object Attachment 299990 [details] pushed as 0068098 - Background: add a comment for questionable GSettings usage Attachment 299991 [details] pushed as bbe4171 - Magnifier: invert connection to settings changes and initial read Attachment 299992 [details] pushed as 49fe033 - status/a11y: invert connection to changes and initial read Attachment 299993 [details] pushed as 018025d - dash: use global settings instead of own settings object Attachment 299994 [details] pushed as e1b575d - System: remove unused GSettings Attachment 299995 [details] pushed as 8327dbd - System: update lock screen at construction Attachment 299996 [details] pushed as e87656a - WorkspacesView: remove unused GSettings
Attachment 299997 [details] pushed as a13f906 - prefs: connect to changed:: before reading the value of a setting