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 669350 - GDBus: introduce new convenience wrappers
GDBus: introduce new convenience wrappers
Status: RESOLVED WONTFIX
Product: gjs
Classification: Bindings
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: gjs-maint
gjs-maint
Depends on:
Blocks: 669349
 
 
Reported: 2012-02-03 22:51 UTC by Jasper St. Pierre (not reading bugmail)
Modified: 2015-04-27 02:05 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
GDBus: introduce new convenience wrappers (19.90 KB, patch)
2012-02-03 22:51 UTC, Jasper St. Pierre (not reading bugmail)
none Details | Review
GDBus: fix tests (2.26 KB, patch)
2012-02-03 22:51 UTC, Jasper St. Pierre (not reading bugmail)
none Details | Review
GDBus: Allow prefixing property names with 'DBus' to help prevent collisions (1.34 KB, patch)
2012-02-03 22:51 UTC, Jasper St. Pierre (not reading bugmail)
none Details | Review
GDBus: Allow automatic exporting of implementations on construction (1.24 KB, patch)
2012-02-03 22:51 UTC, Jasper St. Pierre (not reading bugmail)
none Details | Review
GDBus: introduce new convenience wrappers (18.85 KB, patch)
2012-02-06 18:58 UTC, Giovanni Campagna
none Details | Review
GDBus: Allow automatic exporting of implementations on construction (1.68 KB, patch)
2012-02-06 19:34 UTC, Jasper St. Pierre (not reading bugmail)
reviewed Details | Review
GDBus: introduce new convenience wrappers (20.28 KB, patch)
2012-06-02 22:45 UTC, Giovanni Campagna
needs-work Details | Review
Gio: reinstate deprecated GDBus bindings (11.52 KB, patch)
2012-06-02 22:45 UTC, Giovanni Campagna
none Details | Review
Gio: reinstate deprecated GDBus bindings (11.54 KB, patch)
2012-06-02 22:54 UTC, Giovanni Campagna
needs-work Details | Review
Gio: split GDBus implementation into helpers (6.68 KB, patch)
2012-06-14 20:43 UTC, Giovanni Campagna
reviewed Details | Review
GDBus: introduce new convenience wrappers (23.79 KB, patch)
2012-06-14 20:43 UTC, Giovanni Campagna
none Details | Review
scanner: fix pairing of error quarks with registered enums (1.84 KB, patch)
2012-06-16 13:16 UTC, Giovanni Campagna
committed Details | Review
Complete implementation of GError marshalling (4.21 KB, patch)
2012-06-16 14:39 UTC, Giovanni Campagna
committed Details | Review
Fix memory leaks (3.62 KB, patch)
2012-06-16 14:39 UTC, Giovanni Campagna
committed Details | Review
Gio: split GDBus implementation into helpers (6.08 KB, patch)
2012-06-16 14:40 UTC, Giovanni Campagna
needs-work Details | Review
GDBus: introduce new convenience wrappers (24.52 KB, patch)
2012-06-16 14:41 UTC, Giovanni Campagna
needs-work Details | Review
GDBus: use gio-style asyncs for the new bindings (14.04 KB, patch)
2012-06-16 16:40 UTC, Giovanni Campagna
reviewed Details | Review
Gio: split GDBus implementation into helpers (5.39 KB, patch)
2012-06-17 15:38 UTC, Giovanni Campagna
committed Details | Review
Gio: modernize DBus bindings (19.50 KB, patch)
2012-06-17 16:09 UTC, Giovanni Campagna
committed Details | Review
GDBus: introduce new convenience wrappers (26.43 KB, patch)
2012-06-17 16:09 UTC, Giovanni Campagna
reviewed Details | Review
GDBus: use gio-style asyncs for the new bindings (17.88 KB, patch)
2012-06-17 16:09 UTC, Giovanni Campagna
none Details | Review
scanner: complete the enum-to-error-quark fix (14.67 KB, patch)
2012-06-18 21:07 UTC, Giovanni Campagna
committed Details | Review
GDBus: introduce new convenience wrappers (25.51 KB, patch)
2012-06-20 22:20 UTC, Giovanni Campagna
needs-work Details | Review
GDBus: allow overriding _init in a Gio.DBusProxyClass (4.50 KB, patch)
2012-06-20 22:21 UTC, Giovanni Campagna
none Details | Review
GDBus: allow overriding _init in a Gio.DBusProxyClass (4.93 KB, patch)
2012-06-21 17:39 UTC, Giovanni Campagna
none Details | Review
Throw an exception when registering a GType that already exists (892 bytes, patch)
2012-06-21 17:39 UTC, Giovanni Campagna
committed Details | Review
Unit tests: run tests in a forked child (4.23 KB, patch)
2012-06-21 17:40 UTC, Giovanni Campagna
rejected Details | Review
GDBus: allow overriding _init in a Gio.DBusProxyClass (9.00 KB, patch)
2012-08-24 13:35 UTC, Giovanni Campagna
accepted-commit_now Details | Review
GDBus2: support org.gtk.GDBus.C.ForceGVariant annotation (10.82 KB, patch)
2012-11-02 14:44 UTC, Giovanni Campagna
none Details | Review
GDBus: introduce new convenience wrappers (25.51 KB, patch)
2013-04-12 15:31 UTC, Giovanni Campagna
none Details | Review
GDBus: allow overriding _init in a Gio.DBusProxyClass (8.95 KB, patch)
2013-04-12 15:31 UTC, Giovanni Campagna
none Details | Review
GObject: fix building a class with non trivial accessor properties (2.65 KB, patch)
2013-04-12 15:31 UTC, Giovanni Campagna
none Details | Review
GDBus2: support org.gtk.GDBus.C.ForceGVariant annotation (11.65 KB, patch)
2013-04-12 15:31 UTC, Giovanni Campagna
none Details | Review
GDBus: use gio-style asyncs for the new bindings (17.28 KB, patch)
2013-04-12 15:32 UTC, Giovanni Campagna
none Details | Review

Description Jasper St. Pierre (not reading bugmail) 2012-02-03 22:51:26 UTC
These are the only patches left from the giant meta-class/gobject-inheritance/other stuff landing. We're holding off on pushing them because they break backwards compatibility.
Comment 1 Jasper St. Pierre (not reading bugmail) 2012-02-03 22:51:28 UTC
Created attachment 206736 [details] [review]
GDBus: introduce new convenience wrappers

Using the new metaclass system, introduce Gio.DBusProxyClass and
Gio.DBusImplementerClass, that allow declaring specialized proxies/
implementations for specific interfaces.
Comment 2 Jasper St. Pierre (not reading bugmail) 2012-02-03 22:51:30 UTC
Created attachment 206737 [details] [review]
GDBus: fix tests

I like the new names better.
Comment 3 Jasper St. Pierre (not reading bugmail) 2012-02-03 22:51:33 UTC
Created attachment 206738 [details] [review]
GDBus: Allow prefixing property names with 'DBus' to help prevent collisions

Some implementers of DBus interfaces may be asked to implement properties
called 'Name' or 'Interface', which would collide with our own uses of
these functions. Allow these implementers to provide 'DBusName' or
'DBusInterface' instead.
Comment 4 Jasper St. Pierre (not reading bugmail) 2012-02-03 22:51:36 UTC
Created attachment 206739 [details] [review]
GDBus: Allow automatic exporting of implementations on construction

If a DBusImplementerClass provides "BusType" and "ObjectPath" names
in its parameters, export it automatically when an instance is
constructed.
Comment 5 Giovanni Campagna 2012-02-06 18:58:05 UTC
Created attachment 206926 [details] [review]
GDBus: introduce new convenience wrappers

Using the new metaclass system, introduce Gio.DBusProxyClass and
Gio.DBusImplementerClass, that allow declaring specialized proxies/
implementations for specific interfaces.

----

Setting .Extends must be done in _construct, since the prototype
and JS class are created by Lang.Class._construct and GObject.Class._construct.
Comment 6 Giovanni Campagna 2012-02-06 19:09:45 UTC
(In reply to comment #4)
> Created an attachment (id=206739) [details] [review]
> GDBus: Allow automatic exporting of implementations on construction
> 
> If a DBusImplementerClass provides "BusType" and "ObjectPath" names
> in its parameters, export it automatically when an instance is
> constructed.

Seems you had git problems with this one.
Comment 7 Jasper St. Pierre (not reading bugmail) 2012-02-06 19:28:38 UTC
(In reply to comment #5)
> Created an attachment (id=206926) [details] [review]
> GDBus: introduce new convenience wrappers
> 
> Using the new metaclass system, introduce Gio.DBusProxyClass and
> Gio.DBusImplementerClass, that allow declaring specialized proxies/
> implementations for specific interfaces.
> 
> ----
> 
> Setting .Extends must be done in _construct, since the prototype
> and JS class are created by Lang.Class._construct and GObject.Class._construct.

Yeah, I noticed that right after I uploaded it. Incorrect rebase.

(In reply to comment #6)
> (In reply to comment #4)
> > Created an attachment (id=206739) [details] [review] [details] [review]
> > GDBus: Allow automatic exporting of implementations on construction
> > 
> > If a DBusImplementerClass provides "BusType" and "ObjectPath" names
> > in its parameters, export it automatically when an instance is
> > constructed.
> 
> Seems you had git problems with this one.

Huh. That's certainly not what I meant to happen.
Comment 8 Jasper St. Pierre (not reading bugmail) 2012-02-06 19:34:04 UTC
Created attachment 206930 [details] [review]
GDBus: Allow automatic exporting of implementations on construction

If a DBusImplementerClass provides "BusType" and "ObjectPath" names
in its parameters, export it automatically when an instance is
constructed.
Comment 9 Giovanni Campagna 2012-02-06 19:44:33 UTC
Review of attachment 206930 [details] [review]:

I'd say it looks good in general.
Comment 10 Giovanni Campagna 2012-02-10 21:43:34 UTC
(In reply to comment #0)
> These are the only patches left from the giant
> meta-class/gobject-inheritance/other stuff landing. We're holding off on
> pushing them because they break backwards compatibility.

There was another patch in that branch, the support for boxed constructors. As I don't if you uploaded somewhere else, or just skipped, I'm uploading it at bug 612033. It's worth merging it together with these one, as it includes minor API breaks for some edge cases.
Comment 11 Jasper St. Pierre (not reading bugmail) 2012-02-10 22:17:13 UTC
(In reply to comment #10)
> (In reply to comment #0)
> > These are the only patches left from the giant
> > meta-class/gobject-inheritance/other stuff landing. We're holding off on
> > pushing them because they break backwards compatibility.
> 
> There was another patch in that branch, the support for boxed constructors. As
> I don't if you uploaded somewhere else, or just skipped, I'm uploading it at
> bug 612033. It's worth merging it together with these one, as it includes minor
> API breaks for some edge cases.

Right. I talked it over with Colin and he didn't like the patch. It removes support for hash-args boxed constructors like:

    new Clutter.Color({ red: 0, green: 0, blue: 255, alpha: 255 });

Which we weren't comfortable breaking API for. I'm not sure we can support both, though/
Comment 12 Giovanni Campagna 2012-02-11 16:06:54 UTC
(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #0)
> > > These are the only patches left from the giant
> > > meta-class/gobject-inheritance/other stuff landing. We're holding off on
> > > pushing them because they break backwards compatibility.
> > 
> > There was another patch in that branch, the support for boxed constructors. As
> > I don't if you uploaded somewhere else, or just skipped, I'm uploading it at
> > bug 612033. It's worth merging it together with these one, as it includes minor
> > API breaks for some edge cases.
> 
> Right. I talked it over with Colin and he didn't like the patch. It removes
> support for hash-args boxed constructors like:
> 
>     new Clutter.Color({ red: 0, green: 0, blue: 255, alpha: 255 });
> 
> Which we weren't comfortable breaking API for. I'm not sure we can support
> both, though/

It doesn't remove that ability, the fact is that Clutter.Color is a broken edge case: it has no zero-args constructor and one default constructor with four arguments. If the type already has a zero args constructor, nothing changes and we call that instead.
In any case, if backward compatibility is a problem, we can add an override specifically for Clutter.Color.

Additionally, it would make sense at least to merge the GVariant part of it, so that we can drop the awful "GLib.Variant.new" at the same time as the big GDBus switch.
Comment 13 Jasper St. Pierre (not reading bugmail) 2012-02-11 16:36:52 UTC
(In reply to comment #12)
> It doesn't remove that ability, the fact is that Clutter.Color is a broken edge
> case: it has no zero-args constructor and one default constructor with four
> arguments. If the type already has a zero args constructor, nothing changes and
> we call that instead.

It's not a broken edge case. While porting gnome-shell I came across several other boxed types just like that, and we need to support them. I'm not sure how we can possibly support both (what if a boxed constructor takes a GHashTable, etc.)

> we call that instead.
> In any case, if backward compatibility is a problem, we can add an override
> specifically for Clutter.Color.

I don't think adding overrides for "third-party libraries" (GTK+, Clutter, not directly related to GLib/GObject) is a good path to start down.
 
> Additionally, it would make sense at least to merge the GVariant part of it, so
> that we can drop the awful "GLib.Variant.new" at the same time as the big GDBus
> switch.

GLib.Variant.new isn't that terrible, and I can wait.
Comment 14 Jasper St. Pierre (not reading bugmail) 2012-02-11 16:46:06 UTC
So, how do you feel about keeping the monkey patch approach, but introducing new classes? This should keep the changes quite low, and allow the old monkey patching approach when it's more convenient (EndSessionDialog, which inherits from ModalDialog, but also exports a DBus API), and should allow us to patch the Shell whenever we feel like it.
Comment 15 Giovanni Campagna 2012-02-15 15:26:14 UTC
(In reply to comment #14)
> So, how do you feel about keeping the monkey patch approach, but introducing
> new classes? This should keep the changes quite low, and allow the old monkey
> patching approach when it's more convenient (EndSessionDialog, which inherits
> from ModalDialog, but also exports a DBus API), and should allow us to patch
> the Shell whenever we feel like it.

I thought about that when I wrote the new API, but really, the monkey patching, especially on the client-side, is horrible and incredibly hacky, in particular if you need asynchronous init or special flags.
I'd like to remove it before API freeze, because otherwise we'll need to support it indefinitely, and we already have to support the old dbus-glib bindings. Two DBus bindings should be enough.
I know it's quite a big change for the Shell, but we're still in time, we can fix bugs as they surface before 3.4.0, and most code changes are about proxy class declarations, not proxy usage.

As for the implementation side, I'd like to remove wrapJSObject too, as it exposes one implementation detail, GjsDBus.Implementation. If all exporters are Gio.DBusImplementerBase, we can at some point remove GjsDBusImplementation without an API break and have Gio.DBusImplementerBase be a GDBusIterfaceSkeleton (once we sort out GDBusInterfaceVTable, that is). This is not possible if code relies on GjsDBus.Implementation directly.
Comment 16 Jasper St. Pierre (not reading bugmail) 2012-02-15 20:33:28 UTC
(In reply to comment #15)
> I'd like to remove it before API freeze, because otherwise we'll need to
> support it indefinitely, and we already have to support the old dbus-glib
> bindings.

Indefinitely meaning the time between 3.4 and 3.6?

This isn't important enough to land for 3.4, for me.
Comment 17 Giovanni Campagna 2012-02-15 20:52:22 UTC
(In reply to comment #16)
> (In reply to comment #15)
> > I'd like to remove it before API freeze, because otherwise we'll need to
> > support it indefinitely, and we already have to support the old dbus-glib
> > bindings.
> 
> Indefinitely meaning the time between 3.4 and 3.6?

No, indefinitely meaning between 3.3.90 and forever. gjs is not part of the Core, it's in the bindings, and thus subject to same API/ABI stability rules as the Platform, which means that once something goes in, it's in forever (or that's the idea - it's easier with C, I agree)
Comment 18 Jasper St. Pierre (not reading bugmail) 2012-02-15 20:58:12 UTC
I'm not sure where you got the idea of those API breaks. We've certainly broken API before (a major one being the array length changes, for example).
Comment 19 Colin Walters 2012-02-18 15:57:11 UTC
(In reply to comment #17)
> (In reply to comment #16)
> > (In reply to comment #15)
> > > I'd like to remove it before API freeze, because otherwise we'll need to
> > > support it indefinitely, and we already have to support the old dbus-glib
> > > bindings.
> > 
> > Indefinitely meaning the time between 3.4 and 3.6?
> 
> No, indefinitely meaning between 3.3.90 and forever. gjs is not part of the
> Core, it's in the bindings, and thus subject to same API/ABI stability rules as
> the Platform, which means that once something goes in, it's in forever (or
> that's the idea - it's easier with C, I agree)

I think in reality unless we take concrete technical steps to try to enforce this, it tends to be "just words".  It is more obvious when one breaks compatibility in C for sure.

But as far as the official status of gjs goes - it's in gnome-suites-core-deps-3.4.modules, and listed under the comment:

  <!-- OS Core unstable dependencies -->

I think the "official status" therefore is "while GNOME does currently use this, it may or may not be API/ABI stable, and may or may not appear in future releases".

That all said I'd like to *try* to avoid breaking gjs API where possible, for multiple reasons.  One of those reasons is that due to JavaScript being a dynamic language, it's really easy to have "undetected" breakage in a less used codepath.  Jasper has still been fixing random bugs from the GDBus porting for example.
Comment 20 Jasper St. Pierre (not reading bugmail) 2012-05-17 12:40:09 UTC
How do you guys feel about getting this in for 3.6?
Comment 21 Giovanni Campagna 2012-06-02 22:43:43 UTC
I'd like to have these in for 3.6, as it was seen they are definitely nicer from the POV of the app.
I've made an attempt to reinstate backward compatibility, while still keeping code duplication low.
Comment 22 Giovanni Campagna 2012-06-02 22:45:11 UTC
Created attachment 215511 [details] [review]
GDBus: introduce new convenience wrappers

Using the new metaclass system, introduce Gio.DBusProxyClass and
Gio.DBusImplementerClass, that allow declaring specialized proxies/
implementations for specific interfaces.
Comment 23 Giovanni Campagna 2012-06-02 22:45:31 UTC
Created attachment 215512 [details] [review]
Gio: reinstate deprecated GDBus bindings

Reintroduce makeProxyWrapper and wrapJSObject for backward
compatibility with apps there were ported for 3.4
Comment 24 Giovanni Campagna 2012-06-02 22:54:36 UTC
Created attachment 215513 [details] [review]
Gio: reinstate deprecated GDBus bindings

Reintroduce makeProxyWrapper and wrapJSObject for backward
compatibility with apps there were ported for 3.4

Removed the dependency on the boxed constructor patch.
Comment 25 Jasper St. Pierre (not reading bugmail) 2012-06-14 04:00:11 UTC
Review of attachment 215511 [details] [review]:

So, while I like the new proxy API, I'm not sure I like the implementer stuff. I'm really not a big fan of having a DBus implementation requiring inheritance.

Additionally, this patch and the next do a lot of moving around. I'd like to see two patches:

1) Clean up existing implementation (split _handleMethodCall out)
2) Introduce new API wrappers.

::: test/js/testGDBus.js
@@ +85,3 @@
 const PROP_WRITE_ONLY_INITIAL_VALUE = "Initial value";
 
+log('about to build a Test class');

scrap
Comment 26 Jasper St. Pierre (not reading bugmail) 2012-06-14 04:00:18 UTC
Review of attachment 215513 [details] [review]:

Can we keep the tests and squash this patch in with the last?

::: modules/overrides/Gio.js
@@ +335,2 @@
+function _makeProxyWrapper(interfaceXml) {
+    log ('makeProxyWrapper is deprecated. Use Gio.DBusProxyClass instead');

This really should be behind an envvar at best.
Comment 27 Giovanni Campagna 2012-06-14 20:43:24 UTC
Created attachment 216462 [details] [review]
Gio: split GDBus implementation into helpers

Soon new API wrappers will be introduced, and this will help
reusing code.
Comment 28 Giovanni Campagna 2012-06-14 20:43:34 UTC
Created attachment 216463 [details] [review]
GDBus: introduce new convenience wrappers

Using the new metaclass system, introduce Gio.DBusProxyClass and
Gio.DBusImplementerClass, that allow declaring specialized proxies/
implementations for specific interfaces.
Comment 29 Giovanni Campagna 2012-06-14 20:46:44 UTC
Here you are, two patches ready.
I kept the deprecation warning for makeProxyWrapper (the sooner we kill it the better, as it's using hacks to hook into construction), but removed the one for wrapJSObject: as I understand it, there are use cases for exporting without subclassing, although it does expose some of the implementation details.
Comment 30 Colin Walters 2012-06-14 20:56:09 UTC
Review of attachment 216462 [details] [review]:

::: modules/overrides/Gio.js
@@ +302,3 @@
+	    // if we don't do this, the other side will never see a reply
+	    invocation.return_dbus_error('org.gnome.gjs.JSError.ValueError',
+					 "The return value from the method handler was not in the correct format");

This is somewhat confusing because it might make the programmer think THEIR code was buggy, but actually it's the service-side code.

How about "Remote service returned an incorrect value type"

@@ +322,3 @@
+}
+
+function _handlePropertySet(info, impl, property_name, new_value) {

Hmm...we're not validating that the property exists?  We should support read-only properties too.
Comment 31 Jasper St. Pierre (not reading bugmail) 2012-06-14 21:00:20 UTC
So, OK. Let's also try to get a fix for

https://bugzilla.gnome.org/show_bug.cgi?id=677513#c4

in here. I think the suggestion we wanted was to do something like GSimpleAsyncResult, which requires a pair of calls, so it can throw an error.

I'd also like to drop the unmaintained imports.dbus as well.
Comment 32 Jasper St. Pierre (not reading bugmail) 2012-06-14 21:03:41 UTC
Review of attachment 216462 [details] [review]:

::: modules/overrides/Gio.js
@@ +125,3 @@
+
+    if (insideClass)
+	return this.wrapFunction(method, f);

Not quite yet.

@@ +276,3 @@
+	    if (e.name.indexOf('.') == -1) {
+		// likely to be a normal JS error
+		e.name = 'org.gnome.gjs.JSError.' + e.name;

If this is a new GLib error wrapper, we'll get an error as we try to assign to a read only property.

@@ +287,3 @@
+	try {
+	    if (!(retval instanceof GLib.Variant)) {
+		// attemp packing according to out signature

"attempt"
Comment 33 Giovanni Campagna 2012-06-16 11:46:06 UTC
(In reply to comment #30)
> @@ +322,3 @@
> +}
> +
> +function _handlePropertySet(info, impl, property_name, new_value) {
> 
> Hmm...we're not validating that the property exists?  We should support
> read-only properties too.

GDBus checks that already (gdbusconnection.c:4250)
Comment 34 Giovanni Campagna 2012-06-16 13:16:26 UTC
Created attachment 216567 [details] [review]
scanner: fix pairing of error quarks with registered enums

_uscore_type_names maps from the c_symbol_prefix, which has the
global ns prefix removed, so we need to split the function symbol
before the lookup. Previously it worked because it used the heuristics
for unregistered enums (and failed for GDBusError, which has two
uppercase letters in succession)

I was improving a bit the error handling (to use the new GError
marshalling), and found a bug in gobject-introspection that makes
Gio.DBusError inusable.
Comment 35 Giovanni Campagna 2012-06-16 14:39:38 UTC
Created attachment 216568 [details] [review]
Complete implementation of GError marshalling

GErrors that are not used for "throws" are not reported as regular
boxed types but with special ERROR type tag, so we need to special
case it everywhere. This was already done for out arguments and
return values, but not for in arguments, array elements and argument
releasing.
Comment 36 Giovanni Campagna 2012-06-16 14:39:47 UTC
Created attachment 216569 [details] [review]
Fix memory leaks

GValues inside flat arrays must be unset when not transferring
ownership, and GErrors must be freed after setting the exception.
Comment 37 Giovanni Campagna 2012-06-16 14:40:11 UTC
Created attachment 216570 [details] [review]
Gio: split GDBus implementation into helpers

Soon new API wrappers will be introduced, and this will help
reusing code.
Comment 38 Giovanni Campagna 2012-06-16 14:41:21 UTC
Created attachment 216571 [details] [review]
GDBus: introduce new convenience wrappers

Using the new metaclass system, introduce Gio.DBusProxyClass and
Gio.DBusImplementerClass, that allow declaring specialized proxies/
implementations for specific interfaces.

This still uses the current convention for proxy methods. Next
patch will introduce async gio-style, and then we can decide which
one is better.
Comment 39 Giovanni Campagna 2012-06-16 16:40:49 UTC
Created attachment 216575 [details] [review]
GDBus: use gio-style asyncs for the new bindings

Split asynchronous result collection into a MethodFinish function,
so that exceptions can be caught in the usual way instead of being
passed as objects to the functions. Also, remove the flags parameter
and make the cancellable argument compulsory, matching the code
generated by gdbus-codegen.
Comment 40 Jasper St. Pierre (not reading bugmail) 2012-06-16 19:13:54 UTC
Review of attachment 216567 [details] [review]:

Makes sense to me.
Comment 41 Jasper St. Pierre (not reading bugmail) 2012-06-16 19:15:22 UTC
Review of attachment 216568 [details] [review]:

Considering errors are also boxeds, couldn't we just use the boxed type machinery for this?
Comment 42 Jasper St. Pierre (not reading bugmail) 2012-06-16 19:19:44 UTC
Review of attachment 216570 [details] [review]:

Code shouldn't be changed while splitting out like this. Can you do a fixup patch first?

::: modules/overrides/Gio.js
@@ -370,3 @@
     };
 
-    // This should be done inside a constructor, but it cannot currently

Too early as well.
Comment 43 Jasper St. Pierre (not reading bugmail) 2012-06-16 19:24:14 UTC
Review of attachment 216575 [details] [review]:

::: modules/overrides/Gio.js
@@ +49,3 @@
+
+    var signatureLength = inSignature.length;
+    var numberArgs = sync ? signatureLength + 2 : signatureLength + 1;

Was this wrong initially? For a sync, we require 1 extra argument (callable). For an async, we require two (callable, async callback)

@@ +53,3 @@
+    if (arg_array.length < numberArgs) {
+        throw new Error("Wrong number of arguments passed for method: " + methodName +
+                       ". Expected " + numberArgs + ", got " + arg_array.length);

Indentation issues.

::: test/js/testGDBus2.js
@@ +278,3 @@
+	    [theResult] = proxy.frobateStuffFinish(result);
+	} catch(excp) {
+            theExcp = excp;

Mixing tabs and spaces here. Untabify the entire thing, please.
Comment 44 Jasper St. Pierre (not reading bugmail) 2012-06-16 19:27:44 UTC
Review of attachment 216570 [details] [review]:

::: modules/overrides/Gio.js
@@ +295,3 @@
+		    // if one arg, we don't require the handler wrapping it
+		    // into an Array
+		    retval = [retval];

Also, I know this was in the existing code, but I really don't like this "feature". We should be consistent with our API, not do garbage magic like this.
Comment 45 Jasper St. Pierre (not reading bugmail) 2012-06-16 19:27:51 UTC
Review of attachment 216570 [details] [review]:

::: modules/overrides/Gio.js
@@ +295,3 @@
+		    // if one arg, we don't require the handler wrapping it
+		    // into an Array
+		    retval = [retval];

Also, I know this was in the existing code, but I really don't like this "feature". We should be consistent with our API, not do garbage magic like this.
Comment 46 Jasper St. Pierre (not reading bugmail) 2012-06-16 19:43:20 UTC
Review of attachment 216571 [details] [review]:

::: modules/overrides/Gio.js
@@ +192,3 @@
+
+    _construct: function(params) {
+	params.Extends = Gio.DBusProxy;

Indentation issues. Spaces only.

@@ +213,3 @@
+	}
+
+	params._init = function(params) {

Not a fan of the two completely different variables called "params", shadowed like this. Maybe name _init's params to be something like "klass".

@@ +217,3 @@
+
+	    if (!params)
+		params = { };

{}

@@ +245,3 @@
+	    }
+
+	    // temporary hack, until we have proper GObject signals

Even if we do get proper GObject signals, we shouldn't switch over to them.

@@ +256,3 @@
+    },
+
+    _fillParameters: function(params, bus, name, object) {

In some ways I'm sort of against the different naming schemes here. It's sort of weird to have a slim proxy be

    let A = new Lang.Class({
        BusType: Gio.DBusBusType.SESSION /* I forget the actual enum name */,
        BusName: "FooBar",
        ObjectPath: "/Foo/Bar"
    });

and a non-slim proxy being:

    let a = new A({
        g_connection: Gio.DBus.session,
        g_name: "FooBar",
        g_object_path: "/Foo/Bar"
    });

I'm neutral on

    let A = new Lang.Class({
        Name: "A",
        g_connection: Gio.DBus.session
    });

because stylistically, it's weird. The only other idea that I have that I'm sort of OK on, even though it's overkill, is to implement something like this in Lang.Class or maybe GObject.Class:

    let A = new Lang.Class({
        DefaultParams: {
            g_connection: Gio.DBus.session,
        }
    });

Although we really shouldn't be connecting to the DBus session bus at parse time.

@@ +303,2 @@
 function _makeProxyWrapper(interfaceXml) {
+    log ('makeProxyWrapper is deprecated. Use Gio.DBusProxyClass instead');

I think removing the method outright is better than spamming the user's ~/.xsession-errors.
Comment 47 Giovanni Campagna 2012-06-17 13:11:38 UTC
(In reply to comment #46)
> Review of attachment 216571 [details] [review]:
> [...]
> 
> @@ +256,3 @@
> +    },
> +
> +    _fillParameters: function(params, bus, name, object) {
> 
> In some ways I'm sort of against the different naming schemes here. It's sort
> of weird to have a slim proxy be
> 
>     let A = new Lang.Class({
>         BusType: Gio.DBusBusType.SESSION /* I forget the actual enum name */,
>         BusName: "FooBar",
>         ObjectPath: "/Foo/Bar"
>     });
> 
> and a non-slim proxy being:
> 
>     let a = new A({
>         g_connection: Gio.DBus.session,
>         g_name: "FooBar",
>         g_object_path: "/Foo/Bar"
>     });
> 
> I'm neutral on
> 
>     let A = new Lang.Class({
>         Name: "A",
>         g_connection: Gio.DBus.session
>     });
> 
> because stylistically, it's weird. The only other idea that I have that I'm
> sort of OK on, even though it's overkill, is to implement something like this
> in Lang.Class or maybe GObject.Class:
> 
>     let A = new Lang.Class({
>         DefaultParams: {
>             g_connection: Gio.DBus.session,
>         }
>     });
> 
> Although we really shouldn't be connecting to the DBus session bus at parse
> time.

I think that with Lang.Class / GObject.Class we established the convention that Class parameters begin with a capital letter. Initially, only Interface was meant as a class parameter, since that's the only one required by the metaclass. The others were added for convenience, but I think it is more consistent to have them with the same style.
DefaultParams is interesting (and we could use g_bus_type instead of g_connection to avoid a sync dbus connection at parse time), but is it really necessary? It brings no practical advantage to the DBus bindings, and it is of little use in general, as you normally have a real _init() where you can set whatever you need.
Comment 48 Giovanni Campagna 2012-06-17 14:34:16 UTC
(In reply to comment #43)
> Review of attachment 216575 [details] [review]:
> 
> ::: modules/overrides/Gio.js
> @@ +49,3 @@
> +
> +    var signatureLength = inSignature.length;
> +    var numberArgs = sync ? signatureLength + 2 : signatureLength + 1;
> 
> Was this wrong initially? For a sync, we require 1 extra argument (callable).
> For an async, we require two (callable, async callback)

Previous bindings allowed cancellable, asyncCallback and flags, and they were all optional and could be provided in any order. New bindings require exactly one cancellable and one asyncCallback (or null for both if they're not needed).
This matches what gdbus-codegen does, and reduces the difference with introspected libraries that just export the generated objects.
Comment 49 Giovanni Campagna 2012-06-17 14:36:55 UTC
Comment on attachment 216567 [details] [review]
scanner: fix pairing of error quarks with registered enums

Attachment 216567 [details] pushed as 981f011 - scanner: fix pairing of error quarks with registered enums
Comment 50 Giovanni Campagna 2012-06-17 14:44:21 UTC
(In reply to comment #41)
> Review of attachment 216568 [details] [review]:
> 
> Considering errors are also boxeds, couldn't we just use the boxed type
> machinery for this?

No, unfortunately.
g-ir-compiler special cases GError and stores it with a different type tag, so they would not be picked up by code handling INTERFACE.
We could replace g_error_free and g_error_copy with g_boxed_free and g_boxed_copy, but while we're there, why not save one vtable call?

(As I read the g-ir-compiler code, the idea is at some point to have extra metadata for GError, such as the possible error domains)

Btw, this is the opposite of GValue, which is always a boxed, but nonetheless we have tons of code handling the impossible GI_INFO_TYPE_VALUE case)
Comment 51 Giovanni Campagna 2012-06-17 15:38:06 UTC
Created attachment 216608 [details] [review]
Gio: split GDBus implementation into helpers

Soon new API wrappers will be introduced, and this will help
reusing code.
Comment 52 Giovanni Campagna 2012-06-17 16:09:27 UTC
Created attachment 216610 [details] [review]
Gio: modernize DBus bindings

Remove tabs, use modern GError bindings, remove comments that
are no longer relevant.
Comment 53 Giovanni Campagna 2012-06-17 16:09:39 UTC
Created attachment 216611 [details] [review]
GDBus: introduce new convenience wrappers

Using the new metaclass system, introduce Gio.DBusProxyClass and
Gio.DBusImplementerClass, that allow declaring specialized proxies/
implementations for specific interfaces.
Comment 54 Giovanni Campagna 2012-06-17 16:09:49 UTC
Created attachment 216612 [details] [review]
GDBus: use gio-style asyncs for the new bindings

Split asynchronous result collection into a MethodFinish function,
so that exceptions can be caught in the usual way instead of being
passed as objects to the functions. Also, remove the flags parameter
and make the cancellable argument compulsory, matching the code
generated by gdbus-codegen.
Finally, make proxy initialization explicit, reducing the amount
of magic done by the bindings.
Comment 55 Jasper St. Pierre (not reading bugmail) 2012-06-18 07:35:03 UTC
(In reply to comment #50)
> Btw, this is the opposite of GValue, which is always a boxed, but nonetheless
> we have tons of code handling the impossible GI_INFO_TYPE_VALUE case)

Yeah, I always found that curious. I don't know if it's a scanner oversight or not.
Comment 56 Jasper St. Pierre (not reading bugmail) 2012-06-18 07:35:51 UTC
Review of attachment 216567 [details] [review]:

I also completely forgot, but could you add a scanner regression test to this as well?
Comment 57 Jasper St. Pierre (not reading bugmail) 2012-06-18 07:40:26 UTC
Review of attachment 216610 [details] [review]:

If we can break API like this, sure, why not. Marking ACAF to show that the code looks fine, but I'm not sure about policy here.

::: modules/overrides/Gio.js
@@ -289,3 @@
-		    // if one arg, we don't require the handler wrapping it
-		    // into an Array
-		    retval = [retval];

I just meant it as a flippant comment. If we can easily and readily break API like this, we should do it more often! Can we make GDBus sync method implementations take params as an array, too?

@@ +322,3 @@
     var info;
     if (interfaceInfo instanceof Gio.DBusInterfaceInfo)
+        info = interfaceInfo

Missing a semicolon.
Comment 58 Jasper St. Pierre (not reading bugmail) 2012-06-18 07:40:53 UTC
Review of attachment 216568 [details] [review]:

Sigh. Looks fine, then. Some day I gotta get that introspection cleanup done.
Comment 59 Jasper St. Pierre (not reading bugmail) 2012-06-18 07:41:17 UTC
Review of attachment 216569 [details] [review]:

::: gi/arg.c
@@ +2861,3 @@
+                        gjs_throw(context,
+                                  "Releasing a flat GValue array that was not fixed-size or was nested"
+                                  "inside another container. This is not supported (and will leak)");

It needs to be supported. You can at least handle the zero-terminated case.

@@ +3191,3 @@
         for (i = 0; i < length; i++) {
             elem.v_pointer = array[i];
+

No.
Comment 60 Jasper St. Pierre (not reading bugmail) 2012-06-18 07:41:55 UTC
Review of attachment 216608 [details] [review]:

Sure.
Comment 61 Jasper St. Pierre (not reading bugmail) 2012-06-18 07:44:21 UTC
(In reply to comment #48)
> (In reply to comment #43)
> > Review of attachment 216575 [details] [review] [details]:
> > 
> > ::: modules/overrides/Gio.js
> > @@ +49,3 @@
> > +
> > +    var signatureLength = inSignature.length;
> > +    var numberArgs = sync ? signatureLength + 2 : signatureLength + 1;
> > 
> > Was this wrong initially? For a sync, we require 1 extra argument (callable).
> > For an async, we require two (callable, async callback)
> 
> Previous bindings allowed cancellable, asyncCallback and flags, and they were
> all optional and could be provided in any order. New bindings require exactly
> one cancellable and one asyncCallback (or null for both if they're not needed).
> This matches what gdbus-codegen does, and reduces the difference with
> introspected libraries that just export the generated objects.

Sure, but we're checking for two required extra arguments for sync methods, and one extra argument for async methods. That seems wrong.
Comment 62 Jasper St. Pierre (not reading bugmail) 2012-06-18 07:55:07 UTC
(In reply to comment #47)
> I think that with Lang.Class / GObject.Class we established the convention that
> Class parameters begin with a capital letter. Initially, only Interface was
> meant as a class parameter, since that's the only one required by the
> metaclass. The others were added for convenience, but I think it is more
> consistent to have them with the same style.

I guess it depends on whether we should be consistent with Lang.Class style or GDBus style. I really think it should be the latter.

To be absolutely honest, the cleanest code is when we have no overrides at all, as that makes us better more consistent with the rest of the platform. All in all, I'm a bit on the edge of this original convenience stuff, because it's entirely undocumented. It's just some magic API layer we have in gjs. In my opinion, it doesn't belong in an override module, it belongs in imports.gdbusConvenience.

I know we had a chat on IRC recently about the overrides system and Rename to: and all those terrible fancy tricks. The more I think about it, the more I dislike it.

I guess what I'm saying is that I'd like to see us have a consistent, documented platform. To say that this new layer is a part of Gio itself is an outright lie. I think something like:

let Proxy = new Lang.Class({
    Name: 'Proxy',
    g_interface: <interface />,
    g_object_path: '/foo/bar'
});

is a lot closer to Gio than the magic works "BusType" and "ObjectPath".

(Yes, I know I added GObject.Class, shut up.)

> DefaultParams is interesting (and we could use g_bus_type instead of
> g_connection to avoid a sync dbus connection at parse time), but is it really
> necessary? It brings no practical advantage to the DBus bindings, and it is of
> little use in general, as you normally have a real _init() where you can set
> whatever you need.

Maybe we're just better off leaving out the slim proxy stuff and relying on people to provide a sane default _init in their subclasses. Especially now that we're going to remove the bad async-callback-in-constructor API and make people do an init_async on the new class manually.
Comment 63 Jasper St. Pierre (not reading bugmail) 2012-06-18 07:55:33 UTC
Review of attachment 216611 [details] [review]:

Not going to review this patch until we've decided on what we want to do with the platform here.
Comment 64 Giovanni Campagna 2012-06-18 21:07:54 UTC
Created attachment 216700 [details] [review]
scanner: complete the enum-to-error-quark fix

Turns out that the problem was not only in the wrong matching
to GType enums, but also that the non-GType heuristics used
to_underscores instead of to_underscores_noprefix, turning DBusError
into D_Bus_Error instead of DBus_Error.
Complete with various tests.
Comment 65 Giovanni Campagna 2012-06-18 21:09:07 UTC
(In reply to comment #57)
> Review of attachment 216610 [details] [review]:
> 
> If we can break API like this, sure, why not. Marking ACAF to show that the
> code looks fine, but I'm not sure about policy here.
> 
> ::: modules/overrides/Gio.js
> @@ -289,3 @@
> -            // if one arg, we don't require the handler wrapping it
> -            // into an Array
> -            retval = [retval];
> 
> I just meant it as a flippant comment. If we can easily and readily break API
> like this, we should do it more often! Can we make GDBus sync method
> implementations take params as an array, too?

I wanted to, tried, but then didn't like it. And reverted both.
Comment 66 Jasper St. Pierre (not reading bugmail) 2012-06-19 00:38:47 UTC
(In reply to comment #65)
> I wanted to, tried, but then didn't like it. And reverted both.

What didn't you like about it?
Comment 67 Jasper St. Pierre (not reading bugmail) 2012-06-19 00:42:33 UTC
Review of attachment 216700 [details] [review]:

Tests look fine. I wrote code for this to work a long time ago, which I think is a bit easier to read than the mess of substitutions:

def camel_case_match(string):
    """
    Properly matches the camelCase naming style so that a name like
    writeXMLDocument gets parsed as ["write", "XML", "Document"].
    """
    return re.findall(r'(^[a-z]+|[A-Z][a-z]+|[A-Z]+|[0-9])(?![a-z])', string)

def camel_case_convert(string):
    """
    Properly converts the camelCase naming style to underscore style so that
    writeXMLDocument gets converted to write_xml_document.
    """
    return '_'.join(s.lower() for s in camel_case_match(string))
Comment 68 Giovanni Campagna 2012-06-20 20:30:01 UTC
Comment on attachment 216700 [details] [review]
scanner: complete the enum-to-error-quark fix

Attachment 216700 [details] pushed as 64f3832 - scanner: complete the enum-to-error-quark fix
Comment 69 Giovanni Campagna 2012-06-20 20:38:32 UTC
(In reply to comment #59)
> Review of attachment 216569 [details] [review]:
> 
> ::: gi/arg.c
> @@ +2861,3 @@
> +                        gjs_throw(context,
> +                                  "Releasing a flat GValue array that was not
> fixed-size or was nested"
> +                                  "inside another container. This is not
> supported (and will leak)");
> 
> It needs to be supported. You can at least handle the zero-terminated case.

Uhm... How do you null terminate a flat GValue array? With a GValue of G_TYPE_INVALID (0)?
Does it even make sense?

(OTOH, arrays within arrays are never been supported well, both by gjs and the scanner, and should be avoided anyway)
Comment 70 Giovanni Campagna 2012-06-20 20:41:42 UTC
Attachment 216568 [details] pushed as 3b5ba32 - Complete implementation of GError marshalling
Attachment 216608 [details] pushed as cdf07b0 - Gio: split GDBus implementation into helpers
Attachment 216610 [details] pushed as 1311a11 - Gio: modernize DBus bindings

I also pushed the modernization, because I removed the non-backward
compatible part, which I didn't like not only because it is not
backward compatible, but also because it just looks bad that a method
named getFoo() returns an array.
Comment 71 Giovanni Campagna 2012-06-20 22:20:07 UTC
Created attachment 216880 [details] [review]
GDBus: introduce new convenience wrappers

Using the new metaclass system, introduce Gio.DBusProxyClass and
Gio.DBusImplementerClass, that allow declaring specialized proxies/
implementations for specific interfaces.
Comment 72 Giovanni Campagna 2012-06-20 22:21:19 UTC
Created attachment 216881 [details] [review]
GDBus: allow overriding _init in a Gio.DBusProxyClass

A Gio.DBusProxyClass is a Lang.Class and should behave like one,
including the _init handling. In particular, this allows for
"slim proxies", that can be created without passing parameters
(or with a subset of them).
Comment 73 Giovanni Campagna 2012-06-21 17:39:24 UTC
Created attachment 216948 [details] [review]
GDBus: allow overriding _init in a Gio.DBusProxyClass

A Gio.DBusProxyClass is a Lang.Class and should behave like one,
including the _init handling. In particular, this allows for
"slim proxies", that can be created without passing parameters
(or with a subset of them).
Comment 74 Giovanni Campagna 2012-06-21 17:39:42 UTC
Created attachment 216949 [details] [review]
Throw an exception when registering a GType that already exists

The GType system only logs a warning in that case, and it can
make bugs harder to track.
Comment 75 Giovanni Campagna 2012-06-21 17:40:59 UTC
Created attachment 216950 [details] [review]
Unit tests: run tests in a forked child

Tests can register types or have side effects that cannot be
easily undone by just tearing down the context. To avoid that,
run them in a forked child, so they don't interfere with each
other.
(With this, we're basically saying that you cannot have multiple
GjsContexts in one application, or even destroy one. I don't know
if anyone depended on that.)

I must say I don't like this, but didn't find a better solution:
using GTypePlugin would just leak the GjsContext, and therefore
test would fail anyway.
Comment 76 Colin Walters 2012-06-22 00:21:44 UTC
Review of attachment 216950 [details] [review]:

You need to give an example of the types and side effects you're talking about here.

But regardless, this is a really bad idea; threads and fork conflict with each other.  We use threads in GLib (and SpiderMonkey).  If I'd been paying close attention I would have strongly argued against the addition of g_test_trap_fork() to GLib.

What's particularly problematic here is that the fork() call will terminate all other threads, such as the GLib signal watcher thread, the GDBus worker threads, and also the SpiderMonkey GC thread.

If we need to avoid side effects in tests, run the code in a new process (this would be easier with GSubprocess).  Or fix the relevant code to clean up.
Comment 77 Colin Walters 2012-07-02 19:38:42 UTC
So the first patch in this series was committed as 64f3832, then reverted in e4879c84.  

This is related to bug 634202.  Let's continue discussion there.
Comment 78 Jasper St. Pierre (not reading bugmail) 2012-07-19 01:03:44 UTC
Review of attachment 216949 [details] [review]:

Sure.
Comment 79 Jasper St. Pierre (not reading bugmail) 2012-07-19 01:09:38 UTC
Review of attachment 216880 [details] [review]:

So, I think I've already made my opinion on these new wrappers fairly clear. I'd like to see the slim proxy stuff land, a bit. I'm -0 on the subclass-based exported object, as it's fairly stiff.

I would be fine if they were moved *out* of Gio overrides restoring it to its pristine behaviour, making it clear that this is *not* part of our base platform. We also need to document these wrappers. It's a lot of code, and it also involves the metaclass system. How much can we remove, or delegate to new API in Gio itself, without losing too much convenience.

Also, I swear that we fixed makeProxyWrapper so that you had to call init_async explicitly. Did I just dream that one up?

Maybe you don't frequent IRC as much as I do, but there have been a few people asking me so far where this magic is coming from, having grepped through the glib sources and not finding anything.
Comment 80 Giovanni Campagna 2012-08-07 19:22:04 UTC
Jasper, could you review the memory leak fixes again, after my replies?
Comment 81 Jasper St. Pierre (not reading bugmail) 2012-08-07 19:31:53 UTC
Review of attachment 216569 [details] [review]:

Looks fine, minus the whitespace add.
Comment 82 Giovanni Campagna 2012-08-07 19:35:05 UTC
Comment on attachment 216569 [details] [review]
Fix memory leaks

Attachment 216569 [details] pushed as 90b7edc - Fix memory leaks
Comment 83 Giovanni Campagna 2012-08-24 13:30:54 UTC
Comment on attachment 216949 [details] [review]
Throw an exception when registering a GType that already exists

Attachment 216949 [details] pushed as 0404112 - Throw an exception when registering a GType that already exists
Committing before it gets forgotten.
Comment 84 Giovanni Campagna 2012-08-24 13:35:16 UTC
Created attachment 222319 [details] [review]
GDBus: allow overriding _init in a Gio.DBusProxyClass

A Gio.DBusProxyClass is a Lang.Class and should behave like one,
including the _init handling. In particular, this allows for
"slim proxies", that can be created without passing parameters
(or with a subset of them).
This is implemented by moving the special initialization of DBusProxy
to an _init override, instead of hooking into init and init_async methods.

I had this idea last night, and I like it a lot over all previous
approaches. No hidden base classes, no weird wrapFunction or parent,
no special magic for some parameters - in fact, it even makes it possible to
install methods in GObject prototypes and call .parent() from that.
GDBus has never been cleaner.
Comment 85 Jasper St. Pierre (not reading bugmail) 2012-08-24 18:14:54 UTC
Review of attachment 222319 [details] [review]:

I don't understand what the lang.js changes do. Maybe it's because I just woke up, but it looks like it just calculates the parent dynamically instead of tracking it manually, and installing the _parent function on the prototype instead of every class (an approach that I thought we tried out and removed, so that you couldn't override parent from subclasses). Which is fine, but what's the importance?

::: modules/overrides/GObject.js
@@ +248,3 @@
     };
 
+    this.Object.prototype.parent = Lang._parent;

What's the point of this if you install it on _Base?
Comment 86 Giovanni Campagna 2012-08-25 12:01:36 UTC
(In reply to comment #85)
> Review of attachment 222319 [details] [review]:
> 
> I don't understand what the lang.js changes do. Maybe it's because I just woke
> up, but it looks like it just calculates the parent dynamically instead of
> tracking it manually, and installing the _parent function on the prototype
> instead of every class (an approach that I thought we tried out and removed, so
> that you couldn't override parent from subclasses). Which is fine, but what's
> the importance?

getSuperClass is needed because dynamic GObject classes (before or after the dynamic class rework) don't go through Lang.Class.prototype._init, so __super__ is never defined. I could have added it in C code, but why adding more hidden properties when standard JS works?
The _parent thing is cosmetic really. I don't think anyone would think of overriding .parent() in a subclass, and if he does, I hope he won't call .parent() from the overridden implementation, that would be funny.

> ::: modules/overrides/GObject.js
> @@ +248,3 @@
>      };
> 
> +    this.Object.prototype.parent = Lang._parent;
> 
> What's the point of this if you install it on _Base?

Clocks tells me it's early morning in Boston, but what's GObject.Object.prototype?
Comment 87 Jasper St. Pierre (not reading bugmail) 2012-08-25 17:03:47 UTC
Review of attachment 222319 [details] [review]:

OK, sure.
Comment 88 Giovanni Campagna 2012-08-25 17:21:53 UTC
Ok, wait, what about the rest of the new GDBus wrappers?

(and is it ok to push now, given API freeze?)
Comment 89 Giovanni Campagna 2012-08-27 20:45:24 UTC
So, from IRC discussion, the tentative plan for this bug is:

- 3.5.91: land the new wrappers as *experimental*
- 3.7.1:  new wrappers are stable
          old wrappers are deprecated, public announcement is made to ddl
          imports.dbus is removed
- 3.7.*:  gnome-shell, gnome-documents, sushi are ported to new wrappers
- 3.8.1:  old wrappers are removed
          profit

Anything else?
Comment 90 Jasper St. Pierre (not reading bugmail) 2012-09-26 19:48:40 UTC
OK, So I've thought about this for some time, and I have a new design for an API: Almost exact same as gdbus-codegen would generate.

There's a few differences, though:

 * Naming style. Swap out call_foo_bar_sync for FooBarSync (not entirely happy about this one)
 * Return values are not out params, but are kept as a raw GVariant.
 * Errors are of course converted into thrown errors.

That means, for a FooBar(in='asa{sv}', out='bb'):

    let v;
    try {
        v = proxy.FooBarSync(["one", "two"], {three: "four"}, null);
    } catch (e if e.matches(/* ... */)) {
        // ...
    }
    let [a, b] = v.deep_unpack();

    proxy.FooBar(["one", "two", {three: "four"}, null, function(proxy_obj, res) {
        let v;
        try {
            v = proxy.FooBarFinish(res);
        } catch (e if e.matches(/* ... */)) {
            // ...
        }
        let [a, b] = v.deep_unpack();
    });

This, to me, has the clear advantages that it's pretty much the same as stock GDBus. The raw GVariant is so that we can pass values to C without copying data to JS representation and back, which may be slow for cases like sending pixbufs or icons over the wire (unfortunate, but part of notification-spec).

I like your latest slim proxy approach, and I think it goes well with what we have here. I'd like to see automatic init_async dropped, so that it would need to be called explicitly.

Signals should be automatically marshalled into gjs signals, similar to how GDBus does it. Collisions are unfortunate, but that's the case in glib/gio as well, and we work fine with those.



Similarly, I now agree with you that exported objects should be handled with inheritance. It makes more sense this way, and also pairs us better with Gio/GDBus. The current implementation you have for this in your patches is fine.
Comment 91 Jasper St. Pierre (not reading bugmail) 2012-09-26 20:34:12 UTC
Whoops, forgot a bit:

For the exported objects, I think it your implementation is fine for this, minus a tiny part: I don't like the automatic marshalling of values here. API should explicitly return a GVariant. I think this is fine: Both pack() and GVariantBuilder exist.
Comment 92 Giovanni Campagna 2012-10-03 17:43:20 UTC
While I understand the concerns for performance due to unpacking heavy GVariants, I disagree that forcing everyone to unpack manually is the solution. Instead, the unpacker should recognize the byte array and use a suitable JS type (GBytes, gjs ByteArray or UInt8Array).
On the other hand, dealing with native JS types matches gdbus-codegen a lot more than using GVariants exclusively. If the automatic packing is not enough (strange, but possible), you can still mark the method Async and pass your variant to invocation.return_value()

Signals could be turned in native GObject signals, but that requires more type registration, more marshalling, and we don't usually handle GObject signals with complex signatures, so I'd rather stay with connectSignal/disconnectSignal at the JS level.

Anything else, including the automatic init_async, is part of the latest round of patches.

Thinking of it, should we also have .new_sync() and .new() constructors for proxies, in the style of gdbus-codegen? At least, it would avoid the explicit .init() and the horrible .init_async(GLib.PRIORITY_DEFAULT, ...)
Comment 93 Jasper St. Pierre (not reading bugmail) 2012-10-03 18:07:19 UTC
(In reply to comment #92)
> While I understand the concerns for performance due to unpacking heavy
> GVariants, I disagree that forcing everyone to unpack manually is the solution.
> Instead, the unpacker should recognize the byte array and use a suitable JS
> type (GBytes, gjs ByteArray or UInt8Array).

We already copy to a gjs ByteArray. That's two sets of copies already, and if we want to pass it to C (e.g. load_from_raw) that's another.

I have a WIP patch (it crashes) to turn ByteArray into a hybrid, using GBytes as backing storage for the simple case, and copy it when we start mutating it. If I can finish it off and make it stop crashing, that sounds like a viable approach for the future.

I also don't think unpacking manually is that bad. In the simple case, it just requires adding a .deep_unpack(). I think it's extremely worth it, and matches our Async style.

(In reply to comment #92)
> On the other hand, dealing with native JS types matches gdbus-codegen a lot
> more than using GVariants exclusively. If the automatic packing is not enough
> (strange, but possible), you can still mark the method Async and pass your
> variant to invocation.return_value()

Yeah, it's strange, but I think it's worth it for performance reasons.

> Signals could be turned in native GObject signals, but that requires more type
> registration, more marshalling, and we don't usually handle GObject signals
> with complex signatures, so I'd rather stay with connectSignal/disconnectSignal
> at the JS level.

er, you didn't understand what I meant, I guess. I meant to just do what we're doing now wrt. JS signals, but invert things: connectSignal becomes connect, and GObject signals become connectGObjectSignal, I guess. GObject signals aren't going to be used very often.

> Thinking of it, should we also have .new_sync() and .new() constructors for
> proxies, in the style of gdbus-codegen? At least, it would avoid the explicit
> .init() and the horrible .init_async(GLib.PRIORITY_DEFAULT, ...)

We have this style already, and it's a bit strange and ugly, requiring wrapper functions to set various things, right?
Comment 94 Jasper St. Pierre (not reading bugmail) 2012-10-03 18:50:46 UTC
(In reply to comment #93)
> I have a WIP patch (it crashes) to turn ByteArray into a hybrid, using GBytes
> as backing storage for the simple case, and copy it when we start mutating it.
> If I can finish it off and make it stop crashing, that sounds like a viable
> approach for the future.

=> bug #685431
Comment 95 Giovanni Campagna 2012-10-18 14:29:59 UTC
Ok, so 3.6.1 is out, let's wrap this up and come with something that we can ship as 3.7.1.

First, the client side. I think we agree on the style:
const MyClass = new Gio.DBusProxyClass({ Interface: ... })
(or const MyClass = new Lang.Class({ Extends: Gio.DBusProxy })).
let proxy = new MyClass({ g_name, g_object_path, g_bus_type })
proxy.init();

This almost completely matches the Gio API, except that you don't pass g_interface_info to MyClass().

proxy.FooBar(function() { proxy.FooBarFinish(); });
let result = proxy.FooBarSync(params);

Now the problematic parts: what's result and what are params?
My idea, looking at gdbus-codegen and the existing dbus bindings, is
let [outArg1, outArg2] = proxy.FooBarSync(inArg1, inArg2);
Your idea, if I understand it correctly, is:
let outArgsAsVariantTuple = proxy.FooBarSync(inArgsAsVariantTuple);

A possible compromise is to look even more closely at gdbus-codegen, and adopt 'org.gtk.GDBus.C.ForceGVariant' as an annotation that would skip automatic packing/unpacking of GVariants.
So this:
<method name="DoSomething">
  <arg type="s" direction="in"/>
  <arg type="u" direction="in"/>
  <arg type="b" direction="out"/>
  <arg type="ay" direction="out">
    <annotation name="org.gtk.GDBus.C.ForceGVariant" value="true"/>
  </arg>
</method>

would become
let [ok, byteArrayAsGVariant] = proxy.DoSomethingSync('string', 42);

A similar approach could do for server side, where we would have:

class MyService = new Gio.DBusImplementerClass({
    DoSomething: function(string, integer) {
        return [true, new GLib.Variant('ay', [1, 2, 3]);
    }
});

This has the advantages of both worlds: it avoids unpacking the arguments manually in the common case (and makes the code cleaner, by reducing the number of calls to "new GLib.Variant"), but it prevents costly copies between JS and C for large binary blobs.

As for the next point of contention, i.e. signals, I think we should modify .connect/.disconnect from their normal GObject behavior. Gio.DBusProxy is-a GObject.Object and it is a fundamental OOP principle that methods of a superclass act compatibly when invoked on subclass.
Also, I think that what we have with .connectSignal, while somehow dbus-glib-like, is not a bad API to use.

Finally, for the question of constructors, I think we can land the proxies without initialization now, and add the new API later as a convenience layer, if needed.
Comment 96 Jasper St. Pierre (not reading bugmail) 2012-10-18 14:45:09 UTC
(In reply to comment #95)
> Ok, so 3.6.1 is out, let's wrap this up and come with something that we can
> ship as 3.7.1.
> 
> First, the client side. I think we agree on the style:
> const MyClass = new Gio.DBusProxyClass({ Interface: ... })
> (or const MyClass = new Lang.Class({ Extends: Gio.DBusProxy })).
> let proxy = new MyClass({ g_name, g_object_path, g_bus_type })
> proxy.init();

Not sure what "Interface" is, and it conflicts to me with GInterface, which is represented by "Implements".

> This almost completely matches the Gio API, except that you don't pass
> g_interface_info to MyClass().

That seems like a better idea instead.

> proxy.FooBar(function() { proxy.FooBarFinish(); });
> let result = proxy.FooBarSync(params);
> 
> Now the problematic parts: what's result and what are params?
> My idea, looking at gdbus-codegen and the existing dbus bindings, is
> let [outArg1, outArg2] = proxy.FooBarSync(inArg1, inArg2);
> Your idea, if I understand it correctly, is:
> let outArgsAsVariantTuple = proxy.FooBarSync(inArgsAsVariantTuple);
> 
> A possible compromise is to look even more closely at gdbus-codegen, and adopt
> 'org.gtk.GDBus.C.ForceGVariant' as an annotation that would skip automatic
> packing/unpacking of GVariants.
> So this:
> <method name="DoSomething">
>   <arg type="s" direction="in"/>
>   <arg type="u" direction="in"/>
>   <arg type="b" direction="out"/>
>   <arg type="ay" direction="out">
>     <annotation name="org.gtk.GDBus.C.ForceGVariant" value="true"/>
>   </arg>
> </method>
> 
> would become
> let [ok, byteArrayAsGVariant] = proxy.DoSomethingSync('string', 42);

That seems nifty.

> A similar approach could do for server side, where we would have:
> 
> class MyService = new Gio.DBusImplementerClass({
>     DoSomething: function(string, integer) {
>         return [true, new GLib.Variant('ay', [1, 2, 3]);
>     }
> });
> 
> This has the advantages of both worlds: it avoids unpacking the arguments
> manually in the common case (and makes the code cleaner, by reducing the number
> of calls to "new GLib.Variant"), but it prevents costly copies between JS and C
> for large binary blobs.
> 
> As for the next point of contention, i.e. signals, I think we should modify
> .connect/.disconnect from their normal GObject behavior. Gio.DBusProxy is-a
> GObject.Object and it is a fundamental OOP principle that methods of a
> superclass act compatibly when invoked on subclass.
> Also, I think that what we have with .connectSignal, while somehow
> dbus-glib-like, is not a bad API to use.

We should reverse it, so that .connect is GObject, .connectDBusSignal is DBus.

> Finally, for the question of constructors, I think we can land the proxies
> without initialization now, and add the new API later as a convenience layer,
> if needed.

Isn't that used for the new style of slim proxies?

Also, considering this is now matching gdbus-codegen, I wonder if this should be imports.gdbus or imports.gdbusCodegen or similar. Overrides are designed to be unfortunate but pragmatic compromises in the case of gobject-introspection deficiencies (varargs in pygobject, etc.), not a glue layer as we've discussed before.
Comment 97 Jasper St. Pierre (not reading bugmail) 2012-11-01 21:47:22 UTC
Ping on this? I'd like to land this this cycle.
Comment 98 Giovanni Campagna 2012-11-01 22:00:02 UTC
(In reply to comment #96)
> (In reply to comment #95)
> [...]
> 
> Not sure what "Interface" is, and it conflicts to me with GInterface, which is
> represented by "Implements".

Well, Interface is kind of the point of Gio.DBusProxyClass as a metaclass. It's the thing that makes methods, properties and signals magically appear in the new class.

> > This almost completely matches the Gio API, except that you don't pass
> > g_interface_info to MyClass().
> 
> That seems like a better idea instead.

What's the point of deriving from Gio.DBusProxy, if the API is exactly that of Gio.DBusProxy?

> > [...]
> > 
> > would become
> > let [ok, byteArrayAsGVariant] = proxy.DoSomethingSync('string', 42);
> 
> That seems nifty.

TODO, then.
But can we land the deep_unpack()ing part now, and read the annotations later?

> [...]
> 
> We should reverse it, so that .connect is GObject, .connectDBusSignal is DBus.

It is .connect() for GObject, and .connectSignal() for DBus. It has always been.

> > Finally, for the question of constructors, I think we can land the proxies
> > without initialization now, and add the new API later as a convenience layer,
> > if needed.
> 
> Isn't that used for the new style of slim proxies?
> 
> Also, considering this is now matching gdbus-codegen, I wonder if this should
> be imports.gdbus or imports.gdbusCodegen or similar. Overrides are designed to
> be unfortunate but pragmatic compromises in the case of gobject-introspection
> deficiencies (varargs in pygobject, etc.), not a glue layer as we've discussed
> before.

Well, GDBus bindings have lived in Gio until now, and Gio.DBusProxyClass is a __metaclass__ on Gio.DBusProxy. Also, Gio.DBusExportedObject / Gio.DBusImplementerClass are an unfortunate but pragmatic compromise for GDBusInterfaceVTable.
Btw, pygobject implements this in the Gio overrides, although their layer is smaller because they have __getattr__.
Comment 99 Giovanni Campagna 2012-11-02 14:44:48 UTC
Created attachment 227894 [details] [review]
GDBus2: support org.gtk.GDBus.C.ForceGVariant annotation

This annotation can be applied to an in or out argument, and skips
the default GVariant marshalling, passing the argument variant as-is
to the method proxy and implementation.
It is mostly useful for binary data, to avoid the extra conversion
step.
Comment 100 Jasper St. Pierre (not reading bugmail) 2012-12-03 19:19:34 UTC
Now that we force GBytes on ay which doesn't require a copy in the case of binary data, the GVariant is only a fanciness now.
Comment 101 Jasper St. Pierre (not reading bugmail) 2013-04-01 23:26:28 UTC
What in here needs to land in order to get this done? Can I see a reattached patch stack?
Comment 102 Giovanni Campagna 2013-04-12 15:31:02 UTC
Created attachment 241360 [details] [review]
GDBus: introduce new convenience wrappers

Using the new metaclass system, introduce Gio.DBusProxyClass and
Gio.DBusImplementerClass, that allow declaring specialized proxies/
implementations for specific interfaces.
Comment 103 Giovanni Campagna 2013-04-12 15:31:33 UTC
Created attachment 241361 [details] [review]
GDBus: allow overriding _init in a Gio.DBusProxyClass

A Gio.DBusProxyClass is a Lang.Class and should behave like one,
including the _init handling. In particular, this allows for
"slim proxies", that can be created without passing parameters
(or with a subset of them).
This is implemented by moving the special initialization of DBusProxy
to an _init override, instead of hooking into init and init_async methods.
Comment 104 Giovanni Campagna 2013-04-12 15:31:40 UTC
Created attachment 241362 [details] [review]
GObject: fix building a class with non trivial accessor properties

The accessors would be invoked while traversing the prototype for vfuncs.
Comment 105 Giovanni Campagna 2013-04-12 15:31:46 UTC
Created attachment 241363 [details] [review]
GDBus2: support org.gtk.GDBus.C.ForceGVariant annotation

This annotation can be applied to an in or out argument, and skips
the default GVariant marshalling, passing the argument variant as-is
to the method proxy and implementation.
It is mostly useful for binary data, to avoid the extra conversion
step.
Comment 106 Giovanni Campagna 2013-04-12 15:32:11 UTC
Created attachment 241364 [details] [review]
GDBus: use gio-style asyncs for the new bindings

Split asynchronous result collection into a MethodFinish function,
so that exceptions can be caught in the usual way instead of being
passed as objects to the functions. Also, remove the flags parameter
and make the cancellable argument compulsory, matching the code
generated by gdbus-codegen.
Finally, make proxy initialization explicit, reducing the amount
of magic done by the bindings.
Comment 107 Giovanni Campagna 2015-04-27 02:05:48 UTC
A few years later, and several GNOME versions later, the gjs GDBus bindings are what they are, and people are used to them.
These were never reviewed or merged, and gjs is in deep maintenance mode due to a lack of manpower. Closing because there is little point in keeping this open.