Merge lp:~jeremywootten/appcenter/fix-1617687-multiple-updates into lp:~elementary-apps/appcenter/appcenter

[Jeremy Wootten]

Prevents multiple authentication dialogs on resume from suspend by preventing more than one update_cache timeout at a time.

Prevents multiple authentication dialogs on resume from suspend by preventing more than one update_cache timeout at a time.

[Adam Bieńkowski (donadigo)]

Tested it two times, unfortunately it doesn't fix the issue for me. However, it does display the warning that the update_cache was called when there is an on-going timeout. I guess we probably need to return at the end of the update_cache_timeout_id statement block?

review: Needs Fixing (code / testing)

[Jeremy Wootten (jeremywootten)]

OK - sorry it didnt fix it - will think again.

[Danielle Foré (danrabbit)]

I can confirm (with Adam) that this reduces the number of dialogs to 1, which is a massive improvement. I think we agree though that it still feels weird to get a root password prompt at resume time and we probably should just not show the dialog and let it wait for the next refresh while logged in

[lp:~jeremywootten/appcenter/fix-1617687-multiple-updates]

293. By Jeremy Wootten on 2016-10-05

Delay cache update after network up and do not force

[Matt Spaulding (madsa)]

This fix seems to work pretty good for me, but isn't a complete fix. If I resume my laptop from suspend and wait at the login screen for some period of time then I'll still see the authentication dialog. Couldn't you query DBus to check if the screen is locked? It looks like light-locker supports a GetActive method that would allow you to do that.

[Jeremy Wootten (jeremywootten)]

Yes, I knew this was a quick, incomplete, fix but I have not had time to fid out the details of how to check whether the screen is locked yet and implement it.

[Danielle Foré (danrabbit)]

Since this is an often-reported pain point for users right now, and this branch does offer an improvement from the behavior of trunk, I'm inclined to say we should merge in order to get this fix in the next and make a bug report targeted to the next milestone about how to improve this block of code, if Jeremy doesn't have the time to make those changes at the moment.

[Danielle Foré (danrabbit)]

in the next release*

=== modified file 'src/Application.vala'
--- src/Application.vala	2016-08-23 14:35:57 +0000
+++ src/Application.vala	2016-10-05 13:51:22 +0000
@@ -23,6 +23,8 @@
         { null }
     };
 
+    private const int SECONDS_AFTER_NETWORK_UP = 60;
+
     public static bool show_updates;
     public static bool silent;
     MainWindow main_window;
@@ -59,13 +61,10 @@
         var client = AppCenterCore.Client.get_default ();
         if (silent) {
             NetworkMonitor.get_default ().network_changed.connect ((available) => {
-                if (available) {
-                    client.update_cache.begin (true);
-                }
+                schedule_cache_update (!available);
             });
 
-            client.update_cache.begin ();
-        
+            client.update_cache.begin (true);
             silent = false;
             hold ();
             return;
@@ -84,11 +83,30 @@
             if (show_updates) {
                 main_window.go_to_installed ();
             }
+        }
+
+        main_window.present ();
+    }
+
+    private uint cache_update_timeout_id = 0;
+    private void schedule_cache_update (bool cancel = false) {
+        var client = AppCenterCore.Client.get_default ();
+
+        if (cache_update_timeout_id > 0) {
+            Source.remove (cache_update_timeout_id);
+            cache_update_timeout_id = 0;
+        }
+
+        if (cancel) {
+            client.cancel_updates (true); // Also stops timeouts.
+            return;
         } else {
-            client.interface_cancellable.reset ();
+            cache_update_timeout_id = Timeout.add_seconds (SECONDS_AFTER_NETWORK_UP, () => {
+                client.update_cache.begin ();
+                cache_update_timeout_id = 0;
+                return false;
+            });
         }
-
-        main_window.present ();
     }
 }
 

=== modified file 'src/Core/Client.vala'
--- src/Core/Client.vala	2016-09-24 01:53:02 +0000
+++ src/Core/Client.vala	2016-10-05 13:51:22 +0000
@@ -27,9 +27,13 @@
     private Gee.LinkedList<AppCenter.Task> task_with_agreement_list;
     private Gee.HashMap<string, AppCenterCore.Package> package_list;
     private AppStream.Database appstream_database;
-    public GLib.Cancellable interface_cancellable;
+    private GLib.Cancellable interface_cancellable;
     private GLib.DateTime last_cache_update;
     private uint updates_number = 0U;
+    private uint update_cache_timeout_id = 0;
+    private bool refresh_in_progress = false;
+
+    private const int SECONDS_BETWEEN_REFRESHES = 60*60*24;
 
     private Client () {
         try {
@@ -49,6 +53,7 @@
         task_with_agreement_list = new Gee.LinkedList<AppCenter.Task> ();
         package_list = new Gee.HashMap<string, AppCenterCore.Package> (null, null);
         interface_cancellable = new GLib.Cancellable ();
+        last_cache_update = null;
 
         appstream_database = new AppStream.Database ();
         try {
@@ -340,12 +345,12 @@
         return package;
     }
 
-    public async void refresh_updates () {
+    private async void refresh_updates () {
         var update_task = new AppCenter.Task ();
         updating_cache = true;
 
         try {
-            Pk.Results result = yield update_task.get_updates_async (0, null, (t, p) => {});
+            Pk.Results result = yield update_task.get_updates_async (0, interface_cancellable, (t, p) => {});
             bool was_empty = updates_number == 0U;
             updates_number = get_package_count (result.get_package_array ());
             if (was_empty && updates_number != 0U) {
@@ -368,7 +373,9 @@
         } catch (Error e) {
             critical (e.message);
         }
+
         updating_cache = false;
+        refresh_in_progress = false;
     }
 
     public uint get_package_count (GLib.GenericArray<weak Pk.Package> package_array) {
@@ -391,24 +398,85 @@
         return size;
     }
 
+    public void cancel_updates (bool cancel_timeout) {
+        interface_cancellable.cancel ();
+
+        if (update_cache_timeout_id > 0 && cancel_timeout) {
+            Source.remove (update_cache_timeout_id);
+            update_cache_timeout_id = 0;
+            last_cache_update = null;
+        }
+
+        interface_cancellable = new GLib.Cancellable ();
+        refresh_in_progress = false;
+    }
+
     public async void update_cache (bool force = false) {
-        // One cache update a day, keeps the doctor away!
-        if (force || last_cache_update == null || (new DateTime.now_local ()).difference (last_cache_update) >= GLib.TimeSpan.DAY) {
-            var refresh_task = new AppCenter.Task ();
-            try {
-                yield refresh_task.refresh_cache_async (false, null, (t, p) => { });
-                last_cache_update = new DateTime.now_local ();
-            } catch (Error e) {
-                critical (e.message);
-            }
-
-            refresh_updates.begin ();
-        }
-
-        GLib.Timeout.add_seconds (60*60*24, () => {
-            update_cache.begin ();
-            return GLib.Source.REMOVE;
-        });
+        debug ("update cache called %s", force.to_string ());
+        bool success = false;
+
+        /* Make sure only one update cache can run at a time */
+        if (refresh_in_progress) {
+            debug ("Update cache already in progress - returning");
+            return;
+        } else {
+            refresh_in_progress = true;
+        }
+
+
+        if (update_cache_timeout_id > 0) {
+            if (force) {
+                debug ("Forced update_cache called when there is an on-going timeout - cancelling timeout");
+                Source.remove (update_cache_timeout_id);
+                update_cache_timeout_id = 0;
+            } else {
+                debug ("Refresh timeout running and not forced - returning");
+                refresh_in_progress = false;
+                return;
+            }
+        }
+
+        /* One cache update a day, keeps the doctor away! */
+        if (force || last_cache_update == null ||
+            (new DateTime.now_local ()).difference (last_cache_update) / GLib.TimeSpan.SECOND >= SECONDS_BETWEEN_REFRESHES) {
+
+            var nm = NetworkMonitor.get_default ();
+
+            if (nm.get_network_available()) {
+                debug ("New refresh task");
+                var refresh_task = new AppCenter.Task ();
+
+
+                try {
+                    Pk.Results result = yield refresh_task.refresh_cache_async (false, interface_cancellable, (t, p) => { });
+                    success = result.get_exit_code () == Pk.Exit.SUCCESS;
+                    last_cache_update = new DateTime.now_local ();
+                } catch (Error e) {
+                    critical ("Update_cache: Refesh cache async failed - %s", e.message);
+
+                }
+
+                if (success) {
+                    refresh_updates.begin ();
+                }
+
+            } else {
+                refresh_in_progress = false; //Stops new timeout while no network.
+            }
+        } else {
+            debug ("Too soon to refresh and not forced");
+        }
+
+
+        if (refresh_in_progress) {
+            update_cache_timeout_id = GLib.Timeout.add_seconds (SECONDS_BETWEEN_REFRESHES, () => {
+                update_cache_timeout_id = 0;
+                update_cache.begin (true);
+                return GLib.Source.REMOVE;
+            });
+
+            refresh_in_progress = success;
+        } // Otherwise updates and timeout were cancelled during refresh, or no network present.
     }
 
     public async Gee.TreeSet<Pk.Package> get_installed_packages () {

=== modified file 'src/MainWindow.vala'
--- src/MainWindow.vala	2016-09-07 18:38:31 +0000
+++ src/MainWindow.vala	2016-10-05 13:51:22 +0000
@@ -163,7 +163,7 @@
                 }
             });
 
-            client.interface_cancellable.cancel ();
+            client.cancel_updates (false); //Timeouts keep running
             return true;
         }
 

=== modified file 'src/Views/AppListView.vala'
--- src/Views/AppListView.vala	2016-09-17 19:28:03 +0000
+++ src/Views/AppListView.vala	2016-10-05 13:51:22 +0000
@@ -250,7 +250,7 @@
             uint update_numbers = 0U;
             uint64 update_real_size = 0ULL;
             foreach (var package in get_packages ()) {
-                if (package.state == AppCenterCore.Package.State.UPDATE_AVAILABLE) {
+                if (package.update_available) {
                     update_numbers++;
                     update_real_size += package.change_information.get_size ();
                 }