GNOME Bugzilla – Bug 751343
Should be able to define new interfaces in JS
Last modified: 2015-07-13 21:34:38 UTC
Currently, you can define GObject classes in GJS that implement GObject interfaces that have been defined in C. But you cannot define new interfaces yourself in GJS. This set of four patches (which my coworker Roberto Goizueta also collaborated on) adds this functionality. The first patch adds Lang.Interface which is the counterpart of Lang.Class for interfaces. It becomes possible to define pure JS interfaces without using GObject, analogously to Lang.Class. This part of the patch is loosely based on how MooTools does interfaces. [*] Interfaces can "Require" other interfaces or classes. The second patch extends Lang.Interface to cover defining new GObject interfaces in C. When you create a new Lang.Interface that "Require"s a GObject class or interface, it will be backed by a GObject interface known to the GType system. GObject interfaces support "Signals" and "Properties" just as GObject classes do. The third patch introduces a Lang.Class.implements() method. We chose specifically not to do this with "instanceof" since it's not clear what the interaction between "instanceof" and ES6 classes/interfaces is supposed to be. The fourth patch introduces a GObject.ParamSpec.override() which makes it more convenient to override properties on interfaces. I've discussed this previously on #727368 and though I would prefer to automatically override interface properties on classes that implement the interface, I think it was decided on #727368 that GObject.ParamSpec.override() was the way to go. For convenience you can see the code all together on this GitHub branch: https://github.com/endlessm/gjs/tree/wip/ptomato/interfaces [*] The main difference is that MooTools interfaces can't contain any code. Since GObject interfaces do allow default implementations of functions that are part of the interface, it makes sense to allow this in our GJS interfaces as well.
Created attachment 305855 [details] [review] lang: Pure Javascript Lang.Interface This introduces Lang.Interface, a counterpart to Lang.Class that allows Lang.Class-created objects to implement interfaces. The interface's properties will be copied to the implementing object, so that you can have default implementations of methods. Any properties with the value Lang.Interface.UNIMPLEMENTED must be overridden by the implementing class. You will get an exception if you try to define a class that doesn't override one of these. An interface can require other interfaces or classes, just as with GObject interfaces. If an interface requires a class, it means that a class implementing that interface must inherit from the required class. GObject.Object-derived classes can also implement Lang.Interfaces. This commit does not yet let you define new GObject-style interfaces with properties and signals and a GType. The Lang.Interface._check() mechanism is loosely based on a MooTools plugin: https://github.com/sebwoh/Mootools-Class-Interfaces/ Implementation notes / Discussion points ======================================== - An alternative would be to make Lang.Interface a metaclass. (See installed-tests/js/testMetaclass.js for an example of metaclasses.) This patch assumes that we don't want Lang.Interface to inherit from Lang.Class but instead only have _Base as a shared parent. The metaclass approach would mean that you would create a new interface as a Lang.Class that extends Lang.Interface. - There is no this.parent() equivalent for interfaces. That's on purpose because there is also no such thing in C GObject; unlike parent classes where you get a convenient my_object_parent_class pointer to the class struct, you have to explicitly get the interface's vtable if you want to chain up. That would additionally require us to define a method resolution order, which is a bit of an alien concept in Javascript. - Interfaces purposely don't have an _init function. This is because GObject interfaces don't have an instance init function either, because they can't have any private data. However, I can see it being useful in Javascript, since your `this` is the implementing object, and you can stick any properties you want on there already in your interface methods. However, same concern about method resolution order applies. (Collaboration by Philip Chimento <philip@endlessm.com> and Roberto Goizueta <goizueta@endlessm.com>)
Created attachment 305856 [details] [review] gobject: GObject.Interface This adds the ability to define new interfaces in GJS that are known to GObject and that have signals and properties. Objects implementing a GObject.Interface get the interface's signals automatically. They must, however, reimplement the interface's properties or an error will occur. This is just as in C, except there we conveniently have g_param_spec_override(). In a later commit we will add a GObject.ParamSpec.override() to make this less painful in GJS. This also refactors some code that is now common to both classes and interfaces into reusable functions, both on the C and Javascript side of things. Where possible without too much churn, it uses newer SpiderMonkey APIs in code that it touches. (Collaboration by Philip Chimento <philip@endlessm.com> and Roberto Goizueta <goizueta@endlessm.com>)
Created attachment 305859 [details] [review] lang: Lang.Class.implements() This function does the work of g_type_is_a() and G_IS_...() for interfaces. It allows us to make better assertions in our tests and fills a need in GJS since there is no such thing as the instanceof operator for interfaces. Note. implements is a reserved word in ES6 but reserved words are allowed as property names, and we use reserved words elsewhere in GJS as property names as well. (Collaboration by Philip Chimento <philip@endlessm.com> and Roberto Goizueta <goizueta@endlessm.com>)
Created attachment 305860 [details] [review] gobject: GObject.ParamSpec.override() This adds a GObject.ParamSpec.override() which wraps g_param_spec_override(), in order to more conveniently implement properties that are present on interfaces. (Collaboration by Philip Chimento <philip@endlessm.com> and Roberto Goizueta <goizueta@endlessm.com>)
Review of attachment 305855 [details] [review]: ::: installed-tests/js/testGObjectInterface.js @@ +34,3 @@ + } + }); + new MyHybridObject(); I note that these tests aren't asserting anything in this patchset ... at least not until the next patch. Do they need to be removed? Or should there be a JSUnit.assertDoesntRaise or equavilent? ::: installed-tests/js/testInterface.js @@ +143,3 @@ + JSUnit.assertEquals('MyInterface.some_prop getter', obj.some_prop); + obj.some_prop = 'foobar'; + JSUnit.assertTrue(obj.some_prop_setter_called); Split into two separate tests @@ +175,3 @@ + +function testInterfaceCanRequireOtherInterface() { + new MyOtherObject(); At the risk of more verbosity, I think this would be a lot more readable and immediately understandable if the relevant class definition was copied or moved inline. This is probably a bit of a pattern in the other tests as well.
Review of attachment 305856 [details] [review]: ::: gi/interface.cpp @@ +227,3 @@ + if (priv->info) + gjs_define_static_methods(context, constructor, priv->gtype, priv->info); Probably add a comment here about how gjs-defined interfaces have no static methods too. ::: gi/object.cpp @@ +2614,3 @@ + + if (!class_init_properties) + GType *iface_types) Obviously, because this is a refactoring of existing code, its not worth blocking this patchset on this, but I still think that passing default properties around implicitly by hashtable isn't exactly safe nor maintainable. I suspect we can pack that information into the GTypeInfo's class_data. Perhaps this is something to follow up on. @@ +2705,3 @@ + &type_info, + (GTypeFlags) 0); + } Possibly also name = NULL or g_clear_pointer just to be defensive @@ +2709,3 @@ + g_type_set_qdata(interface_type, gjs_is_custom_type_quark(), GINT_TO_POINTER(1)); + + g_clear_pointer(&properties_native, g_ptr_array_unref); Should this be rolled back when it fails? I suspect that if there's no rollback we'll leak properties here. We also leak a type, since we've already registered the type. ::: modules/overrides/GObject.js @@ +62,3 @@ + } + return propertiesArray; +function _createGTypeName(name, gtypename) { Looks like it was a good idea to refactor this out. Does it belong in a separate commit or can we keep it here?
Review of attachment 305860 [details] [review]: ::: gi/object.cpp @@ +2451,3 @@ + GTypeInterface *interface_type; + + gchar *name = NULL; Technically not needed since we're being called from JS
Review of attachment 305859 [details] [review]: Looks good so far. I did initially have a fleeting concern about interfaces implemented higher up the inheritance tree, but it looks like that's all dealt with in testSubclassImplementsTheSameInterfaceAsItsParent
Ack, I had these review comments written up yesterday and hadn't hit the publish button it seems. Looks really good so far. Most of the implementation review for me was already covered downstream, so the above comments are really just small things which probably don't matter too much. I'm still not too keen on passing initial class properties around via a global hashtable, though I understand that this is effectively inherited from the old approach, so it probably warrants a follow-up patchset after this one.
Thanks for the review, Sam. I fixed some things right away, and below are my clarifying comments and/or questions. (In reply to Sam Spilsbury from comment #5) > Review of attachment 305855 [details] [review] [review]: > > ::: installed-tests/js/testGObjectInterface.js > @@ +34,3 @@ > + } > + }); > + new MyHybridObject(); > > I note that these tests aren't asserting anything in this patchset ... at > least not until the next patch. Do they need to be removed? Or should there > be a JSUnit.assertDoesntRaise or equavilent? There isn't an equivalent; the test will fail if an exception is thrown, though. So I think the tests are still valuable without assertions. > @@ +175,3 @@ > + > +function testInterfaceCanRequireOtherInterface() { > + new MyOtherObject(); > > At the risk of more verbosity, I think this would be a lot more readable and > immediately understandable if the relevant class definition was copied or > moved inline. This is probably a bit of a pattern in the other tests as well. I don't necessarily agree with that, since replicating the class definition would mean a lot of repeated boilerplate code, which on the whole makes things less readable in my opinion. Note that in the cases where a class is only used once, then its definition is inline. Perhaps some well-placed comments could solve your concern? For example, "// MyOtherObject requires MyOtherInterface in addition to MyInterface" (In reply to Sam Spilsbury from comment #6) > ::: gi/object.cpp > @@ +2614,3 @@ > + > + if (!class_init_properties) > + GType *iface_types) > > Obviously, because this is a refactoring of existing code, its not worth > blocking this patchset on this, but I still think that passing default > properties around implicitly by hashtable isn't exactly safe nor > maintainable. I suspect we can pack that information into the GTypeInfo's > class_data. > > Perhaps this is something to follow up on. Well, it's not great, but I haven't really thought about other ways to do it. I definitely don't want to change it as part of this patchset. (If it ain't broke...) In theory it's not that unsafe. I could be talking nonsense, but I think the hashtable will only ever be accessed by the main thread, and GType ensures that each type's class_init / interface_init is called only once. > @@ +2709,3 @@ > + g_type_set_qdata(interface_type, gjs_is_custom_type_quark(), > GINT_TO_POINTER(1)); > + > + g_clear_pointer(&properties_native, g_ptr_array_unref); > > Should this be rolled back when it fails? I suspect that if there's no > rollback we'll leak properties here. We also leak a type, since we've > already registered the type. I think unrefing the properties is taken care of by supplying a destroy func to the hashtable ('g_ptr_array_new_with_free_func((GDestroyNotify) g_param_spec_unref)' on line 2621). Unfortunately there doesn't seem to be a way to roll back the registration of a type, and the comment in gjs_register_type says as much. > ::: modules/overrides/GObject.js > @@ +62,3 @@ > + } > + return propertiesArray; > +function _createGTypeName(name, gtypename) { > > Looks like it was a good idea to refactor this out. Does it belong in a > separate commit or can we keep it here? I've been trying to keep all refactors of duplicate code in the same commit as where it became necessary to duplicate the code. That is, I don't think it would have made sense to refactor this out if it were used only in one place. (In reply to Sam Spilsbury from comment #7) > Review of attachment 305860 [details] [review] [review]: > > ::: gi/object.cpp > @@ +2451,3 @@ > + GTypeInterface *interface_type; > + > + gchar *name = NULL; > > Technically not needed since we're being called from JS I don't understand why? If gjs_parse_call_args() fails before filling in &name, then it can still be uninitialized.
Created attachment 306356 [details] [review] lang: Pure Javascript Lang.Interface This introduces Lang.Interface, a counterpart to Lang.Class that allows Lang.Class-created objects to implement interfaces. The interface's properties will be copied to the implementing object, so that you can have default implementations of methods. Any properties with the value Lang.Interface.UNIMPLEMENTED must be overridden by the implementing class. You will get an exception if you try to define a class that doesn't override one of these. An interface can require other interfaces or classes, just as with GObject interfaces. If an interface requires a class, it means that a class implementing that interface must inherit from the required class. GObject.Object-derived classes can also implement Lang.Interfaces. This commit does not yet let you define new GObject-style interfaces with properties and signals and a GType. The Lang.Interface._check() mechanism is loosely based on a MooTools plugin: https://github.com/sebwoh/Mootools-Class-Interfaces/ Implementation notes / Discussion points ======================================== - An alternative would be to make Lang.Interface a metaclass. (See installed-tests/js/testMetaclass.js for an example of metaclasses.) This patch assumes that we don't want Lang.Interface to inherit from Lang.Class but instead only have _Base as a shared parent. The metaclass approach would mean that you would create a new interface as a Lang.Class that extends Lang.Interface. - There is no this.parent() equivalent for interfaces. That's on purpose because there is also no such thing in C GObject; unlike parent classes where you get a convenient my_object_parent_class pointer to the class struct, you have to explicitly get the interface's vtable if you want to chain up. That would additionally require us to define a method resolution order, which is a bit of an alien concept in Javascript. - Interfaces purposely don't have an _init function. This is because GObject interfaces don't have an instance init function either, because they can't have any private data. However, I can see it being useful in Javascript, since your `this` is the implementing object, and you can stick any properties you want on there already in your interface methods. However, same concern about method resolution order applies. (Collaboration by Philip Chimento <philip@endlessm.com> and Roberto Goizueta <goizueta@endlessm.com>)
Created attachment 306357 [details] [review] gobject: GObject.Interface This adds the ability to define new interfaces in GJS that are known to GObject and that have signals and properties. Objects implementing a GObject.Interface get the interface's signals automatically. They must, however, reimplement the interface's properties or an error will occur. This is just as in C, except there we conveniently have g_param_spec_override(). In a later commit we will add a GObject.ParamSpec.override() to make this less painful in GJS. This also refactors some code that is now common to both classes and interfaces into reusable functions, both on the C and Javascript side of things. Where possible without too much churn, it uses newer SpiderMonkey APIs in code that it touches. (Collaboration by Philip Chimento <philip@endlessm.com> and Roberto Goizueta <goizueta@endlessm.com>)
Created attachment 306358 [details] [review] gobject: GObject.ParamSpec.override() This adds a GObject.ParamSpec.override() which wraps g_param_spec_override(), in order to more conveniently implement properties that are present on interfaces. (Collaboration by Philip Chimento <philip@endlessm.com> and Roberto Goizueta <goizueta@endlessm.com>)
(In reply to Philip Chimento from comment #10) > Thanks for the review, Sam. I fixed some things right away, and below are my > clarifying comments and/or questions. > > (In reply to Sam Spilsbury from comment #5) > > Review of attachment 305855 [details] [review] [review] [review]: > > > > ::: installed-tests/js/testGObjectInterface.js > > @@ +34,3 @@ > > + } > > + }); > > + new MyHybridObject(); > > > > I note that these tests aren't asserting anything in this patchset ... at > > least not until the next patch. Do they need to be removed? Or should there > > be a JSUnit.assertDoesntRaise or equavilent? > > There isn't an equivalent; the test will fail if an exception is thrown, > though. So I think the tests are still valuable without assertions. > > > @@ +175,3 @@ > > + > > +function testInterfaceCanRequireOtherInterface() { > > + new MyOtherObject(); > > > > At the risk of more verbosity, I think this would be a lot more readable and > > immediately understandable if the relevant class definition was copied or > > moved inline. This is probably a bit of a pattern in the other tests as well. > > I don't necessarily agree with that, since replicating the class definition > would mean a lot of repeated boilerplate code, which on the whole makes > things less readable in my opinion. Note that in the cases where a class is > only used once, then its definition is inline. Perhaps some well-placed > comments could solve your concern? For example, "// MyOtherObject requires > MyOtherInterface in addition to MyInterface" > > (In reply to Sam Spilsbury from comment #6) > > ::: gi/object.cpp > > @@ +2614,3 @@ > > + > > + if (!class_init_properties) > > + GType *iface_types) > > > > Obviously, because this is a refactoring of existing code, its not worth > > blocking this patchset on this, but I still think that passing default > > properties around implicitly by hashtable isn't exactly safe nor > > maintainable. I suspect we can pack that information into the GTypeInfo's > > class_data. > > > > Perhaps this is something to follow up on. > > Well, it's not great, but I haven't really thought about other ways to do > it. I definitely don't want to change it as part of this patchset. (If it > ain't broke...) > > In theory it's not that unsafe. I could be talking nonsense, but I think the > hashtable will only ever be accessed by the main thread, and GType ensures > that each type's class_init / interface_init is called only once. > > > @@ +2709,3 @@ > > + g_type_set_qdata(interface_type, gjs_is_custom_type_quark(), > > GINT_TO_POINTER(1)); > > + > > + g_clear_pointer(&properties_native, g_ptr_array_unref); > > > > Should this be rolled back when it fails? I suspect that if there's no > > rollback we'll leak properties here. We also leak a type, since we've > > already registered the type. > > I think unrefing the properties is taken care of by supplying a destroy func > to the hashtable ('g_ptr_array_new_with_free_func((GDestroyNotify) > g_param_spec_unref)' on line 2621). Unfortunately there doesn't seem to be a > way to roll back the registration of a type, and the comment in > gjs_register_type says as much. > > > ::: modules/overrides/GObject.js > > @@ +62,3 @@ > > + } > > + return propertiesArray; > > +function _createGTypeName(name, gtypename) { > > > > Looks like it was a good idea to refactor this out. Does it belong in a > > separate commit or can we keep it here? > > I've been trying to keep all refactors of duplicate code in the same commit > as where it became necessary to duplicate the code. That is, I don't think > it would have made sense to refactor this out if it were used only in one > place. > > (In reply to Sam Spilsbury from comment #7) > > Review of attachment 305860 [details] [review] [review] [review]: > > > > ::: gi/object.cpp > > @@ +2451,3 @@ > > + GTypeInterface *interface_type; > > + > > + gchar *name = NULL; > > > > Technically not needed since we're being called from JS > > I don't understand why? If gjs_parse_call_args() fails before filling in > &name, then it can still be uninitialized. Ah - seems like the old bugzilla-putting-review-comments-in-the-wrong-place again. Click on the "review" link for the patch and have a read of that (Cosimo showed that to me the other day). In case the patch was rebased and re-uploaded - my comment was about JSAutoRequest. Jasper told me the other day that its not needed when being called *from* JS, only when calling *into* JS. Its harmless either way though.
(In reply to Philip Chimento from comment #10) > Thanks for the review, Sam. I fixed some things right away, and below are my > clarifying comments and/or questions. > > (In reply to Sam Spilsbury from comment #5) > > Review of attachment 305855 [details] [review] [review] [review]: > > > > ::: installed-tests/js/testGObjectInterface.js > > @@ +34,3 @@ > > + } > > + }); > > + new MyHybridObject(); > > > > I note that these tests aren't asserting anything in this patchset ... at > > least not until the next patch. Do they need to be removed? Or should there > > be a JSUnit.assertDoesntRaise or equavilent? > > There isn't an equivalent; the test will fail if an exception is thrown, > though. So I think the tests are still valuable without assertions. Hmm okay. I wonder if we can get gjs-jasmine in upstream GJS ;-) Might be good to leave a comment to make that clear that the point is to check if the object can be constructed without throwing. Either encode that in the test name or put it in a comment. > > > @@ +175,3 @@ > > + > > +function testInterfaceCanRequireOtherInterface() { > > + new MyOtherObject(); > > > > At the risk of more verbosity, I think this would be a lot more readable and > > immediately understandable if the relevant class definition was copied or > > moved inline. This is probably a bit of a pattern in the other tests as well. > > I don't necessarily agree with that, since replicating the class definition > would mean a lot of repeated boilerplate code, which on the whole makes > things less readable in my opinion. Note that in the cases where a class is > only used once, then its definition is inline. Perhaps some well-placed > comments could solve your concern? For example, "// MyOtherObject requires > MyOtherInterface in addition to MyInterface" So there are three other possible solutions: 1. Rename the classes and interfaces to make it clear what their purpose is (e.g. MyOtherObject -> ObjectRequiringGObjectInterfaces) 2. Create some functions with descriptive names that return the object in questionn, e.g. function objectRequiringGObjectInterfaces() { return MyObject(); } 3. Comment to indicate what that object actually is inline. > > (In reply to Sam Spilsbury from comment #6) > > ::: gi/object.cpp > > @@ +2614,3 @@ > > + > > + if (!class_init_properties) > > + GType *iface_types) > > > > Obviously, because this is a refactoring of existing code, its not worth > > blocking this patchset on this, but I still think that passing default > > properties around implicitly by hashtable isn't exactly safe nor > > maintainable. I suspect we can pack that information into the GTypeInfo's > > class_data. > > > > Perhaps this is something to follow up on. > > Well, it's not great, but I haven't really thought about other ways to do > it. I definitely don't want to change it as part of this patchset. (If it > ain't broke...) > > In theory it's not that unsafe. I could be talking nonsense, but I think the > hashtable will only ever be accessed by the main thread, and GType ensures > that each type's class_init / interface_init is called only once. In theory, yes. Coarsely provable too, if assumptions about GObject always hold true. From a maintainability perspective I still think it exposes too much to the rest of the module. As I mentioned, its just a note on something to follow up on - maybe I can write a patch after we've merged this in. > > > @@ +2709,3 @@ > > + g_type_set_qdata(interface_type, gjs_is_custom_type_quark(), > > GINT_TO_POINTER(1)); > > + > > + g_clear_pointer(&properties_native, g_ptr_array_unref); > > > > Should this be rolled back when it fails? I suspect that if there's no > > rollback we'll leak properties here. We also leak a type, since we've > > already registered the type. > > I think unrefing the properties is taken care of by supplying a destroy func > to the hashtable ('g_ptr_array_new_with_free_func((GDestroyNotify) > g_param_spec_unref)' on line 2621). Unfortunately there doesn't seem to be a > way to roll back the registration of a type, and the comment in > gjs_register_type says as much. Ah, looks like bugzilla put my comments in the wrong place again. The comment was to do with calling save_properties_for_class_init and returning immediately when it fails. I also just found out now that you can still access the review comments - just show obsolete patches and they'll still be there. > > > ::: modules/overrides/GObject.js > > @@ +62,3 @@ > > + } > > + return propertiesArray; > > +function _createGTypeName(name, gtypename) { > > > > Looks like it was a good idea to refactor this out. Does it belong in a > > separate commit or can we keep it here? > > I've been trying to keep all refactors of duplicate code in the same commit > as where it became necessary to duplicate the code. That is, I don't think > it would have made sense to refactor this out if it were used only in one > place. Ok.
Created attachment 306432 [details] [review] lang: Pure Javascript Lang.Interface This introduces Lang.Interface, a counterpart to Lang.Class that allows Lang.Class-created objects to implement interfaces. The interface's properties will be copied to the implementing object, so that you can have default implementations of methods. Any properties with the value Lang.Interface.UNIMPLEMENTED must be overridden by the implementing class. You will get an exception if you try to define a class that doesn't override one of these. An interface can require other interfaces or classes, just as with GObject interfaces. If an interface requires a class, it means that a class implementing that interface must inherit from the required class. GObject.Object-derived classes can also implement Lang.Interfaces. This commit does not yet let you define new GObject-style interfaces with properties and signals and a GType. The Lang.Interface._check() mechanism is loosely based on a MooTools plugin: https://github.com/sebwoh/Mootools-Class-Interfaces/ Implementation notes / Discussion points ======================================== - An alternative would be to make Lang.Interface a metaclass. (See installed-tests/js/testMetaclass.js for an example of metaclasses.) This patch assumes that we don't want Lang.Interface to inherit from Lang.Class but instead only have _Base as a shared parent. The metaclass approach would mean that you would create a new interface as a Lang.Class that extends Lang.Interface. - There is no this.parent() equivalent for interfaces. That's on purpose because there is also no such thing in C GObject; unlike parent classes where you get a convenient my_object_parent_class pointer to the class struct, you have to explicitly get the interface's vtable if you want to chain up. That would additionally require us to define a method resolution order, which is a bit of an alien concept in Javascript. - Interfaces purposely don't have an _init function. This is because GObject interfaces don't have an instance init function either, because they can't have any private data. However, I can see it being useful in Javascript, since your `this` is the implementing object, and you can stick any properties you want on there already in your interface methods. However, same concern about method resolution order applies. (Collaboration by Philip Chimento <philip@endlessm.com> and Roberto Goizueta <goizueta@endlessm.com>)
Created attachment 306433 [details] [review] gobject: GObject.Interface This adds the ability to define new interfaces in GJS that are known to GObject and that have signals and properties. Objects implementing a GObject.Interface get the interface's signals automatically. They must, however, reimplement the interface's properties or an error will occur. This is just as in C, except there we conveniently have g_param_spec_override(). In a later commit we will add a GObject.ParamSpec.override() to make this less painful in GJS. This also refactors some code that is now common to both classes and interfaces into reusable functions, both on the C and Javascript side of things. Where possible without too much churn, it uses newer SpiderMonkey APIs in code that it touches. (Collaboration by Philip Chimento <philip@endlessm.com> and Roberto Goizueta <goizueta@endlessm.com>)
Created attachment 306434 [details] [review] lang: Lang.Class.implements() This function does the work of g_type_is_a() and G_IS_...() for interfaces. It allows us to make better assertions in our tests and fills a need in GJS since there is no such thing as the instanceof operator for interfaces. Note. implements is a reserved word in ES6 but reserved words are allowed as property names, and we use reserved words elsewhere in GJS as property names as well. (Collaboration by Philip Chimento <philip@endlessm.com> and Roberto Goizueta <goizueta@endlessm.com>)
Created attachment 306435 [details] [review] gobject: GObject.ParamSpec.override() This adds a GObject.ParamSpec.override() which wraps g_param_spec_override(), in order to more conveniently implement properties that are present on interfaces. (Collaboration by Philip Chimento <philip@endlessm.com> and Roberto Goizueta <goizueta@endlessm.com>)
(In reply to Sam Spilsbury from comment #14) > > > ::: gi/object.cpp > > > @@ +2451,3 @@ > > > + GTypeInterface *interface_type; > > > + > > > + gchar *name = NULL; > > > > > > Technically not needed since we're being called from JS > > > > I don't understand why? If gjs_parse_call_args() fails before filling in > > &name, then it can still be uninitialized. > > Ah - seems like the old bugzilla-putting-review-comments-in-the-wrong-place > again. Click on the "review" link for the patch and have a read of that > (Cosimo showed that to me the other day). > > In case the patch was rebased and re-uploaded - my comment was about > JSAutoRequest. Jasper told me the other day that its not needed when being > called *from* JS, only when calling *into* JS. Its harmless either way > though. I see what you mean. I've removed it from the two functions added in this patchset that are called from JS: gjs_override_property() and gjs_register_interface(). I guess that means we could go back to using "goto fail" like I was before, but since the newer mozjs API has more RAII-like stuff than before, probably "goto fail" should be discouraged. (In reply to Sam Spilsbury from comment #15) > Hmm okay. I wonder if we can get gjs-jasmine in upstream GJS ;-) Maybe a "light" version; jasmine-gjs in its current form can't, because it requires GJS. > > > @@ +2709,3 @@ > > > + g_type_set_qdata(interface_type, gjs_is_custom_type_quark(), > > > GINT_TO_POINTER(1)); > > > + > > > + g_clear_pointer(&properties_native, g_ptr_array_unref); > > > > > > Should this be rolled back when it fails? I suspect that if there's no > > > rollback we'll leak properties here. We also leak a type, since we've > > > already registered the type. > > > > I think unrefing the properties is taken care of by supplying a destroy func > > to the hashtable ('g_ptr_array_new_with_free_func((GDestroyNotify) > > g_param_spec_unref)' on line 2621). Unfortunately there doesn't seem to be a > > way to roll back the registration of a type, and the comment in > > gjs_register_type says as much. > > Ah, looks like bugzilla put my comments in the wrong place again. The > comment was to do with calling save_properties_for_class_init and returning > immediately when it fails. I took another look, but I think what I said before still applies; the hashtable isn't modified until the very end of save_properties_for_class_init(), at which point there are no more failure points in that function. So if the function fails, as far as I can tell, the properties are not leaked, the hashtable isn't modified, and we can't help leaking the gtype. Thanks for the review once more.
Review of attachment 306432 [details] [review]: ::: installed-tests/js/testGObjectInterface.js @@ +36,3 @@ + } + }); + new ObjectImplementingLangInterfaceAndCInterface(); Excellent ::: installed-tests/js/testInterface.js @@ +228,3 @@ + Name: 'ObjectWithInterfacesInWrongOrder', + Implements: [ InterfaceRequiringOtherInterface, AnInterface ], + required: function () {} nit: any kind of convention about a whitespace between Name: , Implements: and required: ? We can follow it up later (also reminds me that I was going to write a small linter for this) ::: modules/lang.js @@ +344,3 @@ + + let unfulfilledReqs = this.prototype.__requires__.filter((required) => { + throw new TypeError('Cannot instantiate interface ' + name); I've probably missed it - but any reason for the ordering rule? @@ +348,3 @@ + let interfaces = proto.__interfaces__; + return ((!_interfacePresent(required, proto) || + So my understanding is that you can have this MyInterface = Lang.Interface({ Name: 'MyInterface', Requires: [ 'RequiredByMyInterface', 'MyInterface' ] }); but not: MyInterface = Lang.Interface({ Name: 'MyInterface', Requires: [ 'MyInterface', 'RequiredByMyInterface' ] }); Is that correct? @@ +351,3 @@ + !(proto instanceof required)); + }) + // See note in Class._construct(); this makes "newInterface instanceof .map on same line as })? Also, I'm a little confused as to why we read prototype.__name__ || required.name || required (coerced to a string). Aren't all interface-like objects going to be the same in that we can always read their name? Is there any reason for all these permutations? @@ +360,3 @@ + let unimplementedFns = Object.getOwnPropertyNames(this.prototype) + .filter((p) => this.prototype[p] === Interface.UNIMPLEMENTED) + newInterface.prototype = Object.create(Interface.prototype); Might want to clarify here that this.prototype refers to partially implemented properties/methods on this interface and proto refers to implemented properties/methods on the class implementing this interface.
Created attachment 306889 [details] [review] lang: Pure Javascript Lang.Interface This introduces Lang.Interface, a counterpart to Lang.Class that allows Lang.Class-created objects to implement interfaces. The interface's properties will be copied to the implementing object, so that you can have default implementations of methods. Any properties with the value Lang.Interface.UNIMPLEMENTED must be overridden by the implementing class. You will get an exception if you try to define a class that doesn't override one of these. An interface can require other interfaces or classes, just as with GObject interfaces. If an interface requires a class, it means that a class implementing that interface must inherit from the required class. GObject.Object-derived classes can also implement Lang.Interfaces. This commit does not yet let you define new GObject-style interfaces with properties and signals and a GType. The Lang.Interface._check() mechanism is loosely based on a MooTools plugin: https://github.com/sebwoh/Mootools-Class-Interfaces/ Implementation notes / Discussion points ======================================== - An alternative would be to make Lang.Interface a metaclass. (See installed-tests/js/testMetaclass.js for an example of metaclasses.) This patch assumes that we don't want Lang.Interface to inherit from Lang.Class but instead only have _Base as a shared parent. The metaclass approach would mean that you would create a new interface as a Lang.Class that extends Lang.Interface. - There is no this.parent() equivalent for interfaces. That's on purpose because there is also no such thing in C GObject; unlike parent classes where you get a convenient my_object_parent_class pointer to the class struct, you have to explicitly get the interface's vtable if you want to chain up. That would additionally require us to define a method resolution order, which is a bit of an alien concept in Javascript. - Interfaces purposely don't have an _init function. This is because GObject interfaces don't have an instance init function either, because they can't have any private data. However, I can see it being useful in Javascript, since your `this` is the implementing object, and you can stick any properties you want on there already in your interface methods. However, same concern about method resolution order applies. (Collaboration by Philip Chimento <philip@endlessm.com> and Roberto Goizueta <goizueta@endlessm.com>)
Created attachment 306890 [details] [review] gobject: GObject.Interface This adds the ability to define new interfaces in GJS that are known to GObject and that have signals and properties. Objects implementing a GObject.Interface get the interface's signals automatically. They must, however, reimplement the interface's properties or an error will occur. This is just as in C, except there we conveniently have g_param_spec_override(). In a later commit we will add a GObject.ParamSpec.override() to make this less painful in GJS. This also refactors some code that is now common to both classes and interfaces into reusable functions, both on the C and Javascript side of things. Where possible without too much churn, it uses newer SpiderMonkey APIs in code that it touches. (Collaboration by Philip Chimento <philip@endlessm.com> and Roberto Goizueta <goizueta@endlessm.com>)
Created attachment 306891 [details] [review] lang: Lang.Class.implements() This function does the work of g_type_is_a() and G_IS_...() for interfaces. It allows us to make better assertions in our tests and fills a need in GJS since there is no such thing as the instanceof operator for interfaces. Note. implements is a reserved word in ES6 but reserved words are allowed as property names, and we use reserved words elsewhere in GJS as property names as well. (Collaboration by Philip Chimento <philip@endlessm.com> and Roberto Goizueta <goizueta@endlessm.com>)
Created attachment 306892 [details] [review] gobject: GObject.ParamSpec.override() This adds a GObject.ParamSpec.override() which wraps g_param_spec_override(), in order to more conveniently implement properties that are present on interfaces. (Collaboration by Philip Chimento <philip@endlessm.com> and Roberto Goizueta <goizueta@endlessm.com>)
(In reply to Sam Spilsbury from comment #22) > Review of attachment 306432 [details] [review] [review]: Thanks for the review once more. I added some clarifying comments in this new patch. I hope to have answered the rest of your questions below: > ::: installed-tests/js/testInterface.js > @@ +228,3 @@ > + Name: 'ObjectWithInterfacesInWrongOrder', > + Implements: [ InterfaceRequiringOtherInterface, AnInterface ], > + required: function () {} > > nit: any kind of convention about a whitespace between Name: , Implements: > and required: ? We can follow it up later (also reminds me that I was going > to write a small linter for this) With my immediate coworkers at Endless internally, we've settled on the convention of one stanza of Name:, Extends:, Implements:, Requires:, then a blank line, then a stanza of Properties: and Signals:, then a blank line, then a stanza of Template:, Children:, InternalChildren:, then a blank line, then methods each separated by a blank line. However we don't consider this a hard rule, since if you only have Name:, Extends:, and Template:, for example, then it looks silly to have a blank line in the middle there. In these tests I mostly omitted the blank lines in favour of compactness. > ::: modules/lang.js > I've probably missed it - but any reason for the ordering rule? There is a reason - it's because if interface B requires another interface A, then B may override functions from A. In that case you don't want B to copy its properties to the object first and then have A copy its properties over them, possibly un-overriding B's override of A's function. The alternative would be to allow the interfaces to be listed in any order, but order them ourselves behind the scenes. However, I feel the way it's done here is more explicit API and simpler code behind the scenes, and therefore better on two counts. > So my understanding is that you can have this > > MyInterface = Lang.Interface({ > Name: 'MyInterface', > Requires: [ 'RequiredByMyInterface', 'MyInterface' ] > }); > > but not: > > MyInterface = Lang.Interface({ > Name: 'MyInterface', > Requires: [ 'MyInterface', 'RequiredByMyInterface' ] > }); > > Is that correct? Yes (barring the re-use of MyInterface inside its own definition, which I assume you didn't mean.) > Also, I'm a little confused as to why we read prototype.__name__ || > required.name || required (coerced to a string). Aren't all interface-like > objects going to be the same in that we can always read their name? Is there > any reason for all these permutations? Yes, this is confusing and I have added a comment. The __name__ property is only available on classes/interfaces defined inside GJS, and is the most accurate name from the JS programmer's point of view. required.name is the GType name and will be present on both GJS-defined and introspected GObjects, but it's not preferred to print it because it's not necessarily the name known to the GJS programmer. The last option (coercing required to a string) is just so that we print something sensible instead of "undefined" if someone sticks garbage into the Requires array.
(In reply to Philip Chimento from comment #27) > (In reply to Sam Spilsbury from comment #22) > > Review of attachment 306432 [details] [review] [review] [review]: > > Thanks for the review once more. I added some clarifying comments in this > new patch. I hope to have answered the rest of your questions below: Thank you, this patchset looks really great now. Everything is well tested and all the magic is quite well explained. The last few points that confused me initially are explained well in the comments. I'd say at this point everything is good to go in. I did noticed that there were a few un-covered cases, which I've added an "optional" patch to add some more tests to cover those. Everything is working fine though as far as I can tell. Were there any more burning issues before we merge this in?
Created attachment 307143 [details] [review] Some additional tests to cover the last remaining cases
Review of attachment 307143 [details] [review]: ::: installed-tests/js/testGObjectInterface.js @@ +125,3 @@ +function testRaisesWhenPureGObjectInterfaceDoesntRequireGObject() { + JSUnit.assertRaises(function() { + const GObjectInterfaceNotRequiringGObject = new Lang.Interface({ I think this is a bug, not a feature. Constructing a Lang.Interface without GObject.Object in Requires should just make a pure Javascript interface, without any GObject. Probably the reason why this is throwing is because I forgot to make it handle Requires being present but empty. I'll fix that. ::: installed-tests/js/testInterface.js @@ +311,3 @@ +function testToString() { + JSUnit.assertEquals('[interface Interface for AnInterface]', My inclination would be not to test this, since it doesn't really matter what the exact text is that toString() gives you. Although I suppose that people could conceivably rely on parsing out this string, so maybe it should be documented in this way.
Great idea to run it with coverage, I did not think of that. I think one of your new test cases exposed a bug in Lang.Interface so I'll fix that first.
Created attachment 307158 [details] [review] lang: Pure Javascript Lang.Interface This introduces Lang.Interface, a counterpart to Lang.Class that allows Lang.Class-created objects to implement interfaces. The interface's properties will be copied to the implementing object, so that you can have default implementations of methods. Any properties with the value Lang.Interface.UNIMPLEMENTED must be overridden by the implementing class. You will get an exception if you try to define a class that doesn't override one of these. An interface can require other interfaces or classes, just as with GObject interfaces. If an interface requires a class, it means that a class implementing that interface must inherit from the required class. GObject.Object-derived classes can also implement Lang.Interfaces. This commit does not yet let you define new GObject-style interfaces with properties and signals and a GType. The Lang.Interface._check() mechanism is loosely based on a MooTools plugin: https://github.com/sebwoh/Mootools-Class-Interfaces/ Implementation notes / Discussion points ======================================== - An alternative would be to make Lang.Interface a metaclass. (See installed-tests/js/testMetaclass.js for an example of metaclasses.) This patch assumes that we don't want Lang.Interface to inherit from Lang.Class but instead only have _Base as a shared parent. The metaclass approach would mean that you would create a new interface as a Lang.Class that extends Lang.Interface. - There is no this.parent() equivalent for interfaces. That's on purpose because there is also no such thing in C GObject; unlike parent classes where you get a convenient my_object_parent_class pointer to the class struct, you have to explicitly get the interface's vtable if you want to chain up. That would additionally require us to define a method resolution order, which is a bit of an alien concept in Javascript. - Interfaces purposely don't have an _init function. This is because GObject interfaces don't have an instance init function either, because they can't have any private data. However, I can see it being useful in Javascript, since your `this` is the implementing object, and you can stick any properties you want on there already in your interface methods. However, same concern about method resolution order applies. (Collaboration by Philip Chimento <philip@endlessm.com> and Roberto Goizueta <goizueta@endlessm.com>)
Created attachment 307159 [details] [review] gobject: GObject.Interface This adds the ability to define new interfaces in GJS that are known to GObject and that have signals and properties. Objects implementing a GObject.Interface get the interface's signals automatically. They must, however, reimplement the interface's properties or an error will occur. This is just as in C, except there we conveniently have g_param_spec_override(). In a later commit we will add a GObject.ParamSpec.override() to make this less painful in GJS. This also refactors some code that is now common to both classes and interfaces into reusable functions, both on the C and Javascript side of things. Where possible without too much churn, it uses newer SpiderMonkey APIs in code that it touches. (Collaboration by Philip Chimento <philip@endlessm.com> and Roberto Goizueta <goizueta@endlessm.com>)
Created attachment 307160 [details] [review] lang: Lang.Class.implements() This function does the work of g_type_is_a() and G_IS_...() for interfaces. It allows us to make better assertions in our tests and fills a need in GJS since there is no such thing as the instanceof operator for interfaces. Note. implements is a reserved word in ES6 but reserved words are allowed as property names, and we use reserved words elsewhere in GJS as property names as well. (Collaboration by Philip Chimento <philip@endlessm.com> and Roberto Goizueta <goizueta@endlessm.com>)
Created attachment 307161 [details] [review] gobject: GObject.ParamSpec.override() This adds a GObject.ParamSpec.override() which wraps g_param_spec_override(), in order to more conveniently implement properties that are present on interfaces. (Collaboration by Philip Chimento <philip@endlessm.com> and Roberto Goizueta <goizueta@endlessm.com>)
Created attachment 307162 [details] [review] installed-tests: Add some more cases for interfaces
Review of attachment 307158 [details] [review]: Looks good to me
Review of attachment 307159 [details] [review]: ::: modules/lang.js @@ +332,3 @@ + // interfaces should require GObject.Object anyway. + if (metaInterface === null) + throw new Error('Did you forget to include GObject.Object in Requires?'); Hmm, I think this is what I was referring to earlier in the test I wrote where you said that there was a bug. Does this line need to be removed, or was I in fact testing something else?
Hmm, seems like bugzilla is broken and I'm unable to mark the remaining three patches as accept-commit-now (In reply to Sam Spilsbury from comment #38) > Review of attachment 307159 [details] [review] [review]: > > ::: modules/lang.js > @@ +332,3 @@ > + // interfaces should require GObject.Object anyway. > + if (metaInterface === null) > + throw new Error('Did you forget to include GObject.Object in > Requires?'); > > Hmm, I think this is what I was referring to earlier in the test I wrote > where you said that there was a bug. Does this line need to be removed, or > was I in fact testing something else? Ah okay, on closer inspection, it seems like that error is for the case where a C-based interface doesn't also have GObject.Object in requires (but not for an interface requiring-nothing but specifying a GTypeName).
For some reason I'm unable to change the remaining patches to accepted_commit-now (bugzilla raises an error when attempting to do so). That said they're all LGTM to me. Nice work, looking forward to using this feature. As for patch #2 - that should also be accepted_commit-now in light of my last comment.
Review of attachment 307159 [details] [review]: Marking a-c-n as for last Sam's comment.
Review of attachment 307160 [details] [review]: ++
Review of attachment 307161 [details] [review]: ++
Review of attachment 307162 [details] [review]: ++
Review of attachment 307143 [details] [review]: I think this one also is good to go; I like the idea of testing toString(). We are just making sure it works, not setting it in stone by writing this testcase.
Review of attachment 307143 [details] [review]: This one was superseded, but I didn't have permissions to mark it obsolete.