After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 646633 - Support JS overrides to GObjectIntrospection
Support JS overrides to GObjectIntrospection
Status: RESOLVED FIXED
Product: gjs
Classification: Bindings
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gjs-maint
gjs-maint
Depends on:
Blocks: 622344 622921
 
 
Reported: 2011-04-03 16:49 UTC by Giovanni Campagna
Modified: 2011-05-27 15:47 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Introduce infrastructure for override modules (7.23 KB, patch)
2011-04-03 16:49 UTC, Giovanni Campagna
reviewed Details | Review
Return TRUE from GIRepositoryNamespace.resolve if no error (957 bytes, patch)
2011-05-19 16:51 UTC, Giovanni Campagna
committed Details | Review
Introduce infrastructure for override modules (6.42 KB, patch)
2011-05-19 16:51 UTC, Giovanni Campagna
committed Details | Review
Introduce infrastructure for override modules (7.05 KB, patch)
2011-05-19 17:00 UTC, Giovanni Campagna
reviewed Details | Review
Make GObjectIntrospection classes and methods writable (1.09 KB, patch)
2011-05-19 17:42 UTC, Giovanni Campagna
committed Details | Review

Description Giovanni Campagna 2011-04-03 16:49:26 UTC
PyGObject has a system called "overrides" that allows to replace some objects or methods with ones written in Python, used for example for convenience methods.
We could do the same in JS (and like PyGObject, use it for GVariant and GDBus).
Comment 1 Giovanni Campagna 2011-04-03 16:49:54 UTC
Created attachment 185043 [details] [review]
Introduce infrastructure for override modules

Override modules are JS modules that can subsume or replace native
modules from GObject Introspection.
Comment 2 Colin Walters 2011-04-04 14:39:56 UTC
One reason that pygobject added overrides was to be closer to old pygtk API, which doesn't apply here.

Note we already have a mechanism for overriding via C - that's the "foreign" types system.  I think GVariant is even marked as foreign.  So if the goal is to be able to implement foreign types in JS, sure.  (But it seems a lot saner to me to implement GVariant-JS in C)

If we add a general overrides mechanism, we're sliding back into the original days of hand-written binding API.  Maybe if it's constrained, so that we say only use it to implement varargs.

As far as GDBus, if the API isn't usable after binding GVariant then we should be looking at fixing it in GIO first.
Comment 3 Giovanni Campagna 2011-04-04 15:22:26 UTC
(In reply to comment #2)
> Note we already have a mechanism for overriding via C - that's the "foreign"
> types system.  I think GVariant is even marked as foreign.  So if the goal is
> to be able to implement foreign types in JS, sure.  (But it seems a lot saner
> to me to implement GVariant-JS in C)

GVariant was marked (foreign), and I removed the marking in bug 646635, because it has a GType and should be supported natively, without extra modules.
Then I also implemented GVariant packing and unpacking in JS, and it is very clean, much more than fiddling with jsvals and the JS API from C.

> If we add a general overrides mechanism, we're sliding back into the original
> days of hand-written binding API.  Maybe if it's constrained, so that we say
> only use it to implement varargs.

varargs, maps, iterators, overloading... there is a lot of stuff that introspection cannot do, but some JS magic can.

> As far as GDBus, if the API isn't usable after binding GVariant then we should
> be looking at fixing it in GIO first.

But you cannot have JSON introspection in GIO, or a dynamic custom GDBusProxy that exposes dbus methods as normal methods, or a GDBusInterfaceVTable that exposes JS objects over the bus.
Comment 4 Colin Walters 2011-05-16 22:01:04 UTC
Review of attachment 185043 [details] [review]:

Alright...I won't try to stop this train; but I'd still like to try hard to ensure any new GIO APIs etc. are introspectable out of the box.

::: gi/repo.c
@@ +51,3 @@
 GJS_DEFINE_PRIV_FROM_JS(Repo, gjs_repo_class)
 
+static struct JSObject * lookup_override_function(JSContext *, const char *);

"struct JSObject*" should just be "JSObject*".

@@ +554,3 @@
+                                        "overrides", &overridespkg) ||
+        !JSVAL_IS_OBJECT(overridespkg))
+    JS_BeginRequest(context);

I wonder if we should explicitly tell the imports machinery to *only* search DATADIR for this.  Otherwise we blow if a project happens to have an "overrides/Gio.js"; okay, yes unlikely, but still.

@@ +562,3 @@
+
+    if (!gjs_object_require_property(context, JSVAL_TO_OBJECT(module), "override module",
+    if (!gjs_object_require_property(context, global, "global object", "imports", &importer) ||

Personally I hate the Python '__' spam.  Can we just use "_init"?
Comment 5 Giovanni Campagna 2011-05-17 17:13:21 UTC
(In reply to comment #4)
> Review of attachment 185043 [details] [review]:
> 
> Alright...I won't try to stop this train; but I'd still like to try hard to
> ensure any new GIO APIs etc. are introspectable out of the box.

Of course, also given that GObjectIntrospection is not meant for GLib/GIO/Gtk, for which we can have awesome manual bindings, but for less used libraries (that therefore won't be added to the overrides).

> @@ +554,3 @@
> +                                        "overrides", &overridespkg) ||
> +        !JSVAL_IS_OBJECT(overridespkg))
> +    JS_BeginRequest(context);
> 
> I wonder if we should explicitly tell the imports machinery to *only* search
> DATADIR for this.  Otherwise we blow if a project happens to have an
> "overrides/Gio.js"; okay, yes unlikely, but still.

The same project will blow if it includes gi/GLib.js or gi/Gtk.js, so it's not much of an issue ("imports.gi.NameSpace.Class" is used when looking for the parent prototype of an object class). Just document that some names are reserved.

> @@ +562,3 @@
> +
> +    if (!gjs_object_require_property(context, JSVAL_TO_OBJECT(module),
> "override module",
> +    if (!gjs_object_require_property(context, global, "global object",
> "imports", &importer) ||
> 
> Personally I hate the Python '__' spam.  Can we just use "_init"?

I consider _ for private to the application and __ for private to the language. It can be changed, of course.
Comment 6 Colin Walters 2011-05-18 15:24:16 UTC
(In reply to comment #5)
> (In reply to comment #4)
> > Review of attachment 185043 [details] [review] [details]:
> > 
> > Alright...I won't try to stop this train; but I'd still like to try hard to
> > ensure any new GIO APIs etc. are introspectable out of the box.
> 
> Of course, also given that GObjectIntrospection is not meant for GLib/GIO/Gtk,
> for which we can have awesome manual bindings, but for less used libraries
> (that therefore won't be added to the overrides).

I always said GLib should be manually bound, but definitely g-i was intended for GIO - it's an important part of the platform.

What are you saying here exactly?  That third party libraries won't need overrides?

> The same project will blow if it includes gi/GLib.js or gi/Gtk.js, so it's not
> much of an issue ("imports.gi.NameSpace.Class" is used when looking for the
> parent prototype of an object class). Just document that some names are
> reserved.

There's also the fact that we're doubling the stat() calls that are made for imports.  And I'd actually like to fold "gi" into gjs as a non-module; it's basically a stupid split.

Anyways, this is fine for now, we can fix it later.

> I consider _ for private to the application and __ for private to the language.
> It can be changed, of course.

Neither of:
https://live.gnome.org/GnomeShell/StyleGuide
http://git.gnome.org/browse/gjs/plain/doc/Style_Guide.txt

Have any mention of __.  I guess the only precedent would be Mozilla's __proto__ ?

Honestly I'd prefer "_init" more, and the argument here is that the function is indeed "private" to the override module in that we don't want it exported into the overridden module like Gtk, so all we need to do is filter out a leading '_'.
Comment 7 Giovanni Campagna 2011-05-19 16:48:31 UTC
(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > Review of attachment 185043 [details] [review] [details] [details]:
> > > 
> > > Alright...I won't try to stop this train; but I'd still like to try hard to
> > > ensure any new GIO APIs etc. are introspectable out of the box.
> > 
> > Of course, also given that GObjectIntrospection is not meant for GLib/GIO/Gtk,
> > for which we can have awesome manual bindings, but for less used libraries
> > (that therefore won't be added to the overrides).
> 
> I always said GLib should be manually bound, but definitely g-i was intended
> for GIO - it's an important part of the platform.
> 
> What are you saying here exactly?  That third party libraries won't need
> overrides?

That third-party libraries won't *get* overrides, because overrides live in GJS tree.

> > The same project will blow if it includes gi/GLib.js or gi/Gtk.js, so it's not
> > much of an issue ("imports.gi.NameSpace.Class" is used when looking for the
> > parent prototype of an object class). Just document that some names are
> > reserved.
> 
> There's also the fact that we're doubling the stat() calls that are made for
> imports.  And I'd actually like to fold "gi" into gjs as a non-module; it's
> basically a stupid split.

That's only the first time a module is imported. I don't think it is much of an issue.

> 
> Anyways, this is fine for now, we can fix it later.
> 
> > I consider _ for private to the application and __ for private to the language.
> > It can be changed, of course.
> 
> Neither of:
> https://live.gnome.org/GnomeShell/StyleGuide
> http://git.gnome.org/browse/gjs/plain/doc/Style_Guide.txt
> 
> Have any mention of __.  I guess the only precedent would be Mozilla's
> __proto__ ?

And __parent__, and __moduleName__.

> Honestly I'd prefer "_init" more, and the argument here is that the function is
> indeed "private" to the override module in that we don't want it exported into
> the overridden module like Gtk, so all we need to do is filter out a leading
> '_'.

Ok, so I'll change it to _init.

Preparing updated patch...
Comment 8 Giovanni Campagna 2011-05-19 16:51:17 UTC
Created attachment 188147 [details] [review]
Return TRUE from GIRepositoryNamespace.resolve if no error

Even if nothing is set, we must return TRUE when there is no error
(so the normal property lookup is performed, eventually returning
"undefined"), otherwise JS_ExecuteScript fails with no exception.
Comment 9 Giovanni Campagna 2011-05-19 16:51:33 UTC
Created attachment 188148 [details] [review]
Introduce infrastructure for override modules

Override modules are JS modules that can subsume or replace native
modules from GObject Introspection.
Comment 10 Colin Walters 2011-05-19 16:52:51 UTC
Review of attachment 188147 [details] [review]:

Makes sense.
Comment 11 Colin Walters 2011-05-19 16:52:52 UTC
Review of attachment 188147 [details] [review]:

Makes sense.
Comment 12 Colin Walters 2011-05-19 16:53:28 UTC
Review of attachment 188148 [details] [review]:

Ok, let's go with it.
Comment 13 Giovanni Campagna 2011-05-19 17:00:48 UTC
Created attachment 188149 [details] [review]
Introduce infrastructure for override modules

Override modules are JS modules that can subsume or replace native
modules from GObject Introspection.

(Added a bit that was inside the GVariant patch by mistake)
Comment 14 Giovanni Campagna 2011-05-19 17:32:52 UTC
Attachment 188147 [details] pushed as 663e9df - Return TRUE from GIRepositoryNamespace.resolve if no error
Attachment 188148 [details] pushed as c425d90 - Introduce infrastructure for override modules
Comment 15 Colin Walters 2011-05-19 17:33:34 UTC
Review of attachment 188149 [details] [review]:

So...the JS_PROP_FLAGS change is *important* and merits being a separate commit, with a separate rationale.
Comment 16 Giovanni Campagna 2011-05-19 17:34:39 UTC
Oops. I closed this one too early.
Preparing the new patch.
Comment 17 Giovanni Campagna 2011-05-19 17:42:49 UTC
Created attachment 188158 [details] [review]
Make GObjectIntrospection classes and methods writable

Modify the default JSPROPERTY flags to remove JSPROP_READONLY, so
it is possible to replace the methods and classes from overrides.

I'm really sorry for the mess in git history...
Comment 18 Colin Walters 2011-05-19 19:55:35 UTC
Review of attachment 188158 [details] [review]:

I think this is OK; in some theoretical world, readonly objects could be shared better between multiple processes, but there will probably always be more important things to optimize.

Thanks for splitting it out and making it explicit.

Note with this change we could now get rid of shell_global_set_property_mutable from gnome-shell (right?)
Comment 19 Colin Walters 2011-05-27 15:47:41 UTC
Attachment 188158 [details] pushed as 872afb2 - Make GObjectIntrospection classes and methods writable