GNOME Bugzilla – Bug 622344
Support GVariants
Last modified: 2011-06-23 22:11:00 UTC
It would be nice to decode and encode GVariants.
WIP branch: http://git.collabora.co.uk/?p=user/danni/gjs.git;a=shortlog;h=refs/heads/gvariant-622344
Created attachment 164281 [details] [review] GVariant tests for Everything.gir Adds a number of GVariant tests to Everything
Created attachment 164283 [details] [review] gjs_value_from_g_variant Demarshals basic types, arrays and dicts. Doesn't yet handle tuples because I'm not sure how to create them in gjs.
+JSBool +gjs_value_from_g_variant (JSContext *context, + jsval *value_p, + GVariant *gvariant) +{ + if (g_variant_is_container (gvariant)) { + } else if (g_variant_is_of_type (gvariant, G_VARIANT_TYPE_BOOLEAN)) { + } else if (g_variant_is_of_type (gvariant, G_VARIANT_TYPE_BYTE)) { + } else if (g_variant_is_of_type (gvariant, G_VARIANT_TYPE_INT16)) { [...] You should probably use switch (g_variant_classify(gvariant)) { ... } here instead of this if ... else ... else ... cascade. Also you're not handling SIGNATURE variants.
Talked to smcv from DBus and Telepathy. He thinks we shouldn't immediately unpack GVariants, because doing so makes them impossible to correctly remarshal again (type information is lost). Instead we should provide an unpack() method on the JSObject representing a variant that will unpack the variant into native types.
9 months later... patch coming, that makes GVariant first class objects like GBoxeds and GObjects. Depends on some possibly controversial patches against glib and introspection and its missing all the convenience functions, that I plan to implement in JS using overrides.
Created attachment 185041 [details] [review] Support marshalling arguments that are GVariants Initial GVariant support, includes only obtaining a GVariant from a method that returns one and passing it around mantaining a correct reference count.
Comment on attachment 185041 [details] [review] Support marshalling arguments that are GVariants >+ gjs_define_static_methods (context, constructor, info); this change is really separate from "variant support", and is really needed for better boxed support as well anyway.
Created attachment 185100 [details] [review] start of GVariant support curiously, I had also started poking at GVariant support last week (totally missing this bug and the fact that some work had already been done). I'm attaching what I did here. It compiles but is totally untested.
Created attachment 185225 [details] [review] GVariant: add JS wrappers for packing and unpacking Using JS overrides, adds glue code that packs and unpacks GVariants into JS objects (hash maps, arrays and primitive types)
Created attachment 187522 [details] [review] GVariant: add JS wrappers for packing and unpacking Using JS overrides, adds glue code that packs and unpacks GVariants into JS objects (hash maps, arrays and primitive types)
Created attachment 188152 [details] [review] Support marshalling arguments that are GVariants Initial GVariant support, includes only obtaining a GVariant from a method that returns one and passing it around mantaining a correct reference count. (Moved inside this patch a part that previously was in the JS one)
Created attachment 188153 [details] [review] GVariant: add JS wrappers for packing and unpacking Using JS overrides, adds glue code that packs and unpacks GVariants into JS objects (hash maps, arrays and primitive types) (Updated for the move from __init__ to _init)
Review of attachment 188152 [details] [review]: One request to split out, otherwise good to commit. ::: gi/boxed.c @@ +1274,3 @@ + constructor = JSVAL_TO_OBJECT(value); + gjs_define_static_methods (context, constructor, info); I think danw mentioned this too, but basically this should be a separate change with a distinct rationale.
Review of attachment 188153 [details] [review]: I think I'd need to see some tests for this one.
Review of attachment 188153 [details] [review]: The approach taken here seems quite sane overall for construction. On the other side, I wonder if it would be possible to avoid 'unpack', however. Is it possible to create a wrapper around GVariant that you can use the normal javascript operations like [] and 'for (... in ...)' with? Is it further possible to have GVariant number-like and string-like objects that can act as stand-ins for the usual javascript equivalents without discarding their static type information? ::: modules/overrides/GLib.js @@ +23,3 @@ +let originalVariantClass; + +function _read_single_type(signature) { this function is happy to read invalid type signatures: - 'z' (which corresponds to no type at all) - '{asas}' (key type in dict entry must be a simple type) and the code below feeds that directly into GVariant. That will cause criticals. @@ +91,3 @@ + return GLib.Variant.new_maybe(null, _pack_variant(signature, value)) + else + return GLib.Variant.new_maybe(_read_single_type(signature).join(''), null); my lack of javascript skill is showing: what is the purpose of this? @@ +98,3 @@ + return GLib.Variant.new_strv(value, value.length); + } + if (arrayType[0] == 'y') { not all 'ay' will be bytestrings. it's quite possible that you want to encode an actual array of numbers as type 'ay' in which case this will fail. probably you should check the type of 'value' and act accordingly. @@ +103,3 @@ + } + let variantType = GLib.VariantType.new('a' + arrayType.join('')); + let builder = GLib.VariantBuilder.new(variantType); i guess it is preferable to use Variant.new_array() here @@ +186,3 @@ + return val; + case 'a': + if (variant.is_of_type(GLib.VariantType.new('a{?*}'))) { We have a G_VARIANT_TYPE_DICTIONARY constant for this. @@ +192,3 @@ + for (let i = 0; i < nElements; i++) { + // always unpack the dictionary entry, and always deep unpack + // the key (or it cannot be added as a key) keys are always simply-typed so 'deep unpack' has no relevance @@ +206,3 @@ + case '(': + case '{': + let ret = [ ]; does javascript have tuples?
Created attachment 188600 [details] [review] Support static methods for boxed and structure types Expose all methods that don't have GI_FUNCTION_IS_METHOD flag set as static methods on the JS class constructor. Currently, for boxed, struct and variant types, only C constructors (class_new_foo) are associated, other static methods are left as functions in the global namespace.
Created attachment 188601 [details] [review] Support marshalling arguments that are GVariants Initial GVariant support, includes only obtaining a GVariant from a method that returns one and passing it around mantaining a correct reference count. Splitted into two commits.
Review of attachment 188600 [details] [review]: See also: https://bugzilla.gnome.org/show_bug.cgi?id=572408 After that lands we may want to refactor gjs_define_static_methods into a utility function.
Review of attachment 188601 [details] [review]: Looks good, though I'd really like to see some basic tests. I just took Danielle's patch, adapated it for recent g-i, and pushed to git master.
Comment on attachment 188600 [details] [review] Support static methods for boxed and structure types Attachment 188600 [details] pushed as b5bfae7 - Support static methods for boxed and structure types
Review of attachment 188153 [details] [review]: ::: modules/overrides/GLib.js @@ +199,3 @@ + else + key = val[0]; + ret[key] = val[1]; One issue with this is namespace collisions with default JavaScript object properties like "toString" and the rest: https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Object I could imagine someone having a property called "watch" say, and then being confused why the method doesn't work later. Do we care? Maybe it's not a big deal. And actually we need to carefully think about whether the stringification of all gvariant keys is going to be unique. I'm pretty sure they will be though.
Created attachment 189012 [details] [review] Regress: fix GVariant tests Functions that return floating GVariants must be marked (transfer none).
Created attachment 189019 [details] [review] Support marshalling arguments that are GVariants Initial GVariant support, includes only obtaining a GVariant from a method that returns one and passing it around mantaining a correct reference count. Has some basic unit tests, will add more alongside the JS convenience layer.
(In reply to comment #16) > Review of attachment 188153 [details] [review]: > > The approach taken here seems quite sane overall for construction. > > On the other side, I wonder if it would be possible to avoid 'unpack', however. > Is it possible to create a wrapper around GVariant that you can use the normal > javascript operations like [] and 'for (... in ...)' with? Is it further > possible to have GVariant number-like and string-like objects that can act as > stand-ins for the usual javascript equivalents without discarding their static > type information? This is not Python, unfortunately. You can have objects that auto cast to numbers and strings (using valueOf), but you cannot override [] and enumeration without resorting to the C API. > > ::: modules/overrides/GLib.js > @@ +23,3 @@ > +let originalVariantClass; > + > +function _read_single_type(signature) { > > this function is happy to read invalid type signatures: > > - 'z' (which corresponds to no type at all) > > - '{asas}' (key type in dict entry must be a simple type) > > > and the code below feeds that directly into GVariant. That will cause > criticals. Ok, I'll fix it. > @@ +91,3 @@ > + return GLib.Variant.new_maybe(null, _pack_variant(signature, value)) > + else > + return GLib.Variant.new_maybe(_read_single_type(signature).join(''), > null); > > my lack of javascript skill is showing: what is the purpose of this? Maybe types can be empty, or can be full. If they're empty, value will be null, so we cannot call _pack_variant. Otherwise, they're full, and we don't need to build a GVariantType (the type is inferred from the value). > @@ +98,3 @@ > + return GLib.Variant.new_strv(value, value.length); > + } > + if (arrayType[0] == 'y') { > > not all 'ay' will be bytestrings. it's quite possible that you want to encode > an actual array of numbers as type 'ay' in which case this will fail. All 'ay' are bytestrings, in the sense they're all arrays of guint8. Strings are converted to guint8*, and so are Arrays. > probably you should check the type of 'value' and act accordingly. gjs does that for us. > @@ +103,3 @@ > + } > + let variantType = GLib.VariantType.new('a' + arrayType.join('')); > + let builder = GLib.VariantBuilder.new(variantType); > > i guess it is preferable to use Variant.new_array() here Yes, will fix. > @@ +186,3 @@ > + return val; > + case 'a': > + if (variant.is_of_type(GLib.VariantType.new('a{?*}'))) { > > We have a G_VARIANT_TYPE_DICTIONARY constant for this. Ok. > @@ +192,3 @@ > + for (let i = 0; i < nElements; i++) { > + // always unpack the dictionary entry, and always deep unpack > + // the key (or it cannot be added as a key) > > keys are always simply-typed so 'deep unpack' has no relevance You're right, I'll remove it. > @@ +206,3 @@ > + case '(': > + case '{': > + let ret = [ ]; > > does javascript have tuples? No. I emulated them with arrays (as Gjs currently does in DBus). (In reply to comment #22) > Review of attachment 188153 [details] [review]: > > ::: modules/overrides/GLib.js > @@ +199,3 @@ > + else > + key = val[0]; > + ret[key] = val[1]; > > One issue with this is namespace collisions with default JavaScript object > properties like "toString" and the rest: > https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Object > > I could imagine someone having a property called "watch" say, and then being > confused why the method doesn't work later. Do we care? Maybe it's not a big > deal. All those properties are replaceable, so it is fine to set them. > And actually we need to carefully think about whether the stringification of > all gvariant keys is going to be unique. I'm pretty sure they will be though. The problem would be with big integers (over 2^54), as they are rounded. I don't known if there is some way to represent them in JS, though. Double and float stringification should be unique enough, and everything else is an integer or a string. (and in any case one can obtain the variant and then iterate keys and values as if it was an array)
Created attachment 189024 [details] [review] GVariant: add JS wrappers for packing and unpacking Using JS overrides, adds glue code that packs and unpacks GVariants into JS objects (hash maps, arrays and primitive types)
Review of attachment 189012 [details] [review]: Yep, sorry for getting that wrong.
Comment on attachment 189012 [details] [review] Regress: fix GVariant tests Attachment 189012 [details] pushed as e70cdbc - Regress: fix GVariant tests
Comment on attachment 189019 [details] [review] Support marshalling arguments that are GVariants >+ /* Short-circuit construction for GVariants (simply cannot construct here, >+ the constructor should be overridden) */ >+ if (g_type_is_a(gtype, G_TYPE_VARIANT)) { >+ gjs_throw(context, >+ "Can't create instance of GVariant directly, use GVariant.new_*"); >+ return JS_FALSE; >+ } blah. "new GVariant()" should wrap g_variant_new()
ah, i see, you do that in the next patch. shrug.
(In reply to comment #25) > The problem would be with big integers (over 2^54), as they are rounded. I > don't known if there is some way to represent them in JS, though. Ah yes, this is a problem that is going to bite us badly later I fear =( It's not new to your patch series though. > Double and float stringification should be unique enough, It won't be unique is the point - if we receive a GVariant that has closely spaced large 64 bit integers, we'll lose some of the values. But again, this is not your fault and not a reason to block this patch series.
(In reply to comment #31) > > Double and float stringification should be unique enough, > > It won't be unique is the point - if we receive a GVariant that has closely > spaced large 64 bit integers, we'll lose some of the values. > > But again, this is not your fault and not a reason to block this patch series. No, I meant real 'd' variants. Otherwise, we would need GLib.HashTable as a boxed, with explicit lookup() and insert().
Attachment 189019 [details] pushed as 6dca0dd - Support marshalling arguments that are GVariants Attachment 189024 [details] pushed as 522b15e - GVariant: add JS wrappers for packing and unpacking
I think this can be closed now.