GNOME Bugzilla – Bug 665432
Make the ParamSpec override useful
Last modified: 2012-03-01 18:22:35 UTC
Currently the ParamSpec override is broken, in the wrong place, and is of no use. Let's fix those issues, shall we?
Created attachment 202650 [details] [review] param: It's GObject.ParamSpec, not GLib.ParamSpec
Created attachment 202651 [details] [review] param: Rip out JSResolve op This was preventing proper property access, and it's not doing anything.
Created attachment 202652 [details] [review] param: Implement a lot more properties for ParamSpecs
Created attachment 202653 [details] [review] param: Allow creating new ParamSpecs This way, users can start to add their own properties
Created attachment 202654 [details] [review] arg: Wrap GParamSpec values when sending them to JS
Created attachment 202655 [details] [review] Add a new "GObject" override module to aid us in creating ParamSpecs
Created attachment 202658 [details] [review] Add a new "GObject" override module to aid us in creating ParamSpecs Add a lot more param specs
So the reason we can't just use introspection for GParamSpec, even though it's now boxed, is because of the non-GObject inheritance from e.g. GParamSpecChar to GParamSpec? I wonder though if we could dump the native override, and just monkey patch in e.g. .get_name() on each of the subclasses from JS?
GParamSpec int for example has public fields for minimum, maximum, and default that are already exposed in the .gir, and gjs should be able to read correctly. So we may be able to get away with not doing all of that manually.
(In reply to comment #8) > So the reason we can't just use introspection for GParamSpec, even though it's > now boxed, is because of the non-GObject inheritance from e.g. GParamSpecChar > to GParamSpec? > > I wonder though if we could dump the native override, and just monkey patch in > e.g. .get_name() on each of the subclasses from JS? Actually, GObject.ParamSpec is a fundamental class, not a boxed. So, on the one hand you get inheritance in the same way as GObject, on the other hand, you need to manually specify dup/free functions. We could extend object.c for this (and GstMiniObject)
Created attachment 202662 [details] [review] param: Implement property lookup for ParamSpecs Is this more like what you wanted, Colin?
(In reply to comment #10) > Actually, GObject.ParamSpec is a fundamental class, not a boxed. So, on the one > hand you get inheritance in the same way as GObject, on the other hand, you > need to manually specify dup/free functions. We could extend object.c for this > (and GstMiniObject) We can still take advantage of the fact that it has all the properties in the .gir and it's a GTypeInstance subclass.
Review of attachment 202653 [details] [review]: ::: gi/param.c @@ +266,3 @@ + nick = gjs_string_get_ascii(cx, argv[2]); + blurb = gjs_string_get_ascii(cx, argv[3]); + flags = JSVAL_TO_INT(argv[4]); gjs_parse_args() is a nicer way to do most of the above. @@ +268,3 @@ + flags = JSVAL_TO_INT(argv[4]); + + argv += 5; Oh except I guess it won't give you the remaining args. Oh well. @@ +315,3 @@ + minimum = JSVAL_TO_INT(argv[0]); + maximum = JSVAL_TO_INT(argv[1]); + default_value = JSVAL_TO_INT(argv[2]); So this is basically going to fail for INT64 values outside of JSVAL_INT_MAX (currently 0x7fffffff, or 2147483647). You should probably use JS_ValueToNumber and accept the loss of precision when handling large 64 bit values. (We still don't have a good story for int64 handling in gjs)
This whole series is very noticeably lacking any tests. Other than that, and the above comment, they look OK to me.
While making some tests, I hit a few snags: * The fields for nick and blurb are called "_nick" and "_blurb", so it may be a little unexpected that they're private members. * While making tests for enum and flag types, I wanted to expose the GType of an enum in enumeration.c so we could refer to it. Problem: g_registered_type_info_get_gtype for a GIEnumInfo has been spitting out bogus values: GType gtype; gtype = g_registered_type_info_get_g_type((GIRegisteredTypeInfo*)info); g_print("%s = %d\n", g_base_info_get_name((GIBaseInfo*)info), gtype); in gjs_define_enumeration gives: ParamFlags = 4 Something completely unusable.
(In reply to comment #15) > While making some tests, I hit a few snags: > > * The fields for nick and blurb are called "_nick" and "_blurb", so it may be a > little unexpected that they're private members. That they're "public" you mean? Note the header has /*< private >*/ gchar *_nick; gchar *_blurb; And there's an old bug about making g-ir-scanner respect that I think. > * While making tests for enum and flag types, I wanted to expose the GType of > an enum in enumeration.c so we could refer to it. Problem: > g_registered_type_info_get_gtype for a GIEnumInfo has been spitting out bogus > values: > > GType gtype; > gtype = g_registered_type_info_get_g_type((GIRegisteredTypeInfo*)info); > g_print("%s = %d\n", g_base_info_get_name((GIBaseInfo*)info), gtype); > > in gjs_define_enumeration gives: > > ParamFlags = 4 Note GParamFlags isn't a registered type, and 4 is G_TYPE_NONE. You need to handle that. It's pretty lame we return G_TYPE_NONE instead of G_TYPE_INVALID in this case, but oh well.
Created attachment 203000 [details] [review] param: It's GObject.ParamSpec, not GLib.ParamSpec
Created attachment 203001 [details] [review] param: Rip out JSResolve op This was preventing proper property access, and it's not doing anything.
Created attachment 203002 [details] [review] param: Implement property lookup for ParamSpecs
Created attachment 203003 [details] [review] Add a common way to grab a GType for an object This is only hooked up for objects, enums and flags right now, but it's absolutely possible that more will be hooked up in the future.
Created attachment 203004 [details] [review] param: Allow creating new ParamSpecs This way, users can start to add their own properties
Created attachment 203005 [details] [review] arg: Wrap GParamSpec values when sending them to JS
Created attachment 203006 [details] [review] Add a new "GObject" override module to aid us in creating ParamSpecs
Review of attachment 203003 [details] [review]: Is there precedence for $ as a prefix for magic identifiers? I dunno. I think I'd prefer a set of global magic functions over magic properties on individual objects. For example, imports.GObject.gtype_from_js_instance(somevalue).
Review of attachment 203004 [details] [review]: ::: gi/param.c @@ +250,3 @@ + minimum = JSVAL_TO_INT(argv[0]); + maximum = JSVAL_TO_INT(argv[1]); + default_value = JSVAL_TO_INT(argv[2]); So you just decided to keep the JSVAL_TO_INT stuff here? It seems like it's at least worth a comment as to why trying to define G_TYPE_INT64 property maximums will probably blow up. Why didn't you change to at least coerce double values to their closest integer? That might be enough for more cases. There's a gjs_value_to_int64() you can use. @@ +306,3 @@ + + if (!JSVAL_IS_INT(argv[1])) + goto out; I'm also concerned about the enum case. Or really, in general how this function duplicates a lot of code from arg.c. Note all the special handling of enums there. ::: test/js/testParamSpec.js @@ +14,3 @@ +let flags = GObject.ParamFlags.READABLE; + +function testStringParamSpec() { Do these tests pass without the further two patches? If not, I'd prefer to add the tests at the end to preserve bisectability.
Created attachment 203112 [details] [review] param: Allow creating new ParamSpecs OK, this should hopefully be what you're looking for. I made everything an 64-bit integer.
Created attachment 203113 [details] [review] test: Add new tests to test ParamSpecs And attached the tests in a separate commit at the end.
(In reply to comment #25) > Review of attachment 203003 [details] [review]: > > Is there precedence for $ as a prefix for magic identifiers? I dunno. I think > I'd prefer a set of global magic functions over magic properties on individual > objects. > > For example, imports.GObject.gtype_from_js_instance(somevalue). I'm not a fan of that. To make this work for enums, we'd need to have some magic "$gtype" variable defined on it anyway, or make an enumeration its own class. The function would need a giant switch statement depending on the type of the object it is, or we would need some sort of assertion like "the first 64 bits of an object's private member is the GType". If we want to name it __gtype__ instead, we're fine. In previous versions of the ECMAScript specification, there was a statement that properties starting with "$" were for internal use only. This statement has been removed from recent versions of ECMAScript.
(In reply to comment #29) > I'm not a fan of that. To make this work for enums, we'd need to have some > magic "$gtype" variable defined on it anyway, or make an enumeration its own > class. The function would need a giant switch statement depending on the type > of the object it is, or we would need some sort of assertion like "the first 64 > bits of an object's private member is the GType". Right but let's distinguish between the ugliness of implementation versus ugliness for the JS programmer. I'm mostly concerned about the second right now - the implementation can always change later. I think at the very least it should be a non-enumerable property; otherwise e.g. if you do (for v in Gio.FileCreateFlags) { print v } (for hand-rolled inspection) you see it show up. As far as implementation, the "giant" switch statement would be around 5-6 things, right? > If we want to name it __gtype__ instead, we're fine. In previous versions of > the ECMAScript specification, there was a statement that properties starting > with "$" were for internal use only. This statement has been removed from > recent versions of ECMAScript. I'm not really a big fan of Python's __ fetish either honestly. But the main question to me is more about properties on every object versus global functions. If they're used rarely in special circumstances, doing the latter keeps them much more hidden. A good example is a "System.addressOf" module I had written which gave you the native C address for a proxied GObject. I guess in your model that'd be $address or something. Which isn't bad, just very different aesthetically. Anyways feel free to go ahead with this if you prefer $gtype (though do consider making it non-enumerable).
Review of attachment 203113 [details] [review]: A test for a 64 bit integer value would probably help us catch any possible regressions. Other than that, this all looks OK to me to push.
Attachment 203000 [details] pushed as d188677 - param: It's GObject.ParamSpec, not GLib.ParamSpec Attachment 203001 [details] pushed as a78bfb2 - param: Rip out JSResolve op Attachment 203002 [details] pushed as 378ff47 - param: Implement property lookup for ParamSpecs Attachment 203003 [details] pushed as ebc8449 - Add a common way to grab a GType for an object Attachment 203005 [details] pushed as c90ecef - arg: Wrap GParamSpec values when sending them to JS Attachment 203006 [details] pushed as 296360b - Add a new "GObject" override module to aid us in creating ParamSpecs Pushed! Thanks!
(In reply to comment #10) > (In reply to comment #8) > > So the reason we can't just use introspection for GParamSpec, even though it's > > now boxed, is because of the non-GObject inheritance from e.g. GParamSpecChar > > to GParamSpec? > > > > I wonder though if we could dump the native override, and just monkey patch in > > e.g. .get_name() on each of the subclasses from JS? > > Actually, GObject.ParamSpec is a fundamental class, not a boxed. So, on the one > hand you get inheritance in the same way as GObject, on the other hand, you > need to manually specify dup/free functions. We could extend object.c for this > (and GstMiniObject) That is what I tried in bug #656440, but see comment #15 and #16 there
Well, maybe the current implementation of GParamSpec is not the best possible, but it's there and does what's supposed to do, so how about fixed?