Merge lp:~jeremywootten/appcenter/restructuring-part1 into lp:~elementary-apps/appcenter/appcenter

[Jeremy Wootten]

Restructure Package.vala and ChangeInformation.vala to reduce duplication and increase clarity and make a few consequent changes elsewhere

This branch restructures the code in Package.vala and ChangeInformation.vala to reduce duplication and increase clarity and makes a few consequent changes elsewhere.

This branch is not intended to fix bugs but to prepare for later bug fixing branches.

[Adam Bieńkowski (donadigo)]

Great cleanup, although I still have some issues with sorting apps to their right header. An example of this would be to click "update" in the OS updates entry, it would create a new "up to date" header at the top and add the OS updates entry to it like that:

Up to date:
   OS Updates

Available updates:
   ...

Up to date:
   ...

Also if you click "update" on the normal app it is sorted immediately to up to date header, before I even authorize. Is that intentional, or maybe it's fixed in your next branches?

I did comment on the diff for a few code style issues, but it's not something really important.

review: Needs Fixing (code / function)

[lp:~jeremywootten/appcenter/restructuring-part1]

280. By Jeremy Wootten on 2016-08-24

Stop AppListView sort while updating

281. By Jeremy Wootten on 2016-08-25

Remove whitespace; codestyle changes

[Adam Bieńkowski (donadigo)]

Thanks for fixing my issues! The sorting works great now, although I'm not sure about the "OS Updates" entry staying under the "updated" header. However I'm gonna approve this, it's a great cleanup.

review: Approve (code / testing)

=== modified file 'src/Core/ChangeInformation.vala'
--- src/Core/ChangeInformation.vala	2016-07-19 20:41:00 +0000
+++ src/Core/ChangeInformation.vala	2016-08-25 09:09:18 +0000
@@ -25,27 +25,23 @@
     public Gee.TreeSet<Pk.Package> changes { public get; private set; }
     public Gee.TreeSet<Pk.Details> details { public get; private set; }
     public bool can_cancel { public get; private set; default=true; }
-    public Pk.Status status;
+    public Pk.Status status { public get; private set; }
     private Gee.HashMap<string, double?> change_progress;
-    private double progress;
+    public double progress { public get; private set; }
 
     construct {
         changes = new Gee.TreeSet<Pk.Package> ();
         details = new Gee.TreeSet<Pk.Details> ();
         change_progress = new Gee.HashMap<string, double?> ();
         status = Pk.Status.SETUP;
-        progress = 1.0f;
+        progress = 0.0f;
     }
 
     public bool has_changes () {
         return changes.size > 0;
     }
 
-    public double get_progress () {
-        return progress;
-    }
-
-    public string get_status () {
+    public string get_status_string () {
         switch (status) {
             case Pk.Status.SETUP:
                 return _("Starting");
@@ -131,18 +127,36 @@
         return size;
     }
 
-    public void clear () {
-        changes.clear ();
-        details.clear ();
-        reset ();
-    }
-
-    public void reset () {
+    public void start () {
+        progress = 0.0f;
+        progress_changed ();
+        status = Pk.Status.WAIT;
+        status_changed ();
+    }
+
+    public void complete () {
+        status = Pk.Status.FINISHED;
+        status_changed ();
+        reset_progress ();
+    }
+
+    public void cancel () {
+        progress = 0.0f;
+        progress_changed ();
+        status = Pk.Status.CANCEL;
+        status_changed ();
+        reset_progress ();
+    }
+
+    public void clear_update_info () {
+         changes.clear ();
+         details.clear ();
+     }
+
+    public void reset_progress () {
         change_progress.clear ();
         status = Pk.Status.SETUP;
-        status_changed ();
-        progress = 1.0f;
-        progress_changed ();
+        progress = 0.0f;
     }
 
     public void ProgressCallback (Pk.Progress progress, Pk.ProgressType type) {

=== modified file 'src/Core/Package.vala'
--- src/Core/Package.vala	2016-07-08 20:54:44 +0000
+++ src/Core/Package.vala	2016-08-25 09:09:18 +0000
@@ -6,12 +6,12 @@
  * it under the terms of the GNU General Public License as published by
  * the Free Software Foundation, either version 3 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, see <http://www.gnu.org/licenses/>.
  *
@@ -19,6 +19,7 @@
  */
 
 public class AppCenterCore.Package : Object {
+
     public enum State {
         NOT_INSTALLED,
         INSTALLED,
@@ -37,6 +38,12 @@
     public bool changing { public get; private set; default = false; }
     public State state { public get; private set; default = State.NOT_INSTALLED; }
 
+    public double progress {
+        get {
+            return change_information.progress;
+        }
+    }
+
     public bool installed {
         get {
             return !installed_packages.is_empty || component.get_id () == OS_UPDATES_ID;
@@ -45,9 +52,21 @@
 
     public bool update_available {
         get {
-            return installed && change_information.has_changes ();
-        }
-    }
+            return state == State.UPDATE_AVAILABLE;
+        }
+    }
+
+    public bool is_updating {
+        get {
+            return state == State.UPDATING;
+        }
+    }
+
+    public bool is_os_updates {
+        get {
+            return component.id == "xxx-os-updates";
+         }
+     }
 
     public Package (AppStream.Component component) {
         this.component = component;
@@ -58,106 +77,100 @@
 
     public void update_state () {
         if (installed) {
-            if (update_available) {
+            if (change_information.has_changes ()) {
                 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;
-
-        var previous_state = state;
-        state = State.UPDATING;
-        try {
-            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 ();
-
-            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;
-        }
-    }
-
-    public async void install () throws GLib.Error {
-        action_cancellable.reset ();
-        changing = true;
-
-        var previous_state = state;
-        state = State.INSTALLING;
-        try {
+        }
+    }
+
+    public async bool update () {
+        if (state != State.UPDATE_AVAILABLE) {
+            return false;
+        }
+        return yield perform_operation (State.UPDATING, State.INSTALLED, State.UPDATE_AVAILABLE);
+    }
+
+    public async bool install () {
+        if (state != State.UPDATE_AVAILABLE) {
+            return false;
+        }
+        if (yield perform_operation (State.INSTALLING, State.INSTALLED, State.NOT_INSTALLED)) {
+            /* TODO: Move this to a higher level */
             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);
-            installed_packages.add_all (change_information.changes);
-            change_information.clear ();
-
-            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;
+            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);
             }
-
-            changing = false;
-        } catch (Error e) {
-            change_information.reset ();
-            changing = false;
-            state = previous_state;
-            throw e;
-        }
-    }
-
-    public async void uninstall () throws GLib.Error {
+            return true;
+        }
+
+        return false;
+    }
+    public async bool uninstall () {
+        if (state != State.INSTALLED) {
+            return false;
+        }
+        return yield perform_operation (State.REMOVING, State.NOT_INSTALLED, State.INSTALLED);
+    }
+
+    private async bool perform_operation (State performing, State after_success, State after_fail) {
+        var exit_status = Pk.Exit.UNKNOWN;
+        prepare_package_operation (performing);
+        try {
+            exit_status = yield perform_package_operation ();
+        } catch (GLib.Error e) {
+            critical ("Operation failed for package %s - %s", get_name (), e.message);
+        } finally {
+            clean_up_package_operation (exit_status, after_success, after_fail);
+        }
+        return (exit_status == Pk.Exit.SUCCESS);
+    }
+
+    private void prepare_package_operation (State initial_state) {
+        changing = true;
+
         action_cancellable.reset ();
-        changing = true;
-
-        var previous_state = state;
-        state = State.REMOVING;
-        try {
-            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 ();
-
-            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;
+        change_information.start ();
+        state = initial_state;
+    }
+
+    private async Pk.Exit perform_package_operation () throws GLib.Error {
+        Pk.ProgressCallback cb = change_information.ProgressCallback;
+        var client = AppCenterCore.Client.get_default ();
+        switch (state) {
+            case State.UPDATING:
+                return yield client.update_package (this, cb, action_cancellable);
+            case State.INSTALLING:
+                return yield client.install_package (this, cb, action_cancellable);
+            case State.REMOVING:
+                return yield client.remove_package (this, cb, action_cancellable);
+            default:
+                return Pk.Exit.UNKNOWN;
         }
     }
 
+    private void clean_up_package_operation (Pk.Exit exit_status, State success_state, State fail_state) {
+        changing = false;
+
+        installed_packages.add_all (change_information.changes);
+        if (exit_status == Pk.Exit.SUCCESS) {
+            change_information.complete ();
+            state = success_state;
+        } else {
+            state = fail_state;
+            change_information.cancel ();
+         }
+     }
+
     public string? get_name () {
         var _name = component.get_name ();
         if (_name != null) {
@@ -186,6 +199,10 @@
         return null;
     }
 
+    public string get_progress_description () {
+        return change_information.get_status_string ();
+    }
+
     public GLib.Icon get_icon (uint size = 32) {
         GLib.Icon? icon = null;
         uint current_size = 0;
@@ -194,7 +211,7 @@
         component.get_icons ().foreach ((_icon) => {
             if (is_stock) {
                 return;
-            }            
+            }
 
             switch (_icon.get_kind ()) {
                 case AppStream.IconKind.STOCK:
@@ -202,7 +219,7 @@
                         is_stock = true;
                         icon = new ThemedIcon (_icon.get_name ());
                     }
-                    
+
                     break;
                 case AppStream.IconKind.CACHED:
                 case AppStream.IconKind.LOCAL:

=== modified file 'src/Views/AppInfoView.vala'
--- src/Views/AppInfoView.vala	2016-07-04 20:31:29 +0000
+++ src/Views/AppInfoView.vala	2016-08-25 09:09:18 +0000
@@ -246,14 +246,11 @@
     }
 
     private void update_progress_status () {
-        progress_bar.text = package.change_information.get_status ();      
+        progress_bar.text = package.get_progress_description ();      
     }
 
     private void update_progress () {
-        double progress = package.change_information.get_progress ();
-        if (progress < 1.0f) {
-            progress_bar.fraction = progress;
-        }
+        progress_bar.fraction = package.progress;
     }
 
     private void update_state () {
@@ -299,28 +296,18 @@
     }
 
     private async void action_clicked () {
-        try {
-            if (package.update_available) {
-                yield package.update ();
-            } else {
-                yield package.install ();
-
-                // Add this app to the Installed Apps View
-                MainWindow.installed_view.add_app.begin (package);
-            }
-        } catch (Error e) {
-            critical (e.message);
+        if (package.update_available) {
+            yield package.update ();
+        } else if (yield package.install ()) {
+            // Add this app to the Installed Apps View
+            MainWindow.installed_view.add_app.begin (package);
         }
     }
 
     private async void uninstall_clicked () {
-        try {
-            yield package.uninstall ();
-
+        if (yield package.uninstall ()) {
             // Remove this app from the Installed Apps View
             MainWindow.installed_view.remove_app.begin (package);
-        } catch (Error e) {
-            critical (e.message);
         }
     }
 

=== modified file 'src/Views/AppListView.vala'
--- src/Views/AppListView.vala	2016-07-20 13:48:28 +0000
+++ src/Views/AppListView.vala	2016-08-25 09:09:18 +0000
@@ -111,16 +111,20 @@
     private int package_row_compare (Widgets.PackageRow row1, Widgets.PackageRow row2) {
         unowned Package package_a = row1.package;
         unowned Package package_b = row2.package;
-        if (updates_on_top) {
+
+        bool a_update = package_a.update_available || package_a.is_updating;
+        bool b_update = package_b.update_available || package_b.is_updating;
+
+        if (updates_on_top) {    
             if (package_a.component.id == "xxx-os-updates") {
                 return -1;
             } else if (package_b.component.id == "xxx-os-updates") {
                 return 1;
             }
 
-            if (package_a.update_available && !package_b.update_available) {
+            if (a_update && !b_update) {
                 return -1;
-            } else if (!package_a.update_available && package_b.update_available) {
+            } else if (!a_update && b_update) {
                 return 1;
             }
         }
@@ -130,15 +134,17 @@
 
     [CCode (instance_pos = -1)]
     private void package_row_update_header (Widgets.PackageRow row, Widgets.PackageRow? before) {
-        bool update_available = row.package.update_available;
-        if (before == null && update_available) {
-            var updates_grid = get_updates_grid ();
-            row.set_header (updates_grid);
-        } else if ((before == null && !update_available) || update_available != before.package.update_available) {
-            var updated_grid = get_updated_grid ();
-            row.set_header (updated_grid);
-        } else {
-            row.set_header (null);
+        if (!row.package.is_updating && (before == null || !before.package.is_updating)) {
+            bool update_available = row.package.update_available;
+            if (before == null && update_available) {
+                var updates_grid = get_updates_grid ();
+                row.set_header (updates_grid);
+            } else if ((before == null && !update_available) || update_available != before.package.update_available) {
+                var updated_grid = get_updated_grid ();
+                row.set_header (updated_grid);
+            } else {
+                row.set_header (null);
+            }
         }
     }
 

=== modified file 'src/Widgets/PackageRow.vala'
--- src/Widgets/PackageRow.vala	2016-07-05 21:30:40 +0000
+++ src/Widgets/PackageRow.vala	2016-08-25 09:09:18 +0000
@@ -110,14 +110,11 @@
     }
 
     private void update_progress_status () {
-        progress_bar.text = package.change_information.get_status ();
+        progress_bar.text = package.get_progress_description ();
     }
 
     private void update_progress () {
-        double progress = package.change_information.get_progress ();
-        if (progress < 1.0f) {
-            progress_bar.fraction = progress;
-        }
+        progress_bar.fraction = package.progress;
     }
 
     private void update_state () {
@@ -166,17 +163,11 @@
     }
 
     private async void action_clicked () {
-        try {
-            if (package.update_available) {
-                yield package.update ();
-            } else {
-                yield package.install ();
-
-                // Add this app to the Installed Apps View
-                MainWindow.installed_view.add_app.begin (package);
-            }
-        } catch (Error e) {
-            critical (e.message);
+        if (package.update_available) {
+            yield package.update ();
+        } else if (yield package.install ()) {
+            // Add this app to the Installed Apps View
+            MainWindow.installed_view.add_app.begin (package);
         }
     }
 }