Closed Bug 675820 Opened 9 years ago Closed 8 years ago

"Set up Sync" link at bottom of home tab: mobile

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 10

People

(Reporter: rnewman, Assigned: ally)

References

Details

(Whiteboard: [verfied in services])

Attachments

(2 files, 9 obsolete files)

Putting this in Sync UI for now.
Blocks: 675826
Could be on our about:home, which is our start page (not new tab page) for now.
Attached patch WIP for Part 1. (obsolete) — Splinter Review
This puts a couple of links, with localization entities, on the Fennec about:home page.
This hasn't even been run. Uploading for completeness.
Component: Firefox Sync: UI → General
Product: Mozilla Services → Fennec
QA Contact: sync-ui → general
Version: unspecified → Trunk
I think we want to say at least "Pair *this* Device" to incidate the difference between the "I want to connect this device to another device" and "There's this other device that I want to connect to this one".

Also, once the phone is paired, it can serve as a pairing partner, so we will also need "Pair a Device". At which point we should ask ourselves whether this isn't a bit confusing and we should get better strings for those two cases.
Summary: "Pair device" link at bottom of home tab: mobile → "Pair this Device" link at bottom of home tab: mobile
We will eventually have a "Setup Sync on desktop". This patch is for potential future work when there will be two buttons. Both buttons currently do the same thing, prettily.
Attachment #562917 - Flags: feedback?(philipp)
Attachment #555589 - Attachment is obsolete: true
Attachment #555588 - Attachment is obsolete: true
Comment on attachment 562917 [details] [diff] [review]
Part 1 (v0) Pair Device on Mobile Home tab

>       <div id="footer-wrapper">
>         <span id="feedback" style="width: &aboutHome.footerWidth;" class="section-row" pref="app.feedbackURL" onclick="openLink(this);" role="button">&aboutHome.giveFeedback;</span
>         ><span id="support" style="width: &aboutHome.footerWidth;" class="section-row" pref="app.support.baseURL" onclick="openLink(this);" role="button">&aboutHome.getHelp;</span>
>       </div>
>-
>+      <div id="sync-setup">
>+        <span id="syncAddDevice"
>+              class="section-row" style="width: &aboutHome.footerWidth;" 
>+              role="button"
>+              onclick="openPairADeviceWizard()"
>+              >
>+          &aboutHome.addDevice;
>+        </span>
>+      </div>

If the coding style were up to me, I'd prefer to put the 'style' attr on its own line and put the closing bracket (>) on the same line as the 'onclick' attr.

That said, the predominant style in this document is putting everything on the same line (see the <span>s in the footer-wrapper <div> above), and predominant style wins. So please adhere to that.

>@@ -483,11 +496,11 @@
>        }, false);
> 
>        // Giving a chance for any page transitions to happen, if any, before displaying the lightbox
>        setTimeout(function() {
>          document.getElementById("lightbox").style.display = "block";
>        }, 300);
>      }
>    }
>-  ]]></script>
>+   ]]></script>

Unrelated whitespace change. Please remove.

>+#sync-setup {
>+  font-size: 18px;
>+  margin-top: 24px;
>+  text-align: center;
>+}

This doesn't seem relevant to this patch. Left-over stuff?


Strings may be subject to change, of course. I'll talk to faaborg about them tomorrow. Looks good otherwise!
Attachment #562917 - Flags: feedback?(philipp) → feedback+
> 
> >+#sync-setup {
> >+  font-size: 18px;
> >+  margin-top: 24px;
> >+  text-align: center;
> >+}
> 
> This doesn't seem relevant to this patch. Left-over stuff?
> 

'sync-setup' is the name of the div that contains the button, so it is still a relevant rule, as far as I understand. I know it doesn't look right if I remove it.
(In reply to :Ally Naaktgeboren from comment #8)
> 'sync-setup' is the name of the div that contains the button, so it is still
> a relevant rule, as far as I understand. I know it doesn't look right if I
> remove it.

Duh, you're right. Ignore me.
Nits addressed. Philikon, should I just hold it until after the discussion tomorrow for the correct string?
Attachment #562917 - Attachment is obsolete: true
Spoke to Faaborg about the inconsistency of strings and the meaning of "Set up Sync" vs "Pair a Device":

* "Set up Sync" always means I'm configuring the local device for Sync.

* "Pair a Device" means I'm the master and I'm pairing with a second slave device that will receive my credentials. IN fact, we're going to call this "Pair a Device with an Activation Code" to make it crystal clear.

Eventually mobile will have both links, but for now only the first one is relevant (until we implement bug 624028).

So Ally, please change the string from "Pair a Device" to "Set up Sync". Also, can you provide a screenshot please? Thanks!
Summary: "Pair this Device" link at bottom of home tab: mobile → "Set up Sync" link at bottom of home tab: mobile
philikon: Dumb question, but should the button disappear if sync is already set up on the device? Ultimately he pairing button will appear in its stead, but what about the intermediate state we are in now? 

The code as it exists now will not, but there is no feedback suggesting that I should change it. thoughts?
Attached image Screenshot of single button (obsolete) —
(In reply to :Ally Naaktgeboren from comment #12)
> philikon: Dumb question, but should the button disappear if sync is already
> set up on the device? Ultimately he pairing button will appear in its stead,
> but what about the intermediate state we are in now? 
> 
> The code as it exists now will not, but there is no feedback suggesting that
> I should change it. thoughts?

Oh yeah, good point. Yes, the link should of course disappear when you already have Sync set up. The easiest way to test for that is to check whether the 'services.sync.username' pref is set and a non-empty string.
Attachment #562911 - Attachment is obsolete: true
Attachment #562932 - Attachment is obsolete: true
Attachment #563256 - Flags: feedback?(philipp)
Attached image screenshot of issue after disconnect (obsolete) —
Comment on attachment 563256 [details] [diff] [review]
Part 1 (v2) Pair Device on Mobile Home tab

once the borked profile was removed, testing went fine. ready for review
Attachment #563256 - Flags: feedback?(philipp) → review?(philipp)
Comment on attachment 563256 [details] [diff] [review]
Part 1 (v2) Pair Device on Mobile Home tab

>+   function initSetupSync() {
>+    if (getChromeWin().Services.prefs.prefHasUserValue("services.sync.username")) {
>+      document.getElementById("syncSetupSync").style.display = "none";
>+    }
>+   }
>+
>+   function openSetupSyncWizard() {
>+     let chromeWin = getChromeWin();
>+     chromeWin.WeaveGlue.open();
>+   }

This is a good start, but not enough. If we get to the Sync wizard from the Home tab and then once it completes, we get back to it, it should no longer say "Set Up Sync". The link should disappear automatically, without the need for a manual reload. I think the best way is to observe the 'weave:service:setup-complete' notification, as well as 'weave:service:start-over'. The former will tell you that the device was just connected to Sync, the latter that the device was disconnected. In those observers you want to flip the visibility of the link accordingly. There's already examples for observers and how to clean them up properly in aboutHome.xhtml ({update|init|uninit}Addons).

> +<!ENTITY aboutHome.setupSync  "Setup Sync">

Our existing strings say: Set Up Sync (space between Set and Up.) See also: title of the bug :) (Also, I don't believe "setup" is actually a verb, it's a noun. But I may be wrong. Playing the ESL card here :))
Attachment #563256 - Flags: review?(philipp) → review-
(In reply to Philipp von Weitershausen [:philikon] from comment #18)
> Comment on attachment 563256 [details] [diff] [review] [diff] [details] [review]
> Part 1 (v2) Pair Device on Mobile Home tab
> 
> >+   function initSetupSync() {
> >+    if (getChromeWin().Services.prefs.prefHasUserValue("services.sync.username")) {
> >+      document.getElementById("syncSetupSync").style.display = "none";
> >+    }
> >+   }
> >+
> >+   function openSetupSyncWizard() {
> >+     let chromeWin = getChromeWin();
> >+     chromeWin.WeaveGlue.open();
> >+   }
> 
> This is a good start, but not enough. If we get to the Sync wizard from the
> Home tab and then once it completes, we get back to it, it should no longer
> say "Set Up Sync". The link should disappear automatically, without the need
> for a manual reload. I think the best way is to observe the
> 'weave:service:setup-complete' notification, as well as
> 'weave:service:start-over'. The former will tell you that the device was
> just connected to Sync, the latter that the device was disconnected. In
> those observers you want to flip the visibility of the link accordingly.
> There's already examples for observers and how to clean them up properly in
> aboutHome.xhtml ({update|init|uninit}Addons).
> 
what about just reloading the page? it is probably simpler. would using observers buy us anything in terms of speed, performance, or memory usage?
(In reply to :Ally Naaktgeboren from comment #19)
> what about just reloading the page? it is probably simpler. would using
> observers buy us anything in terms of speed, performance, or memory usage?

For sure. Reloading the page creates flickering and lots of activity that we can easily avoid. Also, you would still have to know *when* to reload, right? So I'm not sure how you can get out of using observers in the first place.
Attachment #563256 - Attachment is obsolete: true
Attachment #563259 - Attachment is obsolete: true
Attachment #563638 - Flags: feedback?(philipp)
Comment on attachment 563638 [details] [diff] [review]
Part 1 (v3) Pair Device on Mobile Home tab

>+      <div id="sync-setup">
>+        <a id="syncSetupSync" class="setup-link" href="#" onclick="openSetupSyncWizard();">&aboutHome.setupSync;</a>
>+      </div>

...

>+.setup-link { 
>+  text-decoration: underline; 
>+  color: blue; 
>+} 

Micro-optimization nit: a class rule will be slightly slower than an ID rule because it's an extra thing to check, so I suggest removing the "setup-link" class and making this a #syncSetupSync rule.

>+   function syncDisconnected() {
>+     /*todo double check that this is the correct way to turn the button back on*/
>+     document.getElementById("syncSetupSync").style.display = "inline";
>+   }

I don't see any other good way, but feel free to double check :)

>+#sync-setup {
>+  font-size: 18px;
>+  margin-top: 24px;
>+  text-align: center;
>+}
>+
>+.setup-link { 
>+  text-decoration: underline; 
>+  color: blue; 
>+} 

Can you upload a screenshot for faaborg to ui-r please?
Attachment #563638 - Flags: feedback?(philipp) → feedback+
Attachment #563175 - Attachment is obsolete: true
Attachment #563654 - Flags: ui-review?(faaborg)
Attachment #563638 - Attachment is obsolete: true
Attachment #563656 - Flags: review?(philipp)
Attachment #563656 - Flags: review?(philipp) → review+
Comment on attachment 563654 [details]
Screenshot of single button

works for me, consistent with the home tab on desktop.  perhaps not as high profile as we might like for conversion but we can worry about that later.
Attachment #563654 - Flags: ui-review?(faaborg) → ui-review+
Assignee: nobody → ally
Verified and pushed to s-c on ally's behalf, because my machine finished an incremental build waaaaay faster than her MBA can do a clean build.

  https://hg.mozilla.org/services/services-central/rev/329e56f80534

I'll leave you guys to write up appropriate verification steps for QA to incorporate into their test suite.
Whiteboard: [fixed in services]
Attachment #563656 - Attachment is patch: true
on galaxy tab link correctly opens easy setup dialog.  The link is not present if Sync is setup already.
Whiteboard: [fixed in services] → [verfied in services]
https://hg.mozilla.org/mozilla-central/rev/329e56f80534
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 10
This bug is verified fixed on the latest Nightly build. However, the Set Up Sync and Tabs from your other computer buttons are doing the same thing. Should we duplicate this option on the about:home page?
IMO, I guess that it would be better that the name of "Tabs from your other computer" to be changed in "Set Up Sync" until the Sync is connected. After this step, the button name will be changed back to "Tabs from your other computer". 

--
Mozilla/5.0 (Android;Linux armv7l;rv:10.0a1)Gecko/20111005
Firefox/10.0a1 Fennec/10.0a1
Device: Samsung Galaxy S
OS: Android 2.2
(In reply to Cristian Nicolae (:xti) from comment #29)
> IMO, I guess that it would be better that the name of "Tabs from your other
> computer" to be changed in "Set Up Sync" until the Sync is connected. After
> this step, the button name will be changed back to "Tabs from your other
> computer". 
> OS: Android 2.2

+1
(In reply to Cristian Nicolae (:xti) from comment #29)
> This bug is verified fixed on the latest Nightly build. However, the Set Up
> Sync and Tabs from your other computer buttons are doing the same thing.
> Should we duplicate this option on the about:home page?
> IMO, I guess that it would be better that the name of "Tabs from your other
> computer" to be changed in "Set Up Sync" until the Sync is connected. After
> this step, the button name will be changed back to "Tabs from your other
> computer". 
> 
> --
> Mozilla/5.0 (Android;Linux armv7l;rv:10.0a1)Gecko/20111005
> Firefox/10.0a1 Fennec/10.0a1
> Device: Samsung Galaxy S
> OS: Android 2.2

This bug is fixed as filed. Please file a follow up bug and mark it blocking the set up tracking bug 675826. Thank you.
(In reply to Tracy Walker [:tracy] from comment #31)
> 
> This bug is fixed as filed. Please file a follow up bug and mark it blocking
> the set up tracking bug 675826. Thank you.

I've filled Bug 692104.
Verified fixed on:
Mozilla/5.0 (Android;Linux armv7l;rv:10.0a1)Gecko/20111004
Firefox/10.0a1 Fennec/10.0a1
Device: Samsung Galaxy S
OS: Android 2.2
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.