Merge lp:~donadigo/appcenter/action-buttons-state into lp:~elementary-apps/appcenter/appcenter

[Adam Bieńkowski]

- Rework action buttons frontend.
- Fix incorrect action label on update page.
- Remove unneded code.

This branch reworks the current frontend for action buttons.
It now uses it's own enum "Package.State" which reflects the current state. The frontend reads the state and based on it shows available actions.

There are also some other changes to the backend and that's why I would like to have a code and test review first before merging this branch.

It also fixes current incorrect displaying "Install" label on update page.

[lp:~donadigo/appcenter/action-buttons-state]

237. By Adam Bieńkowski on 2016-07-02

Do not show uninstall button for OS updates

[Corentin Noël (tintou)]

The code looks fine, thank you for this simplification !

review: Approve (code)

[Danielle Foré (danrabbit)]

Everything seems to work correctly from the appinfo view.

If you install from packagerow, the button state for that package won't update until you perform another installation

review: Needs Fixing

[lp:~donadigo/appcenter/action-buttons-state]

238. By Adam Bieńkowski on 2016-07-03

Added missing line to PackageRow.vala; remove unneded progress state action_stack setting

239. By Adam Bieńkowski on 2016-07-03

Remove unneded code

[Danielle Foré (danrabbit)]

1. Start an uninstall in AppInfo
2. Press back

Expectation: See the uninstall progress in PackageRow
Reality: No progress

review: Needs Fixing

[lp:~donadigo/appcenter/action-buttons-state]

240. By Adam Bieńkowski on 2016-07-04

Remove unneded action_stack visibility changing in AppInfoView; show action stack in the PackageRow when action in progress

[Adam Bieńkowski (donadigo)]

Updated.

[Danielle Foré (danrabbit)]

On the updates tab, after updating a package it no longer re-organizes itself under the "Up to Date" header.

review: Needs Fixing

[lp:~donadigo/appcenter/action-buttons-state]

241. By Adam Bieńkowski on 2016-07-05

Move state setting after clearing action info

[Adam Bieńkowski (donadigo)]

Updated again. Just FYI number of available updates doesn't update, but that issue also exists in trunk.

[Corentin Noël (tintou)]

The code remains good.

review: Approve (code)

[Cody Garver (codygarver)]

I found a regression in this branch:

When you click Update on Package A, and while it's in progress, click Update on Package B

Package A says Finished when it completes but the progress bar remains and displays 50% until Package B update is complete

In Trunk, the Package A progress bar disappears as soon as Package A update is complete, regardless of the state of Package B

[lp:~donadigo/appcenter/action-buttons-state]

242. By Adam Bieńkowski on 2016-07-08

Fixed condition where the state could only change if the app was in focus

[Danielle Foré (danrabbit)]

I can still confirm the issue that Cody pointed out

=== modified file 'src/Core/ChangeInformation.vala'
--- src/Core/ChangeInformation.vala	2016-06-29 21:40:17 +0000
+++ src/Core/ChangeInformation.vala	2016-07-08 20:54:59 +0000
@@ -26,11 +26,8 @@
     public Gee.TreeSet<Pk.Details> details { public get; private set; }
     public bool can_cancel { public get; private set; default=true; }
     private Gee.HashMap<string, double?> change_progress;
-    public Pk.Status status;
+    private Pk.Status status;
     private double progress;
-    public ChangeInformation () {
-        
-    }
 
     construct {
         changes = new Gee.TreeSet<Pk.Package> ();

=== modified file 'src/Core/Client.vala'
--- src/Core/Client.vala	2016-07-01 22:02:04 +0000
+++ src/Core/Client.vala	2016-07-08 20:54:59 +0000
@@ -143,7 +143,8 @@
         return exit_status;
     }
 
-    public async void update_package (Package package, Pk.ProgressCallback cb, GLib.Cancellable cancellable) throws GLib.Error {
+    public async Pk.Exit update_package (Package package, Pk.ProgressCallback cb, GLib.Cancellable cancellable) throws GLib.Error {
+        Pk.Exit exit_status = Pk.Exit.UNKNOWN;
         SuspendControl sc = new SuspendControl ();
         AppCenter.Task update_task = request_task ();
         string[] packages_ids = {};
@@ -155,7 +156,8 @@
         try {
             sc.inhibit ();
             var results = yield update_task.update_packages_async (packages_ids, cancellable, cb);
-            if (results.get_exit_code () != Pk.Exit.SUCCESS) {
+            exit_status = results.get_exit_code ();
+            if (exit_status != Pk.Exit.SUCCESS) {
                 release_task (update_task);
                 throw new GLib.IOError.FAILED (Pk.Exit.enum_to_string (results.get_exit_code ()));
             }
@@ -168,9 +170,11 @@
 
         yield refresh_updates ();
         release_task (update_task);
+        return exit_status;
     }
 
-    public async void remove_package (Package package, Pk.ProgressCallback cb, GLib.Cancellable cancellable) throws GLib.Error {
+    public async Pk.Exit remove_package (Package package, Pk.ProgressCallback cb, GLib.Cancellable cancellable) throws GLib.Error {
+        Pk.Exit exit_status = Pk.Exit.UNKNOWN;
         AppCenter.Task remove_task = request_task ();
         AppCenter.Task search_task = request_task ();
         string[] packages_ids = {};
@@ -187,7 +191,8 @@
                 packages_ids += package.package_id;
             });
 
-            yield remove_task.remove_packages_async (packages_ids, true, true, cancellable, cb);
+            results = yield remove_task.remove_packages_async (packages_ids, true, true, cancellable, cb);
+            exit_status = results.get_exit_code ();
         } catch (Error e) {
             release_task (search_task);
             release_task (remove_task);
@@ -197,6 +202,7 @@
         yield refresh_updates ();
         release_task (search_task);
         release_task (remove_task);
+        return exit_status;
     }
 
     public async void get_updates () {
@@ -228,7 +234,7 @@
 
                     package.change_information.changes.add (pk_package);
                     package.change_information.details.add (pk_detail);
-                    package.notify_property ("update-available");
+                    package.update_state ();
                 } catch (Error e) {
                     critical (e.message);
                 }
@@ -252,7 +258,7 @@
             var package = package_list.get (pk_package.get_name ());
             if (package != null) {
                 package.installed_packages.add (pk_package);
-                package.notify_property ("installed");
+                package.update_state ();
                 packages.add (package);
             }
         }

=== modified file 'src/Core/Package.vala'
--- src/Core/Package.vala	2016-07-01 22:02:04 +0000
+++ src/Core/Package.vala	2016-07-08 20:54:59 +0000
@@ -19,26 +19,32 @@
  */
 
 public class AppCenterCore.Package : Object {
+    public enum State {
+        NOT_INSTALLED,
+        INSTALLED,
+        INSTALLING,
+        UPDATE_AVAILABLE,
+        UPDATING,
+        REMOVING
+    }
+
     public const string OS_UPDATES_ID = "xxx-os-updates";
-    public signal void changed ();
 
     public AppStream.Component component { public get; private set; }
     public ChangeInformation change_information { public get; private set; }
     public Gee.TreeSet<Pk.Package> installed_packages { public get; private set; }
     public GLib.Cancellable action_cancellable { public get; private set; }
-    public bool changing { public get; private set; default= false; }
+    public bool changing { public get; private set; default = false; }
+    public State state { public get; private set; default = State.NOT_INSTALLED; }
 
     public bool installed {
-        public get {
+        get {
             return !installed_packages.is_empty || component.get_id () == OS_UPDATES_ID;
         }
-        private set {
-            
-        }
     }
 
     public bool update_available {
-        public get {
+        get {
             return installed && change_information.has_changes ();
         }
     }
@@ -50,19 +56,40 @@
         action_cancellable = new GLib.Cancellable ();
     }
 
+    public void update_state () {
+        if (installed) {
+            if (update_available) {
+                state = State.UPDATE_AVAILABLE;
+            } else {
+                state = State.INSTALLED;
+            }
+        } else {
+            state = State.NOT_INSTALLED;
+        }        
+    }
+
     public async void update () throws GLib.Error {
         action_cancellable.reset ();
         changing = true;
-        changed ();
+
+        var previous_state = state;
+        state = State.UPDATING;
         try {
-            yield AppCenterCore.Client.get_default ().update_package (this, (progress, type) => {change_information.ProgressCallback (progress, type);}, action_cancellable);
+            var exit_status = yield AppCenterCore.Client.get_default ().update_package (this, (progress, type) => {change_information.ProgressCallback (progress, type);}, action_cancellable);
             installed_packages.add_all (change_information.changes);
             change_information.clear ();
-            notify_property ("update-available");
-            changing = false;
+
+            if (exit_status == Pk.Exit.SUCCESS) {
+                state = State.INSTALLED;
+            } else {
+                state = previous_state;
+            }      
+
+            changing = false;      
         } catch (Error e) {
             change_information.reset ();
             changing = false;
+            state = previous_state;
             throw e;
         }
     }
@@ -70,28 +97,37 @@
     public async void install () throws GLib.Error {
         action_cancellable.reset ();
         changing = true;
-        changed ();
+
+        var previous_state = state;
+        state = State.INSTALLING;
         try {
             var application = (Gtk.Application)Application.get_default ();
             var window = application.get_active_window ().get_window ();
 
             var exit_status = yield AppCenterCore.Client.get_default ().install_package (this, (progress, type) => {change_information.ProgressCallback (progress, type);}, action_cancellable);
-            if (exit_status == Pk.Exit.SUCCESS && (window.get_state () & Gdk.WindowState.FOCUSED) == 0) {
-                var notification = new Notification (_("Application installed"));
-                notification.set_body (_("%s has been successfully installed").printf (get_name ()));
-                notification.set_icon (new ThemedIcon ("system-software-install"));
-                notification.set_default_action ("app.open-application");
-
-                application.send_notification ("installed", notification);
-            }
-
             installed_packages.add_all (change_information.changes);
             change_information.clear ();
-            installed = true;
+
+            if (exit_status == Pk.Exit.SUCCESS) {
+                state = State.INSTALLED;
+
+                if ((window.get_state () & Gdk.WindowState.FOCUSED) == 0) {
+                    var notification = new Notification (_("Application installed"));
+                    notification.set_body (_("%s has been successfully installed").printf (get_name ()));
+                    notification.set_icon (new ThemedIcon ("system-software-install"));
+                    notification.set_default_action ("app.open-application");
+
+                    application.send_notification ("installed", notification);
+                }
+            } else {
+                state = previous_state;
+            }
+
             changing = false;
         } catch (Error e) {
             change_information.reset ();
             changing = false;
+            state = previous_state;
             throw e;
         }
     }
@@ -99,16 +135,25 @@
     public async void uninstall () throws GLib.Error {
         action_cancellable.reset ();
         changing = true;
-        changed ();
+
+        var previous_state = state;
+        state = State.REMOVING;
         try {
-            yield AppCenterCore.Client.get_default ().remove_package (this, (progress, type) => {change_information.ProgressCallback (progress, type);}, action_cancellable);
+            var exit_status = yield AppCenterCore.Client.get_default ().remove_package (this, (progress, type) => {change_information.ProgressCallback (progress, type);}, action_cancellable);
             installed_packages.clear ();
             change_information.clear ();
-            installed = false;
+
+            if (exit_status == Pk.Exit.SUCCESS) {
+                state = State.NOT_INSTALLED;
+            } else {
+                state = previous_state;
+            }
+
             changing = false;
         } catch (Error e) {
             change_information.reset ();
             changing = false;
+            state = previous_state;
             throw e;
         }
     }

=== modified file 'src/Views/AppInfoView.vala'
--- src/Views/AppInfoView.vala	2016-07-01 16:48:32 +0000
+++ src/Views/AppInfoView.vala	2016-07-08 20:54:59 +0000
@@ -21,7 +21,7 @@
 using AppCenterCore;
 
 public class AppCenter.Views.AppInfoView : Gtk.Grid {
-    AppCenterCore.Package package;
+    Package package;
 
     Gtk.Image app_icon;
     Gtk.Image app_screenshot;
@@ -41,12 +41,15 @@
     Gtk.Button cancel_button;
     Gtk.Stack action_stack;
 
-    public AppInfoView (AppCenterCore.Package package) {
+    bool is_os_updates = false;
+
+    public AppInfoView (Package package) {
         this.package = package;
         app_name.label = package.get_name ();
         app_summary.label = package.get_summary ();
         parse_description (package.component.get_description ());
         app_icon.gicon = package.get_icon (128);
+        is_os_updates = package.component.id == "xxx-os-updates";
 
         if (package.component.get_extensions ().length > 0) {
             extension_box = new Gtk.ListBox ();
@@ -63,29 +66,15 @@
             load_extensions.begin ();
         }
 
-        if (package.update_available) {
-            action_button.label = _("Update");
-        } else if (package.installed) {
-            action_button.no_show_all = true;
-            action_button.hide ();
-        } else {
-            uninstall_button.no_show_all = true;
-            uninstall_button.hide ();
-        }
-
-        if (package.component.id == "xxx-os-updates") {
-            uninstall_button.no_show_all = true;
-            uninstall_button.hide ();
-        }
-
-        package.notify["installed"].connect (() => update_buttons ());
-        package.notify["update-available"].connect (() => update_buttons ());
+        package.notify["state"].connect (update_state);
 
         package.change_information.bind_property ("can-cancel", cancel_button, "sensitive", GLib.BindingFlags.SYNC_CREATE);
-        package.change_information.progress_changed.connect (() => update_progress ());
-        package.change_information.status_changed.connect (() => update_status ());
+        package.change_information.progress_changed.connect (update_progress);
+        package.change_information.status_changed.connect (update_progress_status);
+
+        update_progress_status (); 
         update_progress ();
-        update_status ();
+        update_state ();
     }
 
     construct {
@@ -234,7 +223,7 @@
     }
 
     public void load_more_content () {
-        new Thread<void*> ("content loading", () => {
+        new Thread<void*> ("content-loading", () => {
             string url = null;
             uint max_size = 0U;
             package.component.get_screenshots ().foreach ((screenshot) => {
@@ -256,42 +245,53 @@
         });
     }
 
-    private void update_status () {
-        progress_bar.text = package.change_information.get_status ();
-        if (package.change_information.status == Pk.Status.FINISHED) {
-            action_stack.set_visible_child_name ("buttons");
-        } else {
-            action_stack.set_visible_child_name ("progress");
-        }         
+    private void update_progress_status () {
+        progress_bar.text = package.change_information.get_status ();      
     }
 
     private void update_progress () {
-        var progress = package.change_information.get_progress ();
+        double progress = package.change_information.get_progress ();
         if (progress < 1.0f) {
-            action_stack.set_visible_child_name ("progress");
             progress_bar.fraction = progress;
+        }
+    }
+
+    private void update_state () {
+        if (package.state != Package.State.NOT_INSTALLED && !is_os_updates) {
+            uninstall_button.no_show_all = false;
+            uninstall_button.show_all ();               
         } else {
-            action_stack.set_visible_child_name ("buttons");
+            uninstall_button.no_show_all = true;
+            uninstall_button.hide ();
         }
-    }
-
-    private void update_buttons () {
-        if (package.installed) {
-            if (package.update_available) {
+
+        switch (package.state) {
+            case Package.State.NOT_INSTALLED:
+                action_button.label = _("Install");
+                action_button.no_show_all = false;
+                action_button.show_all ();
+
+                action_stack.set_visible_child_name ("buttons");
+                break;
+            case Package.State.INSTALLED:
+                action_button.no_show_all = true;
+                action_button.hide ();
+
+                action_stack.set_visible_child_name ("buttons");
+                break;
+            case Package.State.UPDATE_AVAILABLE:
                 action_button.label = _("Update");
                 action_button.no_show_all = false;
-                action_button.show_all ();                
-            } else {
-                action_button.no_show_all = true;
-                action_button.hide ();                   
-                uninstall_button.no_show_all = false;
-                uninstall_button.show_all ();                
-            }
-        } else {
-            action_button.label = _("Install");
-            action_button.no_show_all = false;
-            action_button.show_all ();
-        }
+                action_button.show_all ();    
+
+                action_stack.set_visible_child_name ("buttons");
+                break;
+            case Package.State.INSTALLING:
+            case Package.State.UPDATING:
+            case Package.State.REMOVING:
+                action_stack.set_visible_child_name ("progress");
+                break;
+        }        
     }
 
     private void action_cancelled () {
@@ -302,38 +302,25 @@
         try {
             if (package.update_available) {
                 yield package.update ();
-                action_button.no_show_all = true;
-                action_button.hide ();
             } else {
                 yield package.install ();
-                action_button.no_show_all = true;
-                action_button.hide ();
-                if (package.component.id != "xxx-os-updates") {
-                    uninstall_button.no_show_all = false;
-                    uninstall_button.show_all ();
-                }
-                
+
                 // Add this app to the Installed Apps View
                 MainWindow.installed_view.add_app.begin (package);
             }
         } catch (Error e) {
             critical (e.message);
-            action_stack.set_visible_child_name ("buttons");
         }
     }
 
     private async void uninstall_clicked () {
         try {
             yield package.uninstall ();
-            action_button.label = _("Install");
-            uninstall_button.no_show_all = true;
-            uninstall_button.hide ();
 
             // Remove this app from the Installed Apps View
             MainWindow.installed_view.remove_app.begin (package);
         } catch (Error e) {
             critical (e.message);
-            action_stack.set_visible_child_name ("buttons");
         }
     }
 
@@ -382,7 +369,8 @@
     }
 
     private void parse_description (string? description) {
-        if (description != null)
+        if (description != null) {
             app_description.buffer.text = AppStream.description_markup_convert_simple (description);
+        }
     }
 }

=== modified file 'src/Views/AppListView.vala'
--- src/Views/AppListView.vala	2016-06-26 23:28:28 +0000
+++ src/Views/AppListView.vala	2016-07-08 20:54:59 +0000
@@ -191,7 +191,7 @@
         uint current_update_number = update_numbers;
         list_box.get_children ().foreach ((child) => {
             var package = ((Widgets.PackageRow) child).package;
-            if (package.update_available) {
+            if (package != null && package.update_available) {
                 package.notify["changing"].connect (() => {
                     if (package.changing) {
                         current_update_number--;

=== modified file 'src/Widgets/PackageRow.vala'
--- src/Widgets/PackageRow.vala	2016-06-29 21:40:17 +0000
+++ src/Widgets/PackageRow.vala	2016-07-08 20:54:59 +0000
@@ -40,21 +40,15 @@
         package_summary.ellipsize = Pango.EllipsizeMode.END;
         image.gicon = package.get_icon ();
 
-        if (package.update_available) {
-            action_button.label = _("Update");
-        } else if (package.installed) {
-            action_stack.no_show_all = true;
-            action_stack.hide ();
-        }
-
-        package.notify["installed"].connect (() => update_buttons ());
-        package.notify["update-available"].connect (() => update_buttons ());
+        package.notify["state"].connect (update_state);
 
         package.change_information.bind_property ("can-cancel", cancel_button, "sensitive", GLib.BindingFlags.SYNC_CREATE);
-        package.change_information.progress_changed.connect (() => update_progress ());
-        package.change_information.status_changed.connect (() => update_status ());
+        package.change_information.progress_changed.connect (update_progress);
+        package.change_information.status_changed.connect (update_progress_status);
+
+        update_progress_status (); 
         update_progress ();
-        update_status ();
+        update_state ();
     }
 
     construct {
@@ -115,52 +109,56 @@
         add (grid);
     }
 
-    private void update_status () {
+    private void update_progress_status () {
         progress_bar.text = package.change_information.get_status ();
-        if (package.change_information.status == Pk.Status.FINISHED) {
-            action_stack.set_visible_child_name ("buttons");
-        } else {
-            action_stack.set_visible_child_name ("progress");   
-            action_stack.no_show_all = false;
-            action_stack.show_all ();   
-        }        
     }
 
     private void update_progress () {
-        var progress = package.change_information.get_progress ();
+        double progress = package.change_information.get_progress ();
         if (progress < 1.0f) {
-            action_stack.set_visible_child_name ("progress");
             progress_bar.fraction = progress;
-            action_stack.no_show_all = false;
-            action_stack.show_all ();               
-        } else {
-            action_stack.set_visible_child_name ("buttons");
         }
     }
 
-    private void update_buttons () {
-        if (package.installed) {
-            if (package.update_available) {
+    private void update_state () {
+        switch (package.state) {
+            case Package.State.NOT_INSTALLED:
+                action_button.label = _("Install");
+                action_button.no_show_all = false;
+                action_button.show_all ();
+
+                action_stack.no_show_all = false;
+                action_stack.show_all ();
+
+                action_stack.set_visible_child_name ("buttons");
+                break;
+            case Package.State.INSTALLED:
+                action_stack.no_show_all = true;
+                action_stack.hide ();                
+
+                action_stack.set_visible_child_name ("buttons");
+                break;
+            case Package.State.UPDATE_AVAILABLE:
                 action_button.label = _("Update");
                 action_button.no_show_all = false;
                 action_button.show_all ();    
 
                 action_stack.no_show_all = false;
                 action_stack.show_all ();
-            } else {
-                action_stack.no_show_all = true;
-                action_stack.hide ();
-            }
-        } else {
-            action_button.label = _("Install");
-            action_button.no_show_all = false;
-            action_button.show_all ();
-
-            action_stack.no_show_all = false;
-            action_stack.show_all ();                
-        }
-
-        changed ();        
+
+                action_stack.set_visible_child_name ("buttons");
+                break;
+            case Package.State.INSTALLING:
+            case Package.State.UPDATING:
+            case Package.State.REMOVING:
+                action_stack.no_show_all = false;
+                action_stack.show_all ();
+
+                action_stack.set_visible_child_name ("progress");
+                break;
+        }  
+
+        changed ();
     }
 
     private void action_cancelled () {
@@ -173,12 +171,12 @@
                 yield package.update ();
             } else {
                 yield package.install ();
+
+                // Add this app to the Installed Apps View
+                MainWindow.installed_view.add_app.begin (package);
             }
-            action_stack.no_show_all = true;
-            action_stack.hide ();
         } catch (Error e) {
             critical (e.message);
-            action_stack.set_visible_child_name ("buttons");
         }
     }
 }