Merge lp:~amigadave/gnome-control-center-signon/switch-mockup into lp:gnome-control-center-signon

[David King]

Add switches to toggle the enabled state for application (really services)

Add switches to toggle the enabled state for application (really services)

* Add a non-functional switch to each application row
* Add an empty application switch activated handler
* Implement application service switch handler
* Use asynchronous store methods in AccountDetailsPage
* Avoid storing the account when a store is in progress
* Avoid unnecessarily changing the account enabled state in the UI

[Alberto Mardegan (mardy)]

I will test the functionality soon. Meanwhile, I have a couple of suggestions about the code, which shouldn't affect the functionality.

73 + // Fetch current state from service.
74 + current_account.select_service (service);
75 + app_switch.active = current_account.get_enabled ();
76 + current_account.select_service (null);
77 +
78 + acc_service.set_data ("switch", app_switch);
79 + acc_service.enabled.connect (on_app_account_service_enabled);

This code (as well as on_app_account_service_enabled()), fits better inside the AccountApplicationSwitch class (maybe in the constructor).

246 + // Ignore service-level changes.
247 + // FIXME: http://code.google.com/p/accounts-sso/issues/detail?id=157
248 + if (service != "global")
249 + {
250 + return;
251 + }

Check the indentation (it appears correct here, but see the diff below). Also, in order to continue working when the libaccounts-glib issue is fixed, add a check for "null" as well.

review: Needs Fixing

[Alberto Mardegan (mardy)]

There is one bug: if you take an account which has only one service (identi.ca, for example) and disable the service, then disable the global account, you'll see that the global account is still enabled (you can use account-console to verify that).

The other issue is that when you disable the global account, all services are disabled. It's actually a UI preference, but I'd like better if the services stayed enabled (those that were enabled, at least) but their widget become disabled.
Besides the UI impact, this should also help you to keep the logic in the code cleaner, when one switch has only one function.

In any case, the switch widgets should be disabled when the global account is disabled.

[lp:~amigadave/gnome-control-center-signon/switch-mockup]

140. By David King on 2013-02-11

Insulate againt a future libaccounts-glib bugfix

141. By David King on 2013-02-11

Refactor some code inside AccountApplicationSwitch

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:141
http://s-jenkins:8080/job/gnome-control-center-signon-ci/23/
Executed test runs:
    SUCCESS: http://s-jenkins:8080/job/gnome-control-center-signon-ci/./build=pbuilder,distribution=raring,flavor=amd64/23/console
    SUCCESS: http://s-jenkins:8080/job/gnome-control-center-signon-ci/./build=pbuilder,distribution=raring,flavor=i386/23/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/gnome-control-center-signon-ci/23//rebuild/?

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:142
http://s-jenkins:8080/job/gnome-control-center-signon-ci/24/
Executed test runs:
    SUCCESS: http://s-jenkins:8080/job/gnome-control-center-signon-ci/./build=pbuilder,distribution=raring,flavor=amd64/24/console
    SUCCESS: http://s-jenkins:8080/job/gnome-control-center-signon-ci/./build=pbuilder,distribution=raring,flavor=i386/24/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/gnome-control-center-signon-ci/24//rebuild/?

review: Approve (continuous-integration)

[lp:~amigadave/gnome-control-center-signon/switch-mockup]

142. By David King on 2013-02-18

Refactor some code into AccountApplicationSwitch

Move the switch-specific signal handlers into the switch class.

143. By David King on 2013-02-18

Use synchronous store for the account application switches

Avoids a critical warning when two applications share the same service.

[David King (amigadave)]

Should be ready for merging now, please test!

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:143
http://s-jenkins:8080/job/gnome-control-center-signon-ci/25/
Executed test runs:
    SUCCESS: http://s-jenkins:8080/job/gnome-control-center-signon-ci/./build=pbuilder,distribution=raring,flavor=amd64/25/console
    SUCCESS: http://s-jenkins:8080/job/gnome-control-center-signon-ci/./build=pbuilder,distribution=raring,flavor=i386/25/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/gnome-control-center-signon-ci/25//rebuild/?

review: Approve (continuous-integration)

[Alberto Mardegan (mardy)]

Thanks, works very well now!

review: Approve

=== modified file 'src/cc-credentials-account-applications-model.vala'
--- src/cc-credentials-account-applications-model.vala	2012-09-18 11:06:40 +0000
+++ src/cc-credentials-account-applications-model.vala	2013-02-18 11:18:30 +0000
@@ -27,6 +27,8 @@
      * @param description the description of the application
      * @param plugin the plugin to configure account options, if any
      * @param plugin_widget the widget to show when configuring account options
+     * @param service_name the service name, as supplied to
+     * Ag.Manager.get_service ()
      */
     public struct AccountApplicationRow
     {
@@ -35,6 +37,7 @@
         public string description;
         public Ap.ApplicationPlugin plugin;
         public Gtk.Widget plugin_widget;
+        public string service_name;
     }
 }
 
@@ -165,7 +168,8 @@
             icon = app_icon,
             description = description_markup,
             plugin = app_plugin,
-            plugin_widget = app_plugin_widget
+            plugin_widget = app_plugin_widget,
+            service_name = service_name
         };
 
         application_rows.prepend (application_row);

=== modified file 'src/cc-credentials-account-details-page.vala'
--- src/cc-credentials-account-details-page.vala	2012-10-25 14:42:58 +0000
+++ src/cc-credentials-account-details-page.vala	2013-02-18 11:18:30 +0000
@@ -38,6 +38,91 @@
 }
 
 /**
+ * Switch widget, additionally storing an AccountApplicationRow.
+ */
+private class Cc.Credentials.AccountApplicationSwitch : Gtk.Switch
+{
+    public Ag.Account account { get; construct; }
+    public Ag.Service service { get; construct; }
+    bool store_in_progress;
+
+    public AccountApplicationSwitch (Ag.Account acc, Ag.Service serv)
+    {
+        Object (account: acc, service: serv);
+    }
+
+    construct
+    {
+        store_in_progress = false;
+
+        // Fetch current state from service.
+        account.select_service (service);
+        active = account.get_enabled ();
+        account.select_service (null);
+
+        account.set_data ("switch", this);
+        account.enabled.connect (on_app_account_enabled);
+        set_tooltip_text (_("Control whether this application integrates with Online Accounts"));
+        notify["active"].connect (on_app_switch_activated);
+    }
+
+    /**
+     * Handle the account being enabled or disabled and update the switch state
+     * accordingly.
+     */
+    private void on_app_account_enabled (Ag.Account account, string service,
+                                         bool enabled)
+    {
+        // If the global account is toggled, toggle the switch sensitivity.
+        if (service == "global" || service == null)
+        {
+            sensitive = enabled;
+            return;
+        }
+
+        if (this.service.get_name () == service && active != enabled)
+        {
+            active = enabled;
+        }
+    }
+
+    /**
+     * Handle the per-application switch being activated.
+     */
+    private void on_app_switch_activated ()
+    {
+        // Toggle the enabled state.
+        account.select_service (service);
+        account.set_enabled (active);
+        account.select_service (null);
+
+        try
+        {
+            account.store_blocking ();
+        }
+        catch (Ag.AccountsError err)
+        {
+            if (err is Ag.AccountsError.DELETED)
+            {
+                debug ("Enabled state changed during deletion of account ID: %u",
+                       account.id);
+            }
+            else if (err is Ag.AccountsError.STORE_IN_PROGRESS)
+            {
+                debug ("Enabled state changed while store in progress of account ID: %u",
+                       account.id);
+            }
+            else
+            {
+                critical ("Error changing enabled state of account: %s\nMessage: %s",
+                          account.get_display_name (),
+                          err.message);
+            }
+        }
+    }
+}
+
+/**
  * Web credentials account details widget. Used inside a notebook page to list
  * the applications that use an account, provide a switch to enable or disable
  * the account and to provide a button for removing the account.
@@ -60,6 +145,7 @@
     private Ag.Account current_account;
     private bool edit_options_button_present = false;
     private bool needs_attention = false;
+    private bool store_in_progress = false;
 
     /**
      * Pages for the action widget notebook.
@@ -434,6 +520,9 @@
                                           1,
                                           1);
 
+        var service = accounts_store.manager.get_service (application.service_name);
+        var app_switch = new AccountApplicationSwitch (account, service);
+
         if (application.plugin_widget != null)
         {
             var button = new AccountApplicationButton (_("Options"),
@@ -443,8 +532,18 @@
                                               Gtk.PositionType.RIGHT,
                                               1,
                                               1);
+            applications_grid.attach_next_to (app_switch,
+                                              button,
+                                              Gtk.PositionType.RIGHT,
+                                              1,
+                                              1);
+
             button.clicked.connect (on_options_button_clicked);
         }
+        else
+        {
+            applications_grid.attach (app_switch, 3, 0, 1, 1);
+        }
     }
 
     /**
@@ -557,33 +656,57 @@
     }
 
     /**
+     * Asynchronously store the current account.
+     */
+    private void store_current_account ()
+    {
+        if (store_in_progress)
+        {
+            debug ("Enabled state changed while store in progress of account ID: %u",
+                   account.id);
+            return;
+        }
+            
+        store_in_progress = true;
+
+        current_account.store_async.begin (null, (obj, res) =>
+        {
+            try
+            {
+                store_in_progress = false;
+
+                current_account.store_async.end (res);
+            }
+            catch (Ag.AccountsError err)
+            {
+                if (err is Ag.AccountsError.DELETED)
+                {
+                    debug ("Enabled state changed during deletion of account ID: %u",
+                           account.id);
+                }
+                else if (err is Ag.AccountsError.STORE_IN_PROGRESS)
+                {
+                    debug ("Enabled state changed while store in progress of account ID: %u",
+                           account.id);
+                }
+                else
+                {
+                    critical ("Error changing enabled state of account: %s\nMessage: %s",
+                              current_account.get_display_name (),
+                              err.message);
+                }
+            }
+        });
+    }
+
+    /**
      * Handle the account enabled switch being activated. Change the current
      * account to be the same state as the switch.
      */
     private void on_enabled_switch_activated ()
     {
         current_account.set_enabled (enabled_switch.active);
-
-        // FIXME: Use asynchronous account.store ().
-        try
-        {
-            current_account.store_blocking ();
-        }
-        catch (Ag.AccountsError err)
-        {
-            if (err is Ag.AccountsError.DELETED)
-            {
-                debug ("Enabled state changed during deletion of account ID: %u",
-                       account.id);
-                return;
-            }
-            else
-            {
-                critical ("Error changing enabled state of account: %s\nMessage: %s",
-                          current_account.get_display_name (),
-                          err.message);
-            }
-        }
+        store_current_account ();
     }
 
     /**
@@ -598,9 +721,14 @@
      * Handle the account being enabled or disabled from elsewhere, and update
      * the switch state accordingly.
      */
-    private void on_account_enabled ()
+    private void on_account_enabled (string? service, bool enabled)
     {
-        var enabled = current_account.get_enabled ();
+        // Ignore service-level changes.
+        // FIXME: http://code.google.com/p/accounts-sso/issues/detail?id=157
+        if (service != "global" || service != null)
+        {
+            return;
+        }
 
         if (enabled != enabled_switch.active)
         {

=== modified file 'src/cc-credentials-accounts-model.vala'
--- src/cc-credentials-accounts-model.vala	2012-09-28 10:23:35 +0000
+++ src/cc-credentials-accounts-model.vala	2013-02-18 11:18:30 +0000
@@ -425,13 +425,26 @@
                                      string? service,
                                      bool enabled)
     {
+        // Ignore service-level changes.
+        // FIXME: http://code.google.com/p/accounts-sso/issues/detail?id=157
+        if (service != "global" || service != null)
+        {
+            return;
+        }
+
         Gtk.TreeIter iter;
+        bool model_enabled;
         if (find_iter_for_account_id (account.id, out iter))
         {
-            this.set (iter,
-                      ModelColumns.ACCOUNT_DESCRIPTION, format_account_description (account),
-                      ModelColumns.ENABLED, enabled,
-                      -1);
+            this.get (iter, ModelColumns.ENABLED, out model_enabled);
+
+            if (model_enabled != enabled)
+            {
+                this.set (iter,
+                          ModelColumns.ACCOUNT_DESCRIPTION, format_account_description (account),
+                          ModelColumns.ENABLED, enabled,
+                          -1);
+            }
         }
         else
         {