Last Comment Bug 716906 - Add "Firefox Sync" item to settings
: Add "Firefox Sync" item to settings
Status: VERIFIED FIXED
: late-l10n
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: ARM Android
: P2 normal (vote)
: Firefox 12
Assigned To: Brian Nicholson (:bnicholson)
:
Mentors:
Depends on:
Blocks: 698594 720734
  Show dependency treegraph
 
Reported: 2012-01-10 08:37 PST by Madhava Enros [:madhava]
Modified: 2016-07-29 14:21 PDT (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
verified
11+


Attachments
patch (7.53 KB, patch)
2012-01-23 13:06 PST, Brian Nicholson (:bnicholson)
no flags Details | Diff | Splinter Review
patch v2 (7.43 KB, patch)
2012-01-23 13:10 PST, Brian Nicholson (:bnicholson)
no flags Details | Diff | Splinter Review
patch v3 (7.37 KB, patch)
2012-01-23 13:25 PST, Brian Nicholson (:bnicholson)
doug.turner: review+
Details | Diff | Splinter Review
patch v4 (6.96 KB, patch)
2012-01-25 14:28 PST, Brian Nicholson (:bnicholson)
doug.turner: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Madhava Enros [:madhava] 2012-01-10 08:37:58 PST
This is covered in the mockups in the meta bug (Bug 698594), but here it is broken out explicitly.

In addition to inviting users to set up sync on the start page (Bug 708414), we have to have an entry point for it in Settings as well.

This is the overview of what this leads to, before and after sync is set up:
http://www.flickr.com/photos/madhava_work/6360153407/in/photostream

Instead of a whole Sync section of prefs, as we used to have, this is a one-liner that leads to other screens, so we can just put this in the "General" section.
Comment 1 Axel Hecht [:Pike] 2012-01-17 10:50:45 PST
I expect this to come with strings?
Comment 2 Madhava Enros [:madhava] 2012-01-18 09:23:26 PST
(In reply to Axel Hecht [:Pike] from comment #1)
> I expect this to come with strings?

Yes. It will just be "Sync"
Comment 3 Brian Nicholson (:bnicholson) 2012-01-23 13:06:24 PST
Created attachment 590840 [details] [diff] [review]
patch
Comment 4 Brian Nicholson (:bnicholson) 2012-01-23 13:10:14 PST
Created attachment 590842 [details] [diff] [review]
patch v2

some minor refactoring
Comment 5 Brian Nicholson (:bnicholson) 2012-01-23 13:25:16 PST
Created attachment 590848 [details] [diff] [review]
patch v3

ARGH. removed unused var. last fix, hopefully.
Comment 6 Doug Turner (:dougt) 2012-01-23 13:49:44 PST
Comment on attachment 590848 [details] [diff] [review]
patch v3

Review of attachment 590848 [details] [diff] [review]:
-----------------------------------------------------------------

with those changes.

::: mobile/android/base/SyncPreference.java.in
@@ +14,5 @@
> + *
> + * The Original Code is Mozilla Android code.
> + *
> + * The Initial Developer of the Original Code is Mozilla Foundation.
> + * Portions created by the Initial Developer are Copyright (C) 2009-2011

2012

@@ +47,5 @@
> +import android.util.AttributeSet;
> +import android.util.Log;
> +
> +class SyncPreference extends Preference {
> +    private static final String LOGTAG = "GeckoSyncPreference";

Not used

@@ +49,5 @@
> +
> +class SyncPreference extends Preference {
> +    private static final String LOGTAG = "GeckoSyncPreference";
> +    private static final String FEEDS_PACKAGE_NAME = "com.android.providers.subscribedfeeds";
> +    private static final String ACCOUNTSYNC_CLASS_NAME = "com.android.settings.AccountSyncSettings";

ACCOUNT_SYNC_CLASS_NAME instead.

@@ +66,5 @@
> +    @Override
> +    protected void onClick() {
> +        Intent intent = new Intent(Intent.ACTION_MAIN);
> +        Account[] accounts = AccountManager.get(mContext).getAccountsByType(FENNEC_ACCOUNT_TYPE);
> +        if (accounts.length > 0) {

What happens if there are more than one account?
Comment 7 Brad Lassey [:blassey] (use needinfo?) 2012-01-23 13:49:57 PST
Comment on attachment 590848 [details] [diff] [review]
patch v3

Review of attachment 590848 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/SyncPreference.java.in
@@ +69,5 @@
> +        Account[] accounts = AccountManager.get(mContext).getAccountsByType(FENNEC_ACCOUNT_TYPE);
> +        if (accounts.length > 0) {
> +            // show sync account
> +            intent.setComponent(new ComponentName(FEEDS_PACKAGE_NAME, ACCOUNTSYNC_CLASS_NAME));
> +            Account account = accounts[0];

what if there is more than one account?
Comment 8 Brian Nicholson (:bnicholson) 2012-01-23 14:02:26 PST
I was under the assumption that we will not support multiple Sync accounts. If we do, we need to modify the flow described here:
http://www.flickr.com/photos/madhava_work/6360153407/in/photostream

The Sync settings screen (show at the bottom of that diagram) is a per-account screen. The "Sync" preference in Fennec, however, is a general setting that does not correspond to a specific account. If we do support multiple Sync accounts, the Sync pref in Fennec should probably point to the general Android "My Accounts" screen (second image from the top left).
Comment 9 Madhava Enros [:madhava] 2012-01-23 14:18:42 PST
Yes, if/when we do multiple accounts we'd have to sort out that flow (as with going to the accounts screen rather than the particular account), but for now -- just one account.
Comment 10 Brian Nicholson (:bnicholson) 2012-01-23 14:41:08 PST
Landed on inbound with nits fixed:
http://hg.mozilla.org/integration/mozilla-inbound/rev/1b2d36e0657b

Also included a comment about the single account assumption.
Comment 11 Marco Bonardo [::mak] 2012-01-24 04:58:44 PST
https://hg.mozilla.org/mozilla-central/rev/1b2d36e0657b
Comment 12 Doug Turner (:dougt) 2012-01-25 11:35:27 PST
backed out on inbound:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/a79f4ada8343
Comment 13 Brian Nicholson (:bnicholson) 2012-01-25 11:50:30 PST
Reopened due to bug 720734.

I got the "Accounts & Sync" class name from logcat (and also discovered the necessary "account" extra by looking at the Android source) on a pre-ICS phone. However, we are crashing in ICS because this class name is different on ICS. According to rnewman and liuche, there's no reliable way to directly open sync settings for a specific account. That leaves us with 2 options:
1) open the generic Accounts & Sync activity (this is consistent with other apps, such as Gmail and Contacts)
2) create our own custom layout/activity for modifying the sync settings (this is what Twitter does)
Comment 14 Brian Nicholson (:bnicholson) 2012-01-25 14:28:02 PST
Created attachment 591610 [details] [diff] [review]
patch v4

This patch associates the Settings > Sync option with the generic "Accounts & Sync" page. We may want to have a custom Sync preferences page in the future, but I think this is sufficient for now as bare-bones functionality.
Comment 15 Brian Nicholson (:bnicholson) 2012-01-25 14:47:24 PST
Landed fixed patch on inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/50a68eb941f9
Comment 16 Ed Morley [:emorley] 2012-01-25 17:55:38 PST
(In reply to Doug Turner (:dougt) from comment #12)
> backed out on inbound:
>  https://hg.mozilla.org/integration/mozilla-inbound/rev/a79f4ada8343

Now backed out of m-c (https://hg.mozilla.org/mozilla-central/rev/a79f4ada8343), the relanding didn't make this merge (not yet PGO green), will be in the next.
Comment 17 Ed Morley [:emorley] 2012-01-26 04:36:21 PST
(In reply to Brian Nicholson (:bnicholson) from comment #15)
> Landed fixed patch on inbound:
> http://hg.mozilla.org/integration/mozilla-inbound/rev/50a68eb941f9

https://hg.mozilla.org/mozilla-central/rev/50a68eb941f9
Comment 18 Aaron Train [:aaronmt] 2012-01-26 06:47:16 PST
Verified Fixed on M-C
Samsung Galaxy Nexus (Android 4.0.3)
20120126031113
http://hg.mozilla.org/mozilla-central/rev/402b394b6623

Aurora nom?
Comment 19 Brian Nicholson (:bnicholson) 2012-01-26 11:26:21 PST
Comment on attachment 591610 [details] [diff] [review]
patch v4

[Approval Request Comment]
Adds Sync to settings. Mobile only. Contains string changes. Low risk.
Comment 20 Madhava Enros [:madhava] 2012-01-26 12:44:17 PST
Not in today's nightly? Did it get removed?
Comment 21 Brian Nicholson (:bnicholson) 2012-01-26 13:20:45 PST
(In reply to Madhava Enros [:madhava] from comment #20)
> Not in today's nightly? Did it get removed?

Yes, due to crashing on ICS - see comments 13/14. It should be in the next nightly.
Comment 22 Alex Keybl [:akeybl] 2012-01-27 16:19:11 PST
Comment on attachment 591610 [details] [diff] [review]
patch v4

[Triage Comment]
Mobile only - approved for Aurora.
Comment 23 Brad Lassey [:blassey] (use needinfo?) 2012-01-30 13:05:14 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/306e0b45c2e7

Note You need to log in before you can comment on or make changes to this bug.