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

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Richard Newman [:rnewman]]

Putting this in Sync UI for now.

Comment 1

[Madhava Enros [:madhava]]

Could be on our about:home, which is our start page (not new tab page) for now.

Comment 2

[Richard Newman [:rnewman]]

Attachment: WIP for Part 1.

This puts a couple of links, with localization entities, on the Fennec about:home page.

Comment 3

[Richard Newman [:rnewman]]

Attachment: WIP (untested) for importing WeaveGlue.

This hasn't even been run. Uploading for completeness.

Comment 4

[Philipp von Weitershausen [:philikon]]

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.

Comment 5

[Allison Naaktgeboren :ally]

Attachment: WIP, largely saving for the formatting

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.

Comment 6

[Allison Naaktgeboren :ally]

Attachment: Part 1 (v0) Pair Device on Mobile Home tab

Comment 7

[Philipp von Weitershausen [:philikon]]

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!

Comment 8

[Allison Naaktgeboren :ally]

> 
> >+#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.

Comment 9

[Philipp von Weitershausen [:philikon]]

(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.

Comment 10

[Allison Naaktgeboren :ally]

Attachment: Part 1 (v1) Pair Device on Mobile Home tab

Nits addressed. Philikon, should I just hold it until after the discussion tomorrow for the correct string?

Comment 11

[Philipp von Weitershausen [:philikon]]

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!

Comment 12

[Allison Naaktgeboren :ally]

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?

Comment 13

[Allison Naaktgeboren :ally]

Attachment: Screenshot of single button

Comment 14

[Philipp von Weitershausen [:philikon]]

(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.

Comment 15

[Allison Naaktgeboren :ally]

Attachment: Part 1 (v2) Pair Device on Mobile Home tab

Comment 16

[Allison Naaktgeboren :ally]

Attachment: screenshot of issue after disconnect

Comment 17

[Allison Naaktgeboren :ally]

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

Comment 18

[Philipp von Weitershausen [:philikon]]

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 :))

Comment 19

[Allison Naaktgeboren :ally]

(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?

Comment 20

[Philipp von Weitershausen [:philikon]]

(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.

Comment 21

[Allison Naaktgeboren :ally]

Attachment: Part 1 (v3) Pair Device on Mobile Home tab

Comment 22

[Philipp von Weitershausen [:philikon]]

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?

Comment 23

[Allison Naaktgeboren :ally]

Attachment: Screenshot of single button

Comment 24

[Allison Naaktgeboren :ally]

Attachment: Part 1 (v4) Pair Device on Mobile Home tab

Comment 25

[Alex Faaborg [:faaborg] (Firefox UX)]

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.

Comment 26

[Richard Newman [:rnewman]]

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.

Comment 27

[Tracy Walker [:tracy]]

on galaxy tab link correctly opens easy setup dialog.  The link is not present if Sync is setup already.

Comment 28

[Philipp von Weitershausen [:philikon]]

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

Comment 29

[Cristian Nicolae (:xti)]

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

Comment 30

[Aaron Train [:aaronmt]]

(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

Comment 31

[Tracy Walker [:tracy]]

(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.

Comment 32

[Cristian Nicolae (:xti)]

(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.

Comment 33

[Cristian Nicolae (:xti)]

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