GNOME Bugzilla – Bug 669350
GDBus: introduce new convenience wrappers
Last modified: 2015-04-27 02:05:48 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.
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.
Created attachment 206737 [details] [review] GDBus: fix tests I like the new names better.
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.
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.
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.
(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.
(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.
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.
Review of attachment 206930 [details] [review]: I'd say it looks good in general.
(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.
(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/
(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.
(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.
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.
(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.
(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.
(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'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).
(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.
How do you guys feel about getting this in for 3.6?
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.
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.
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
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.
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
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.
Created attachment 216462 [details] [review] Gio: split GDBus implementation into helpers Soon new API wrappers will be introduced, and this will help reusing code.
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.
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.
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.
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.
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"
(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)
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.
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.
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.
Created attachment 216570 [details] [review] Gio: split GDBus implementation into helpers Soon new API wrappers will be introduced, and this will help reusing code.
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.
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.
Review of attachment 216567 [details] [review]: Makes sense to me.
Review of attachment 216568 [details] [review]: Considering errors are also boxeds, couldn't we just use the boxed type machinery for this?
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.
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.
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.
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.
(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.
(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 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
(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)
Created attachment 216608 [details] [review] Gio: split GDBus implementation into helpers Soon new API wrappers will be introduced, and this will help reusing code.
Created attachment 216610 [details] [review] Gio: modernize DBus bindings Remove tabs, use modern GError bindings, remove comments that are no longer relevant.
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.
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.
(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.
Review of attachment 216567 [details] [review]: I also completely forgot, but could you add a scanner regression test to this as well?
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.
Review of attachment 216568 [details] [review]: Sigh. Looks fine, then. Some day I gotta get that introspection cleanup done.
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.
Review of attachment 216608 [details] [review]: Sure.
(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.
(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.
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.
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.
(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.
(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?
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 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
(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)
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.
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.
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).
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).
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.
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.
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.
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.
Review of attachment 216949 [details] [review]: Sure.
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.
Jasper, could you review the memory leak fixes again, after my replies?
Review of attachment 216569 [details] [review]: Looks fine, minus the whitespace add.
Comment on attachment 216569 [details] [review] Fix memory leaks Attachment 216569 [details] pushed as 90b7edc - Fix memory leaks
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.
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.
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?
(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?
Review of attachment 222319 [details] [review]: OK, sure.
Ok, wait, what about the rest of the new GDBus wrappers? (and is it ok to push now, given API freeze?)
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?
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.
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.
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, ...)
(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?
(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
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.
(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.
Ping on this? I'd like to land this this cycle.
(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__.
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.
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.
What in here needs to land in order to get this done? Can I see a reattached patch stack?
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.
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.
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.
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.
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.
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.