GNOME Bugzilla – Bug 736337
endSessionDialog: Port offline updates to PackageKit's dbus interface
Last modified: 2014-09-11 14:15:24 UTC
Hughsie has been working on getting rid of the /usr/libexec/pk-trigger-offline-update helper that gnome-shell uses through pkexec. In the upcoming PackageKit 1.0.0 release this week, PK is going to have a replacement DBus API for doing the same thing. If we can pull off syncronized gnome-shell and PackageKit updates (which I think we can, since offline updates are only currently used in Fedora and nowhere else), PK would like to drop the pkexec helpers in the 1.0.0 release. Would be awesome if this patch could make it in next week's 3.13.92 release.
Created attachment 285745 [details] [review] endSessionDialog: Port offline updates to PackageKit's dbus interface
Review of attachment 285745 [details] [review]: ::: js/ui/endSessionDialog.js @@ +260,3 @@ + this._pkOfflineProxy = new PkOfflineProxy(Gio.DBus.system, + 'org.freedesktop.PackageKit', + '/org/freedesktop/PackageKit'); You are adding a hard dependency on PackageKit here - if PackageKit is not available, this call will first block until timing out, then fail with an exception
I've tested this, it doesn't block without PackageKit -- shutdown and reboot still work fine. Looking at what Gio.DBusProxy.makeProxyWrapper() does, it seems to call the async g_dbus_proxy_new constructor, which would explains why it didn't block in my testing.
(In reply to comment #3) > Looking at what Gio.DBusProxy.makeProxyWrapper() does, it seems to call the > async g_dbus_proxy_new constructor Look closer :-) makeProxyWrapper does initialize the proxy asynchronously *if* you provide a callback[0], but you are not doing that - that's why you end up blocking ... [0] https://git.gnome.org/browse/gjs/tree/modules/overrides/Gio.js#n193
Ah indeed, you are right of course :) Here comes an updated patch. Also, if you have any comments or questions about the PK dbus interface, it can all be changed at this point, feel free to go crazy nitpicking.
Created attachment 285818 [details] [review] endSessionDialog: Port offline updates to PackageKit's dbus interface
Review of attachment 285818 [details] [review]: Now that you asked ... ::: js/ui/endSessionDialog.js @@ +157,3 @@ +<interface name="org.freedesktop.PackageKit.Offline"> \ + <property name="UpdatePrepared" type="b" access="read"/> \ + <property name="TriggerAction" type="s" access="read"/> \ The new test for updateAlreadyTriggered is a bit awkward (you have to test for !undefined and know that its value before a call to Trigger is "unset" (rather than "unknown" as the PK enum or the empty string)) - in particular when compared to updatePrepared, which is just completely clear and obvious. So if nobody else is using that API (a quick grep in Software didn't turn up anything), could this be another boolean like "UpdateTriggered" or so? @@ +722,3 @@ this._loadSessions(); + let updateAlreadyTriggered = this._pkOfflineProxy.TriggerAction && this._pkOfflineProxy.TriggerAction != "unset"; If we don't change to a boolean, I wonder if "this._pkgOfflineProxy.TriggerAction == 'power-off' || this._pkOfflineProxy.TriggerAction == 'reboot'" is clearer ... (Also style nitpick: single quotes for non-translatable strings)
fmuellner and hughsie and I discussed this on IRC and the verdict was that keeping the TriggerAction API makes sense even though it's largely unused right now. So let's go with explicit 'power-off' and 'reboot', seems a lot cleaner than the current "unset" checking. Thanks for the thorough review by the way, much appreciated!
Created attachment 285907 [details] [review] endSessionDialog: Port offline updates to PackageKit's dbus interface
Created attachment 285912 [details] [review] endSessionDialog: Port offline updates to PackageKit's dbus interface
Review of attachment 285912 [details] [review]: Untested on my side, but code looks good to me (besides a very minor style nit) ::: js/ui/endSessionDialog.js @@ +271,3 @@ + if (error) { + log(error.message); + return; Pointless return - function(proxy, error) { if (error) log(error.message) } does exactly the same ...
Thanks, fixed this and pushed! Attachment 285912 [details] pushed as ec3f8d4 - endSessionDialog: Port offline updates to PackageKit's dbus interface