GNOME Bugzilla – Bug 576127
In case of GConf failure, fall back to local keybinding values
Last modified: 2015-12-14 21:25:31 UTC
For the gnome-shell work, we have a problem that we have a small JHBuild and we don't build GConf so we can't get the schemas properly installed. Here's a patch I came up with to use built-in values as fallbacks when there are no GConf values. I'm not sure if it's in general for Metacity: - Good because it increases robustness and decreases the amount of code. - Bad because it increases size a small amount and is just catering to broken setups. But filing it here for reference. (I haven't actually tested the no-gconf case here, it conceivably might not even compile, though it's pretty simple.)
Created attachment 131048 [details] [review] In case of GConf failure, fall back to local keybinding values Always compile the default keybindings into Metacity, and if we fail to retrieve the values from GConf, use the compiled-in value. This makes things more robust especially in an environment like JHBuild where GConf-schema-installation may not work correctly. Also use these values for the no-GConf case, rather than having a separate arrray and code path.
BTW, if anyone is wondering how many relocations/how much code size is added, there are ~25 keys that have a default keybinding; there's one constant string and a relocation pointing to that string for each of those bindings. Which strikes me as a pretty minimal increase.
Yeah, I agree that sounds like a pretty minimal increase, and I like the extra robustness (someone's going to hit some crazy bug that breaks gconf and be thankful that their window manager still behaves nicely). It's also nice to get more testing for the previously-no-gconf-only code. It would be nice to have the no-gconf case tested before committing... (and I'm more of a has-been maintainer than a maintainer, so you'll probably need to wait for Thomas' approval.)
Review of attachment 131048 [details] [review]: This patch doesn't apply cleanly; attaching a version that does and reviewing that...
Created attachment 152044 [details] [review] In case of GConf failure, fall back to local keybindings
Review of attachment 152044 [details] [review]: This sounds like a good idea, and I'm certainly willing to sacrifice a small amount of footprint for greater robustness. In testing, it didn't work with GConf off, though; I haven't yet found out why. So marking rejected, but I want to come back to this.
(In reply to comment #6) > Review of attachment 152044 [details] [review]: > > This sounds like a good idea, and I'm certainly willing to sacrifice a small > amount of footprint for greater robustness. In testing, it didn't work with > GConf off, though; I haven't yet found out why. So marking rejected, but I > want to come back to this. There's a conflict with Matthias's patch to reduce GConf roundtrips - I'll attach the patch here that gets the two working together. (And fixes a couple of minor issues's with Matthias's patch.) Other than that, seems to work for me (with mutter) with --disable-gconf. Do you have any specifics of the problem?
Created attachment 153965 [details] [review] Fix fallback to builtin defaults for key bindings The change to reduce GConf trips by using gconf_client_all_entries() broke the fallback to builtin values because update_binding() was no longer called for bindings not found in GConf. Fix this by keeping track of the bindings we find from GConf in a hash table, then looping through and setting all bindings at the end. There's also an efficiency increase here from avoiding a linear scan for each binding in GConf, though in practice the difference is probably small. This patch also corrects another minor problem introduced by the GConf round-trip patch where list key bindings were no longer taking reliably taking preference over scalar key bindings - the precendence was instead based on the order GConf returned entries. https://bugzilla.gnome.org/show_bug.cgi?id=609710
Created attachment 154162 [details] [review] Fix fallback to builtin defaults for key bindings how list and normal bindings interact - the union of both types of bindings is used when both are present, not just the list binding.
Review of attachment 154162 [details] [review]: I apologise for the time taken to review this. In that time, the code has rotted slightly. I can try to fix it up, but you would probably do it better, since it's your patch. If you'd rather I did it, let me know.