Merge lp:~donadigo/appcenter/gtk-application-inhibit into lp:~elementary-apps/appcenter/appcenter

[Adam Bieńkowski]

* Use Gtk.Application.inhibit API instead of a custom one

This branch fixes bug #1656441: "Use Gtk.Application.inhibit".

Instead of creating an entire new class for managing inhibition, we just call Gtk.Application.inhibit, which supports different types of inhibiting and also ensures that in the future we can e.g: implement org.freedesktop.portal.Desktop interface for a custom inhibit behaviour.

[David Hewitt (davidmhewitt)]

This is very difficult to test since the inhibit is only active during updates etc.

Considering the same fix was rejected from Audience for not working, has this been tested to ensure it does inhibit? If so, can you provide details on how to test it?

review: Needs Information

[Adam Bieńkowski (donadigo)]

davidmhewitt: the reason this was rejected from audience is that Audience also calls something like "SimulateUserActivity" and Gtk Application does not. I did check what does the code in Gtk.Application class and it does the same as the unneded SuspendControl class in AppCenter (meaning connecting to the right interface and calling methods in order to inhibit the session, in this case org.gnome.SessionManager).

For testing I guess for the time AppCenter updates the OS, you could look into the gnome's SessionManager interface in d-feet to see if the session is currently inhibited. The other way is to do that in the code using Gtk.Application.is_inhibited to print out the boolean value of it's state.

[David Hewitt (davidmhewitt)]

Ok, so I've had a bit of a play with this and it seems there's a bit of a problem. Although, not with your code.

The inhibit doesn't work with this branch or with trunk. I tested by letting some updates build up and then tried to suspend mid way through the updates being applied, and the computer happily suspended.

I've done a bit of investigation and it seems as though there's a permissions issue.

For example, if you run:
systemd-inhibit --what="idle:sleep:shutdown" --mode=block sleep 40

and then try to suspend the computer, it suspends instantly.

However, if you run that with sudo on the front and give it your password, and then try to suspend, you get the polkit dialog telling you that suspend is inhibited.

I wrote a little helper to talk to the org.freedesktop.login1 D-BUS server and the same applies, if you run as a normal user, it looks like the inhibit calls succeed, but you can still suspend. If you run the helper as root, it works.

Couldn't test the org.gnome.SessionManager stuff as root as that's running on the session D-BUS, but I imagine the result might have been the same. So I don't think there's any point merging this until we can come up with a fix for why the inhibit doesn't work at all.

review: Disapprove

Unmerged revisions

405. By Adam Bieńkowski on 2017-02-20

Use inhibit in Gtk.Application instead of using SuspendControl

=== modified file 'src/Application.vala'
--- src/Application.vala	2017-02-12 16:06:49 +0000
+++ src/Application.vala	2017-02-20 17:25:58 +0000
@@ -26,9 +26,11 @@
     private const int SECONDS_AFTER_NETWORK_UP = 60;
 
     private static string? link = null;
+    private static uint inhibit_cookie = 0;
 
     public static bool show_updates;
     public static bool silent;
+
     MainWindow main_window;
     construct {
         application_id = "org.pantheon.appcenter";
@@ -78,6 +80,23 @@
         add_accelerator ("<Control>q", "app.quit", null);
     }
 
+    public void inhibit_session () {
+        if (inhibit_cookie != 0) {
+            return;
+        }
+
+        inhibit_cookie = inhibit (main_window, Gtk.ApplicationInhibitFlags.SUSPEND, "Inhibit suspend during update");
+    }
+
+    public void uninhibit_session () {
+        if (inhibit_cookie == 0) {
+            return;
+        }
+
+        uninhibit (inhibit_cookie);
+        inhibit_cookie = 0;
+    }
+
     public override void open (File[] files, string hint) {
         var file = files[0];
         if (file == null) {

=== modified file 'src/CMakeLists.txt'
--- src/CMakeLists.txt	2017-02-10 20:15:32 +0000
+++ src/CMakeLists.txt	2017-02-20 17:25:58 +0000
@@ -7,7 +7,6 @@
     Application.vala
     MainWindow.vala
     Settings.vala
-    SuspendControl.vala
     AbstractAppContainer.vala
     AbstractAppList.vala
     Views/AppInfoView.vala

=== modified file 'src/Core/Client.vala'
--- src/Core/Client.vala	2017-02-04 17:18:30 +0000
+++ src/Core/Client.vala	2017-02-20 17:25:58 +0000
@@ -34,7 +34,6 @@
     private const int SECONDS_BETWEEN_REFRESHES = 60*60*24;
 
     private Task client;
-    private SuspendControl sc;
 
     private Client () {
     }
@@ -44,7 +43,6 @@
         cancellable = new GLib.Cancellable ();
 
         client = new Task ();
-        sc = new SuspendControl ();
 
         cancellable = new GLib.Cancellable ();
         last_cache_update = null;
@@ -128,8 +126,10 @@
         
         packages_ids += null;
 
+        var application = (AppCenter.App)Application.get_default ();
+
         try {
-            sc.inhibit ();
+            application.inhibit_session ();
 
             var results = yield client.update_packages_async (packages_ids, cancellable, cb);
             exit_status = results.get_exit_code ();
@@ -137,7 +137,7 @@
             task_count--;
             throw e;
         } finally {
-            sc.uninhibit ();
+            application.uninhibit_session ();
         }
 
         if (exit_status != Pk.Exit.SUCCESS) {

=== removed file 'src/SuspendControl.vala'
--- src/SuspendControl.vala	2016-01-30 16:44:48 +0000
+++ src/SuspendControl.vala	1970-01-01 00:00:00 +0000
@@ -1,71 +0,0 @@
-/*
-* Copyright (c) 2011-2015 elementary LLC (https://elementary.io)
-*
-* This program is free software; you can redistribute it and/or
-* modify it under the terms of the GNU General Public
-* License as published by the Free Software Foundation; either
-* version 2 of the License, or (at your option) any later version.
-*
-* This program is distributed in the hope that it will be useful,
-* but WITHOUT ANY WARRANTY; without even the implied warranty of
-* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
-* General Public License for more details.
-*
-* You should have received a copy of the GNU General Public
-* License along with this program; if not, write to the
-* Free Software Foundation, Inc., 59 Temple Place - Suite 330,
-* Boston, MA 02111-1307, USA.
-*
-* Authored by: Richard Fairthorne <[e-mail]>
-*/
-
-[DBus (name = "org.gnome.SessionManager")]
-public interface SessionManager : Object {
-    public abstract uint32 inhibit (string app_id, uint32 toplevel_xid, string reason, uint32 flags) throws IOError;
-    public abstract void uninhibit (uint32 inhibit_cookie) throws IOError;
-}
-
-public class SuspendControl {
-
-    SessionManager sm;
-    bool inhibited = false;
-    uint32 inhibit_cookie = 0;
-    bool supported = true;
-
-    public SuspendControl () {
-        try {
-            sm = Bus.get_proxy_sync (BusType.SESSION, "org.gnome.SessionManager", "/org/gnome/SessionManager");
-        } catch (IOError e) {
-            supported = false;
-            critical (e.message);
-        }
-    }
-
-    public bool inhibit () {
-        if (inhibited == false && supported) {
-            try {
-                inhibit_cookie = sm.inhibit ("org.richardfairthorne.SuspendControl", 0, "Inhibit suspend during update", 4);
-                inhibited = true;
-            } catch (IOError e) {
-                critical (e.message);
-            }
-        }
-
-        return inhibited;
-    }
-
-    public bool uninhibit () {
-        try {
-            if (inhibited == true && supported) {
-                sm.uninhibit (inhibit_cookie);
-                inhibited = false;
-            }
-        } catch (IOError e) {
-            critical (e.message);
-        }
-
-        return !inhibited;
-    }
-
-}
-

=== modified file 'src/Views/AppListView.vala'
--- src/Views/AppListView.vala	2017-02-15 18:02:21 +0000
+++ src/Views/AppListView.vala	2017-02-20 17:25:58 +0000
@@ -95,7 +95,6 @@
         private uint apps_done = 0;
         private Gee.LinkedList<AppCenterCore.Package> apps_to_update;
         private AppCenterCore.Package first_package;
-        private SuspendControl sc;
 
         private bool _updating_cache;
         public bool updating_cache {
@@ -129,8 +128,6 @@
 
             update_mutex = GLib.Mutex ();
             apps_to_update = new Gee.LinkedList<AppCenterCore.Package> ();
-
-            sc = new SuspendControl ();
         }
 
         public AppListUpdateView () {
@@ -203,7 +200,8 @@
             // Update all updateable apps
             if (apps_to_update.size > 0) {
                 // Prevent computer from sleeping while updating apps
-                sc.inhibit ();
+                var application = (AppCenter.App)Application.get_default ();
+                application.inhibit_session ();
 
                 first_package = apps_to_update[0];
                 first_package.info_changed.connect_after (after_first_package_info_changed);
@@ -254,7 +252,9 @@
             assert (updating_all_apps && packages_changing == 0);
 
             updating_all_apps = false;
-            sc.uninhibit ();
+
+            var application = (AppCenter.App)Application.get_default ();
+            application.uninhibit_session ();
 
             /* Set the action button sensitive and emit "changed" on each row in order to update
              * the sort order and headers (any change would have been ignored while updating) */