Online Accounts: GNOME Control Center

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

switch-mockup
Merge into trunk

Proposed by David King on 2013-02-11

Status:

Merged

Approved by:

Alberto Mardegan on 2013-02-18

Approved revision:

143

Merged at revision:

134

Proposed branch:

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

Merge into:

lp://staging/gnome-control-center-signon

Diff against target:

294 lines (+173/-28)

3 files modified

src/cc-credentials-account-applications-model.vala (+5/-1)
src/cc-credentials-account-details-page.vala (+151/-23)
src/cc-credentials-accounts-model.vala (+17/-4)

To merge this branch:

bzr merge lp://staging/~amigadave/gnome-control-center-signon/switch-mockup

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Alberto Mardegan (community)		2013-02-11	Approve on 2013-02-18
PS Jenkins bot (community)	continuous-integration		Approve on 2013-02-18
Review via email: mp+147624@code.staging.launchpad.net

Commit message

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

Description of the change

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

Revision history for this message

Alberto Mardegan (mardy) wrote on 2013-02-11:

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

Revision history for this message

Alberto Mardegan (mardy) wrote on 2013-02-11:

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://staging/~amigadave/gnome-control-center-signon/switch-mockup updated on 2013-02-11

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