GNOME Bugzilla – Bug 648651
Port to GDBus (for real)
Last modified: 2011-11-11 16:16:22 UTC
For bug 621203, we need to kill off dbus-glib and move to GDBus. But the patches there are not enough, the whole shell must be ported as a whole. Given bug 622921 introducing GDBus to GJS, it's time to move on. (Partly is also an evaluation of the GDBus JS binding in a real application)
Created attachment 186626 [details] [review] Port to new GDBus bindings in GJS Rewrite all DBus usage in JS to use the new syntax and style for bindings (in particular for describing interfaces). Remove all usage of dbus-glib in C code (except for gdmuser, which is shared code). dbus-glib is still used by telepathy-glib, libnm-glib, libgnome-bluetooth, so names/objects exposed by those libraries are on a different connection.
http://git.gnome.org/browse/gnome-shell/commit/?id=153768ef7ffbea5760bcd7611aa75aef71310ba5 added a new DBus usage that will need to be included in this patch on the next rebase. There is a TODO comment in the patch describing how GDBus implementation should act.
Review of attachment 186626 [details] [review]: Ok, I think this is good in general. I went ahead and split off the .c parts (and fixed some style bits in the shell-app-usage.c patch). Let's try splitting this up more? There are some conflicts, but some trivial parts can be committed now.
Created attachment 193936 [details] [review] Port to new GDBus bindings in gjs Rewrite code acquiring dbus names so that it uses GDBus, and rewrite ShellDBus so that it is exposed on the GDBus connection. Ports of the other objects will follow.
Created attachment 193937 [details] [review] screensaver, gnomesession: port to GDBus based bindings Port org.gnome.ScreenSaver and org.gnome.SessionManager glue code to use GDBus, and move /org/gnome/Shell/EndSessionDialog to the GDBus connection, so it is backed by the org.gnome.Shell name.
Created attachment 193938 [details] [review] notificationDaemon, magnifierDBus: port to GDBus Move /org/freedesktop/Notifications and /org/gnome/Magnifier to the GDBus connection, so they're matched with the appropriate DBus name.
Created attachment 193939 [details] [review] Port client side code to GDBus This continues the series of patches for GDBus porting, affecting all code that accesses remote DBus objects. This includes modemManager, automount, autorun (for the hotplug sniffer), calendar, network (for nm-applet only), power, scripting (for perf monitor interface)
Created attachment 193940 [details] [review] Remove all stray references to imports.dbus Some modules where importing dbus without actually using it.
Created attachment 194036 [details] [review] screensaver, gnomesession: port to GDBus based bindings Port org.gnome.ScreenSaver and org.gnome.SessionManager glue code to use GDBus, and move /org/gnome/Shell/EndSessionDialog to the GDBus connection, so it is backed by the org.gnome.Shell name.
Created attachment 194037 [details] [review] Port client side code to GDBus This continues the series of patches for GDBus porting, affecting all code that accesses remote DBus objects. This includes modemManager, automount, autorun (for the hotplug sniffer), calendar, network (for nm-applet only), power, scripting (for perf monitor interface)
Created attachment 194038 [details] [review] notificationDaemon, magnifierDBus: port to GDBus Move /org/freedesktop/Notifications and /org/gnome/Magnifier to the GDBus connection, so they're matched with the appropriate DBus name.
Review of attachment 193936 [details] [review]: ::: js/ui/shellDBus.js @@ +28,3 @@ +<property name="ApiVersion" type="i" access="read" /> +<property name="ShellVersion" type="s" access="read" /> +</interface> ; (also, is there any reason we're using E4X now instead of JSON?) ::: src/main.c @@ +65,3 @@ if (replace) + request_name_flags |= G_BUS_NAME_OWNER_FLAGS_REPLACE; + if (!(request_name_variant = g_dbus_proxy_call_sync (bus, why not just use g_bus_own_name() ?
Review of attachment 193940 [details] [review]: sure.
Review of attachment 194036 [details] [review]: If you didn't know, and we don't mind using it, Mozilla's JS supports destructuring assignment in function signatures too: function(owner, [foo, bar], error) { log(bar + ", " + baz); } It may come in handy here. Destructuring assignment is pretty nifty: function thing({foo: foo, bar: [a, b, {result: c}]}) { log([foo, a, b, c]); } thing(owner, {foo: "Foo!", bar: [1, 2, {result: 15}]}); // Foo!,1,2,15 ::: js/misc/screenSaver.js @@ +15,3 @@ + <arg type="b" direction="out" /> +</signal> +</interface> ; @@ +28,3 @@ + Gio.DBusProxyFlags.DO_NOT_LOAD_PROPERTIES) }); + self.init(null); + self.screenSaverActive = false; I'm not sure I like the pattern of setting random properties on objects from introspection (sure, it's really an override, but that could change). Also, I feel a little dirty depending on the constructor returning something here.
Review of attachment 194037 [details] [review]: Watch your semis after iface XML. Otherwise good. ::: js/ui/automountManager.js @@ +38,3 @@ + var self = new Gio.DBusProxy({ g_connection: Gio.DBus.system, + g_interface_name: ConsoleKitManagerInfo.name, + g_interface_info: ConsoleKitManager, You mean ConsoleKitManagerInfo? @@ +60,3 @@ + }); + }); + } else style nit: either both sides of an if have braces, or neither do. @@ +68,1 @@ }; No semi here. ::: js/ui/scripting.js @@ +86,1 @@ }; No semi. ::: js/ui/status/power.js @@ +52,3 @@ +</interface> + +let PowerManagerProxy = Gio.DBusProxy.makeProxyWrapper(PowerManagerInterface); You use "const" elsewhere else for proxies.
Review of attachment 194038 [details] [review]: semis, otherwise fine. ::: js/ui/notificationDaemon.js @@ +459,3 @@ + Gio.BusNameWatcherFlags.NONE, + null, + Lang.bind(this, this._onNameVanished)); Please define this._watchNameId if there is no sender.
(In reply to comment #12) > Review of attachment 193936 [details] [review]: > > ::: js/ui/shellDBus.js > @@ +28,3 @@ > +<property name="ApiVersion" type="i" access="read" /> > +<property name="ShellVersion" type="s" access="read" /> > +</interface> > > ; > > (also, is there any reason we're using E4X now instead of JSON?) There is a reason we're using XML: that's what GDBus uses in GDBusInterfaceInfo (which is required for most of auto stuff to work properly). There is no particular reason to use E4X over plain XML strings, except that we catch syntax errors earlier. > ::: src/main.c > @@ +65,3 @@ > if (replace) > + request_name_flags |= G_BUS_NAME_OWNER_FLAGS_REPLACE; > + if (!(request_name_variant = g_dbus_proxy_call_sync (bus, > > why not just use g_bus_own_name() ? g_bus_own_name is asynchronous, and this has problems with gnome-session thinking that gnome-shell is started even when it's not. (In reply to comment #14) > Review of attachment 194036 [details] [review]: > > If you didn't know, and we don't mind using it, Mozilla's JS supports > destructuring assignment in function signatures too: > > function(owner, [foo, bar], error) { > log(bar + ", " + baz); > } > > It may come in handy here. I didn't know of that. It will surely help. > @@ +28,3 @@ > + > Gio.DBusProxyFlags.DO_NOT_LOAD_PROPERTIES) }); > + self.init(null); > + self.screenSaverActive = false; > > I'm not sure I like the pattern of setting random properties on objects from > introspection (sure, it's really an override, but that could change). > > Also, I feel a little dirty depending on the constructor returning something > here. The alternative would be a JS object proxying all method calls and signals. Or a JS object inheriting from GDBusProxy. The first one is even dirtier, the second one is not supported by gjs (yet).
Created attachment 194403 [details] [review] Port to new GDBus bindings in gjs Rewrite code acquiring dbus names so that it uses GDBus, and rewrite ShellDBus so that it is exposed on the GDBus connection. Ports of the other objects will follow.
Created attachment 194404 [details] [review] screensaver, gnomesession: port to GDBus based bindings Port org.gnome.ScreenSaver and org.gnome.SessionManager glue code to use GDBus, and move /org/gnome/Shell/EndSessionDialog to the GDBus connection, so it is backed by the org.gnome.Shell name.
Created attachment 194405 [details] [review] notificationDaemon, magnifierDBus: port to GDBus Move /org/freedesktop/Notifications and /org/gnome/Magnifier to the GDBus connection, so they're matched with the appropriate DBus name.
Created attachment 194406 [details] [review] Port client side code to GDBus This continues the series of patches for GDBus porting, affecting all code that accesses remote DBus objects. This includes modemManager, automount, autorun (for the hotplug sniffer), calendar, network (for nm-applet only), power, scripting (for perf monitor interface)
Created attachment 194407 [details] [review] Remove all stray references to imports.dbus Some modules where importing dbus without actually using it.
Review of attachment 194407 [details] [review]: Good
Created attachment 197833 [details] [review] Remove all stray imports to imports.dbus Some modules were importing DBus without actually using it. You missed a couple.
Review of attachment 194405 [details] [review]: ::: js/ui/magnifierDBus.js @@ +2,3 @@ +const Gio = imports.gi.Gio; +const GLib = imports.gi.GLib; Unused. @@ +12,3 @@ // Subset of gnome-mag's Magnifier dbus interface -- to be expanded. See: // http://git.gnome.org/browse/gnome-mag/tree/xml/...Magnifier.xml +const MagnifierIface = <interface name={MAG_SERVICE_NAME}> You don't use this kind of interpolation anywhere else. Be consistent. ::: js/ui/notificationDaemon.js @@ +457,3 @@ this._pid = pid; if (sender) + this._nameWatcherId = Gio.DBus.session.watch_name(sender, You need to define this in the constructor.
Review of attachment 194404 [details] [review]: ::: js/misc/gnomeSession.js @@ +5,3 @@ const Signals = imports.signals; +const PresenceIface = <interface name="org.gnome.SessionManager.Presence"> Is xgettext smart enough to parse E4X? My bet is on "no". ::: js/misc/screenSaver.js @@ +28,3 @@ + Gio.DBusProxyFlags.DO_NOT_LOAD_PROPERTIES) }); + self.init(null); + self.screenSaverActive = false; The shell doesn't really use expando properties like this very often. I'm not sure why we should break the rule now. ::: js/ui/automountManager.js @@ +88,3 @@ this._ssProxy = new ScreenSaver.ScreenSaverProxy(); + this._ssProxy.connectSignal('ActiveChanged', + Lang.bind(this, this._screenSaverActiveChanged)); Why? ::: js/ui/endSessionDialog.js @@ +500,3 @@ + if (!(this._type in DialogContent)) { + invocation.report_dbus_error('org.gnome.Shell.ModalDialog.TypeError', + "Unknown dialog type requested"); I know this isn't your error, but should we be translating this string? @@ +507,3 @@ + let inhibitor = new GnomeSession.Inhibitor(inhibitorObjectPaths[i], Lang.bind(this, function(proxy, error) { + this._onInhibitorLoaded(proxy); + })); I'm not sure why we have both a callback and an 'is-loaded' signal. ::: js/ui/statusMenu.js @@ +1,1 @@ /* -*- mode: js2; js2-basic-offset: 4; indent-tabs-mode: nil -*- */ statusMenu doesn't exist anymore. This needs a rebase.
Review of attachment 194406 [details] [review]: ::: js/ui/status/power.js @@ +3,2 @@ const Gio = imports.gi.Gio; +const GLib = imports.gi.GLib; Unused.
Review of attachment 197833 [details] [review]: I'm assuming you verified this - if so, looks obviously correct.
Comment on attachment 197833 [details] [review] Remove all stray imports to imports.dbus Attachment 197833 [details] pushed as 463e091 - Remove all stray imports to imports.dbus I run with these sorts of patches every day. It's verified.
Created attachment 201089 [details] [review] Port to new GDBus bindings in gjs Rebased to master
Review of attachment 201089 [details] [review]: ::: configure.ac @@ +82,2 @@ # Collect more than 20 libraries for a prize! +PKG_CHECK_MODULES(GNOME_SHELL, gio-unix-2.0 >= $GIO_MIN_VERSION Why are we removing the check for glib? ::: js/ui/main.js @@ +163,3 @@ + // Force a connection now (it is equivalent to g_bus_get_sync(G_BUS_TYPE_SESSION) + Gio.DBus.session; I don't like this. Something a bit more explicit would be better. ::: js/ui/shellDBus.js @@ +161,3 @@ + let dbusObj = this.GetExtensionInfo(uuid); + out[uuid] = dbusObj; + } You forgot to return out; @@ +177,3 @@ + break; + case 'number': + type = 'u'; We can't have signed numbers?
Created attachment 201101 [details] [review] screensaver, gnomesession: port to GDBus based bindings Rebased to master
(In reply to comment #31) > Review of attachment 201089 [details] [review]: > > ::: configure.ac > @@ +82,2 @@ > # Collect more than 20 libraries for a prize! > +PKG_CHECK_MODULES(GNOME_SHELL, gio-unix-2.0 >= $GIO_MIN_VERSION > > Why are we removing the check for glib? It's pointless...they're shipped together, and using gio implies using glib.
(In reply to comment #31) > > + // Force a connection now (it is equivalent to > g_bus_get_sync(G_BUS_TYPE_SESSION) > + Gio.DBus.session; > > I don't like this. Something a bit more explicit would be better. It actually wasn't necessary now that we connect in main.c. > ::: js/ui/shellDBus.js > @@ +161,3 @@ > + let dbusObj = this.GetExtensionInfo(uuid); > + out[uuid] = dbusObj; > + } > > You forgot to return out; Fixed. > > @@ +177,3 @@ > + break; > + case 'number': > + type = 'u'; > > We can't have signed numbers? Good point - I made this a double, which is about the best we can do.
Created attachment 201172 [details] [review] Port client side code to GDBus Rebased to master. Only lightly tested (I can log in, do some basic stuff, lock the screen).
(In reply to comment #26) > > ::: js/misc/screenSaver.js > @@ +28,3 @@ > + > Gio.DBusProxyFlags.DO_NOT_LOAD_PROPERTIES) }); > + self.init(null); > + self.screenSaverActive = false; > > The shell doesn't really use expando properties like this very often. I'm not > sure why we should break the rule now. So I tried to rearrange the code to not do this, but honestly the result was worse. The *real* problem is that it's still too verbose to use GDBus even from JS, so we end up with half-baked wrappers. In the big picture what I want is a system that returns you the one canonical proxy object for a given well-known service (org.gnome.Screensaver etc.). > > ::: js/ui/automountManager.js > @@ +88,3 @@ > this._ssProxy = new ScreenSaver.ScreenSaverProxy(); > + this._ssProxy.connectSignal('ActiveChanged', > + Lang.bind(this, > this._screenSaverActiveChanged)); > > Why? https://mail.gnome.org/archives/commits-list/2011-February/msg09099.html > ::: js/ui/endSessionDialog.js > @@ +500,3 @@ > + if (!(this._type in DialogContent)) { > + > invocation.report_dbus_error('org.gnome.Shell.ModalDialog.TypeError', > + "Unknown dialog type requested"); > > I know this isn't your error, but should we be translating this string? Nah...we don't translate GErrors, and this is the same except it's a dbus error. > @@ +507,3 @@ > + let inhibitor = new > GnomeSession.Inhibitor(inhibitorObjectPaths[i], Lang.bind(this, function(proxy, > error) { > + this._onInhibitorLoaded(proxy); > + })); > > I'm not sure why we have both a callback and an 'is-loaded' signal. Oh this patch actually broke the logout - there is no is-loaded anymore. Fixed. > ::: js/ui/statusMenu.js > @@ +1,1 @@ > /* -*- mode: js2; js2-basic-offset: 4; indent-tabs-mode: nil -*- */ > > statusMenu doesn't exist anymore. This needs a rebase. (Did this already)
Review of attachment 201172 [details] [review]: Getting better... ::: js/ui/calendar.js @@ +216,3 @@ + Gio.DBusProxyFlags.DO_NOT_LOAD_PROPERTIES) }); + + self.init(null); What is the init call here for? ::: js/ui/status/network.js @@ +2,3 @@ const ByteArray = imports.byteArray; const GLib = imports.gi.GLib; +const Gio = imports.gi.Gio; Why are we adding an unused import? ::: js/ui/status/power.js @@ +3,2 @@ const Gio = imports.gi.Gio; +const GLib = imports.gi.GLib; Unused GLib import?
(In reply to comment #37) > Review of attachment 201172 [details] [review]: > > Getting better... > > ::: js/ui/calendar.js > @@ +216,3 @@ > + > Gio.DBusProxyFlags.DO_NOT_LOAD_PROPERTIES) }); > + > + self.init(null); > > What is the init call here for? GDBusProxy implements GAsyncInitable. So we need to either call _init or _async_init(). Things going via makeProxyWrapper do this automatically, but it looks like since that doesn't have support for flags, we make a proxy directly and thus need to init it.
Attachment 194405 [details] pushed as 827bf50 - notificationDaemon, magnifierDBus: port to GDBus Attachment 201089 [details] pushed as 5350302 - Port to new GDBus bindings in gjs Attachment 201101 [details] pushed as adc187c - screensaver, gnomesession: port to GDBus based bindings Attachment 201172 [details] pushed as 6547f75 - Port client side code to GDBus