GNOME Bugzilla – Bug 709116
Settings: add a Gio.Settings wrapper.
Last modified: 2013-11-01 22:42:32 UTC
This adds a GSettings helper class that should make it more fun to work with settings. Ryan asked me to try to get this into gjs, but I feel it's good to start here first.
Created attachment 256115 [details] [review] Settings: add a Gio.Settings wrapper. The Settings class wraps a Gio.Settings object, but takes care of all GVariant (un)boxing. This should make working with settings a much less verbose experience.
Created attachment 256117 [details] [review] Settings: add a Gio.Settings wrapper. The Settings class wraps a Gio.Settings object, but takes care of all GVariant (un)boxing. This should make working with settings a much less verbose experience.
Review of attachment 256117 [details] [review]: Looks good apart from these nitpicks. ::: src/settings.js @@ +30,3 @@ + Extends: Gio.Settings, + + _types: {}, i'd name it '_key_types' or even '_key_types_dict' @@ +40,3 @@ + .get_type() + .dup_string() + .slice(1); woo! such a long chain of calls. Can we break it with variables please. @@ +45,3 @@ + + get: function(name) { + return this.get_value(name).deep_unpack(); is 'deep_unpack' something very new? I can't find it in GVariant docs on F20. @@ +46,3 @@ + get: function(name) { + return this.get_value(name).deep_unpack(); + }, newline btw function bodies.
Review of attachment 256117 [details] [review]: Sorry for the late reply. :) ::: src/settings.js @@ +30,3 @@ + Extends: Gio.Settings, + + _types: {}, "keys" in list_keys refers to the names of the settings. So if we want a more descriptive name I think it would be _setting_types but that seems redundant given that we are already in a Settings class. Would a small comment like this suffice: // The GVariant type formats of the settings _types: {} or just _typeFormats? @@ +40,3 @@ + .get_type() + .dup_string() + .slice(1); Yeah, and I should add a comment on what I'm actually doing and why. The way to get the type of a setting is a bit uncomfortable, but well documented. So a link to the relevant documentation also I guess. Will add that and do 1 or 2 variable breakups (even though I much prefer the js chaining style). @@ +45,3 @@ + + get: function(name) { + return this.get_value(name).deep_unpack(); It's in GJS, they have some custom monkey patches there, deep_unpack is one of them. Jasper is (understandably) not too keen on adding more, for the reason of documentation. @@ +46,3 @@ + get: function(name) { + return this.get_value(name).deep_unpack(); + }, True!
Created attachment 258154 [details] [review] Settings: add a Gio.Settings wrapper. The Settings class wraps a Gio.Settings object, but takes care of all GVariant (un)boxing. This should make working with settings a much less verbose experience. This version incorporates stuff from the discussion above, plus I found a shorter way to list the types.
Review of attachment 258154 [details] [review]: ack /w that changed. ::: src/settings.js @@ +31,3 @@ + + // The GVariant type formats of the settings + _typeFormats: {}, Formats? I don't think anyone calls it that. Its just types. Since we are mapping keys of types, just key_types is good as I suggested before. or call it 'types_hash' or 'types_map'.
Created attachment 258789 [details] [review] Settings: add a Gio.Settings wrapper. The Settings class wraps a Gio.Settings object, but takes care of all GVariant (un)boxing. This should make working with settings a much less verbose experience.
Review of attachment 258789 [details] [review]: COmmited.