Closed Bug 675824 Opened 9 years ago Closed 8 years ago

End mobile setup wizard with first sync download status/progress bar

Categories

(Firefox :: Sync, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla10

People

(Reporter: rnewman, Assigned: ally)

References

Details

(Whiteboard: [verified in services])

Attachments

(3 files, 10 obsolete files)

109.67 KB, image/png
philikon
: feedback+
Details
99.98 KB, image/png
philikon
: feedback+
Details
11.39 KB, patch
philikon
: review+
Details | Diff | Splinter Review
2 bars, one for desktop, one for mobile

    2 phase:

    hi, waiting for data to make it up to the server, indeterminate progress bar

    once it can download data to the phone, progress bar on download

    mobile: requires explicit exit from status page.

    desktop: second desktop should start downloading as soon as the second computer is hooked in.
Blocks: 675826
Assignee: nobody → ally
Attachment #567908 - Flags: feedback?
Attached patch Part 1 (v0) sync progress bar (obsolete) — Splinter Review
Attachment #567908 - Attachment is obsolete: true
Attachment #567908 - Flags: feedback?
Attachment #568179 - Flags: feedback?(philipp)
Comment on attachment 568179 [details] [diff] [review]
Part 1 (v0) sync progress bar

>       onComplete: function onComplete(aCredentials) {
>+

Nit: spurious whitespace. Please remove.

>         self.jpake = null;
>-        self.close();
>+
>+        let progressBar = document.getElementById("syncsetup-progressbar");
>+        progressBar.mode = "determined";
>+        progressBar.max = Weave.Engines.getEnabled().length;
>+        progressBar.value = 0;
>+        document.getElementById("syncsetup-waiting-desc").hidden = true;
>+        document.getElementById("syncsetup-waitingdownload-desc").hidden = false;
>+
>+        onFirstSync: function onFirstSync() {

I'm stumped that this is valid syntax! That `onFirstSync: ` stuff is for defining methods on objects, but here you're just defining a function. Please remove!

>+          let counter = parseInt(progressBar.value) + 1;

Why are you doing this? You could just increment `counter` as a number. (You need to define and initialize `counter` outside the scope of this function, of course.)

(Also, if you were using parseInt, make sure to always use it with the base argument, like parseInt(str, 10).)

>+          progressBar.setAttribute("value", counter);
>+          if(progressBar.value == progressBar.max) {

Nit: space after `if`. Also, we should really be checking `counter` here against Weave.Engines.getEnabled().length (please save the latter expression in a variable to avoid recomputing it everytime.)

>+            Services.obs.removeObserver(onFirstSync,"weave:engine:sync:finish",false);
>+            Services.obs.removeObserver(onFirstSync,"weave:engine:sync:error",false);
>+            self.close();

Hmm, do we want to automatically close here? We're not doing that on desktop, and comment 0 say "mobile: requires explicit exit from status page." Not sure what exactly rnewman meant with this somewhat cryptic comment. Then again, since on mobile the wizard is essentially modal, maybe we do? I can see good points for both. Will try to get some UX feedback here.

>+        Services.obs.addObserver(onFirstSync,"weave:engine:sync:finish",false);
>+        Services.obs.addObserver(onFirstSync,"weave:engine:sync:error",false);

Nit: space after commas (also on the removeObserver calls above.)

>@@ -221,16 +242,22 @@
> 
>     // Clear the UI so it's ready for next time
>     this._elements.account.value = "";
>     this._elements.password.value = "";
>     this._elements.synckey.value = "";
>     this._elements.usecustomserver.checked = false;
>     this._elements.customserver.disabled = true;
>     this._elements.customserver.value = "";
>+    document.getElementById("syncsetup-waiting-desc").hidden = false;
>+    document.getElementById("syncsetup-waitingdownload-desc").hidden = true;
>+    let progressBar = document.getElementById("syncsetup-progressbar");
>+    progressBar.max= 0 ;

Nit: spaces are all sorts of wrong here :)

>+    progressBar.value = 0;
>+    progressBar.mode = "indeterminate";
> 
>     // Close the connect UI
>     document.getElementById("syncsetup-container").hidden = true;
>     BrowserUI.popDialog();
>   },
> 
>   toggleCustomServer: function toggleCustomServer() {
>     let useCustomServer = this._elements.usecustomserver.checked;
>diff --git a/mobile/locales/en-US/chrome/sync.dtd b/mobile/locales/en-US/chrome/sync.dtd
>--- a/mobile/locales/en-US/chrome/sync.dtd
>+++ b/mobile/locales/en-US/chrome/sync.dtd
>@@ -15,8 +15,9 @@
> <!ENTITY sync.password              "Password">
> <!ENTITY sync.recoveryKey           "Recovery Key">
> <!ENTITY sync.customServer          "Use custom server">
> <!ENTITY sync.serverURL             "Server URL">
> <!ENTITY sync.setup.connect         "Connect">
> <!ENTITY sync.setup.cancel          "Cancel">
> <!ENTITY sync.setup.tutorial        "Show me how">
> <!ENTITY sync.setup.waiting         "Pairing in progressâ¦">
>+<!ENTITY sync.setup.waitingdownload "Your data is now being encrypted and downloaded in the background.">

This string doesn't make sense on several levels. First off, downloading goes with *de*crypting, not *en*crypting. Though I don't think it's very useful to go into that detail in the mobile UI. I think it's probably better to explain that users can close the dialog at this point. So I would say something like "Your data is now being downloaded in the background. You can close this window at any time."

On that note: I think the setup wizard displays a "Cancel" button at the bottom. Once the pairing is complete and we start syncing (onComplete() and onwards), we want to replace that with a "Close" button because at this point you can no longer cancel the setup -- we're already set up and just doing the first sync. You can just close the dialog at this point.
Attachment #568179 - Flags: feedback?(philipp)
Attached patch Part 1 (v1) sync progress bar (obsolete) — Splinter Review
Attachment #568179 - Attachment is obsolete: true
Attachment #568294 - Flags: feedback?(philipp)
Comment on attachment 568294 [details] [diff] [review]
Part 1 (v1) sync progress bar

>         <vbox id="syncsetup-waiting" class="syncsetup-page" flex="1" hidden="true">
>-          <description class="syncsetup-desc syncsetup-center" flex="1">&sync.setup.waiting;</description>
>+          <progressmeter id="syncsetup-progressbar" mode="undetermined"/>
>+          <description id="syncsetup-waiting-desc" class="syncsetup-desc syncsetup-center" flex="1">&sync.setup.waiting;</description>
>+          <description id="syncsetup-waitingdownload-desc" class="syncsetup-desc syncsetup-center" flex="1">&sync.setup.waitingdownload;</description>

Add a default hidden="true"? You get to save some code down below.

>+         <hbox class="prompt-buttons" pack="center">
>+            <button id= "syncsetup-waiting-close" class="prompt-button" oncommand="WeaveGlue.close();">&sync.setup.close;</button>

Same here.

>+            <separator/>
>+        </hbox>
>         </vbox>

Nit: indent the <hbox> properly (2 spaces below the <vbox>, and its children 2 spaces below that).

>       try {
>         this._elements.device.value = Services.prefs.getCharPref("services.sync.client.name");
>       } catch(e) {}
>     } else if (Weave.Status.login != Weave.LOGIN_FAILED_NO_USERNAME) {
>       this.loadSetupData();
>     }
>+    this._progressBar = document.getElementById("syncsetup-progressbar");
>+
>   },

Nit: remove empty line before closing brace.

>     // Show the connect UI
>     container.hidden = false;
>     document.getElementById("syncsetup-simple").hidden = false;
>     document.getElementById("syncsetup-waiting").hidden = true;
>     document.getElementById("syncsetup-fallback").hidden = true;
>+    document.getElementById("syncsetup-waitingdownload-desc").hidden = true;
>+    document.getElementById("syncsetup-waiting-close").hidden = true;

With the default hidden="true" attributes in XUL and proper clean up in close() (which you have), this isn't necessary.

>       onComplete: function onComplete(aCredentials) {
>         self.jpake = null;
>-        self.close();
>+        self.onSyncFinish = self.onFirstSync.bind(self);

You can do this just once in init()! Also, why two different names for the same method?

>+  onFirstSync: function onFirstSync() {

This name is not very accurate. I suggest something like onEngineSync(). Or just take some inspiration from your work on the desktop progress bar, e.g. increaseFirstSyncProgressBar.

>+    this._progressValue += 1;
>+    this._progressBar.setAttribute("value", this._progressValue);
>+    if (this._progressMax == null) {
>+    this._progressMax = Weave.Engines.getEnabled().length;
>+    this._progressBar.max = this._progressMax;
>+    }

First off, we should put a comment here explaining why we're initializing _progressMax lazily here (because we don't know which/how many engines are going to be enabled until we sync). On that note, it occurred to me that the special "clients" engine syncs before we even find that out! So to that end we want to add a check here:

  if (this._progressMax == null && data != "clients") {

'data' is the nsIObserver::observe() argument that we get passed into this function.

Also nit: Indention!

>+    if (this._progressValue >= this._progressMax) {
>+      this.close();
>+    }

It just occurred to me that listening to "weave:service:sync:finish" / "weave:service:sync:error" is much more reliable, because if we error out mid-sync, we will never hit progressValue >= progressMax.

>   close: function close() {
>+    Services.obs.removeObserver(this.onSyncFinish, "weave:engine:sync:finish", false);
>+    Services.obs.removeObserver(this.onSyncFinish, "weave:engine:sync:error", false);
>+
>     if (this.jpake)
>       this.abortEasySetup();
>-

Spurious whitespace change. Please revert.

>+    document.getElementById("syncsetup-waiting-desc").hidden = false;
>+    document.getElementById("syncsetup-waitingdownload-desc").hidden = true;
>+    document.getElementById("syncsetup-waiting-close").hidden = true;
>+    this._progressMax = null;
>+    this._progressValue = 0;
>+    this._progressBar.max = 0;
>+    this._progressBar.value = 0;
>+    this._progressBar.mode = "indeterminate";

This is not a valid value for the "mode" attribute. I think you mean "undetermined"?
Attachment #568294 - Flags: feedback?(philipp)
>       onComplete: function onComplete(aCredentials) {
>         self.jpake = null;
>-        self.close();
>+        self.onSyncFinish = self.onFirstSync.bind(self);

You can do this just once in init()! Also, why two different names for the same method?

what do you mean two names for the same method? we're saving it so it can be used properly in the clean up in close so it would be invoked on the correct object and in the correct scope later.
(In reply to :Ally Naaktgeboren from comment #6)
> what do you mean two names for the same method? we're saving it so it can be
> used properly in the clean up in close so it would be invoked on the correct
> object and in the correct scope later.

Right. You're binding it to the object it's defined on so that every invocation, no matter how it's performed, will guarantee the right 'this' object. This is a pretty common pattern. My point is why do this.A = this.B.bind(this) rather than this.A = this.A.bind(this) or at least this._boundA = this.A.bind(this). Related names for related things are better than unrelated names :)
Attached patch Part 1 (v2) sync progress bar (obsolete) — Splinter Review
Attachment #568294 - Attachment is obsolete: true
Attachment #568571 - Flags: feedback?(philipp)
Comment on attachment 568571 [details] [diff] [review]
Part 1 (v2) sync progress bar

>+  onEngineSync: function onEngineSync(data) {
>+    this._progressValue += 1;
>+    this._progressBar.setAttribute("value", this._progressValue);
>+    // We don't know how many engines are going to be enabled until we
>+    // we sync.
>+    if (this._progressMax == null && data !="clients") {
>+      this._progressMax = Weave.Engines.getEnabled().length;

Yeah, so I kinda forgot the Clients engine here. My bad. This needs a + 1 (cf bug 696823)

>   close: function close() {
>+    Services.obs.removeObserver(this._boundOnEngineSync, "weave:engine:sync:finish");
>+    Services.obs.removeObserver(this._boundOnEngineSync, "weave:engine:sync:error");
>+    Services.obs.removeObserver(this._boundOnServiceSync, "weave:service:sync:finish");
>+    Services.obs.removeObserver(this._boundOnServiceSync, "weave:service:sync:error");

These can throw if the dialog gets closed before onComplete() has been called above. So let's wrap them in a try/catch block to make sure we don't error out here and won't perform any of the other clean up below.

r=me with those two things. Can you upload screenshots of the two stages?
Attachment #568571 - Flags: feedback?(philipp) → review+
Attachment #569156 - Attachment description: stage 1 screen shot → stage 2 screen shot, determinate progress bar
Attachment #569157 - Attachment description: stage 2 screen shot → stage 1 screen shot, pairing in progress indeterminate progress bar
STR for QA:
- applies to mobile only. no impact on desktop.
- click set up sync on about:home
- enter jpake code from mobile on desktop
- mobile will display stage 1: pairing in progress with indeterminate bar
-- should not have any buttons, cannot close during this
- once credentials are transferred to mobile & mobile starts syncing, text will change & progress bar will become determinate
-- progress will be uneven (1/n increment per engine)
- close button can be used and closes dialog
- when progress bar reaches 100%, dialog closes itself 
-- this is different from desktop progress bar page
Comment on attachment 568571 [details] [diff] [review]
Part 1 (v2) sync progress bar

Based on the screenshots, I'm withdrawing my r+. Just a few small UI nits:

* The undetermined progress bar page should probably keep the "Cancel" button that's visible on the previous page where the pairing code is shown.

* The "Close" button for the determined progress bar is not bottom-aligned like the Cancel button.
Attachment #568571 - Flags: review+
Attached image stage 1 screen shot, pairing in progress (obsolete) —
Attachment #569157 - Attachment is obsolete: true
Attachment #569431 - Flags: feedback?(philipp)
Attachment #569156 - Attachment is obsolete: true
Attachment #569433 - Flags: feedback?(philipp)
assuming the screen shots pass muster
Attachment #568571 - Attachment is obsolete: true
Attachment #569436 - Flags: feedback?(philipp)
Comment on attachment 569433 [details]
stage 2 screen shot, determinate progress bar during first sync

Nice!
Attachment #569433 - Flags: feedback?(philipp) → feedback+
Comment on attachment 569431 [details]
stage 1 screen shot, pairing in progress

The button should say "Cancel" because at this point closing that dialog will still have the side-effect of canceling the JPAKE exchange.
Attachment #569431 - Flags: feedback?(philipp) → feedback-
Comment on attachment 569436 [details] [diff] [review]
Part 1 (v3) sync progress bar mobile

>         <vbox id="syncsetup-waiting" class="syncsetup-page" flex="1" hidden="true">
>-          <description class="syncsetup-desc syncsetup-center" flex="1">&sync.setup.waiting;</description>
>+          <progressmeter id="syncsetup-progressbar" mode="undetermined"/>
>+          <vbox id="syncsetup-waiting-top" align="center" flex="1">
>+            <description id="syncsetup-waiting-desc" class="syncsetup-desc syncsetup-center" flex="1">&sync.setup.waiting;</description>
>+            <description id="syncsetup-waitingdownload-desc" class="syncsetup-desc syncsetup-center" hidden="true" flex="1">&sync.setup.waitingdownload;</description>
>+          </vbox>
>+          <hbox class="prompt-buttons" pack="center" align="end">
>+            <button id= "syncsetup-waiting-close" class="prompt-button" oncommand="WeaveGlue.close();">&sync.setup.close;</button>

Nit: no space after id=

And what I said in comment 13 and comment 18: While the progress bar is undetermined, the button should say Cancel.
Attachment #569436 - Flags: feedback?(philipp) → feedback+
Attachment #569431 - Attachment is obsolete: true
Attachment #569478 - Flags: feedback?(philipp)
Attachment #569436 - Attachment is obsolete: true
Attachment #569481 - Flags: review?(philipp)
Comment on attachment 569478 [details]
stage 1 indeterminate, pairing in progress

woo!
Attachment #569478 - Flags: feedback?(philipp) → feedback+
Comment on attachment 569481 [details] [diff] [review]
part 1 (v4) sync progress bar on mobile

Very nice! Shippit!
Attachment #569481 - Flags: review?(philipp) → review+
i thought the official mozilla term was 'land it' :D
(In reply to :Ally Naaktgeboren from comment #24)
> i thought the official mozilla term was 'land it' :D

Epic burn, ^5
Comment on attachment 569481 [details] [diff] [review]
part 1 (v4) sync progress bar on mobile

>+  onEngineSync: function onEngineSync(data) {

Just noticed this: 'data' is the 3rd argument for nsIObserver::observe, so you want onEngineSync(subject, topic, data)

>+    this._progressValue += 1;
>+    this._progressBar.setAttribute("value", this._progressValue);
>+    // We don't know how many engines are going to be enabled until we
>+    // we sync.
>+    if (this._progressMax == null && data !="clients") {
>+      // Account for additional notification from client sync engine in max 
>+      // calculation.
>+      this._progressMax = Weave.Engines.getEnabled().length + 1;
>+      this._progressBar.max = this._progressMax;
>+    }

After some discussion, we came to the conclusion that it'd be better to just ignore the clients collection altogether as it provides more predictable behaviour. So we can bail out of this function if data == "clients" and then use Weave.Engines.getEnabled().length (without the additional 1 for the clients engine) as the max.
Attachment #569481 - Flags: review+
Attachment #569481 - Attachment is obsolete: true
Attachment #569558 - Flags: feedback?(philipp)
Comment on attachment 569558 [details] [diff] [review]
part 1(v4) progress bar on mobile

>+  onEngineSync: function onEngineSync(subject, topic, data) {
>+    // We don't know how many engines are going to be enabled until we
>+    // we sync.

You just duplicated this comment from 3 lines below. A better comment to replace both of these would be something like:

  // The Clients engine syncs first. At this point we don't necessarily know yet
  // how many engines will be enabled, so we'll ignore the Clients engine and
  // evaluate how many engines are enabled when the first "real" engine syncs.

>+    if(data == 'clients') {

Nit: space after `if`

>+      return;
>+    }
>+    // We don't know how many engines are going to be enabled until we
>+    // we sync.
>+    if (this._progressMax == null && data !="clients") {

You can get rid of the `data != "clients"` check since we're now bailing out at the top of the function. Please remove it.

I'm assuming you have tested this patch with an account that's not syncing all of the engines and verified that the progressbar works as expected, right?
Attachment #569558 - Flags: feedback?(philipp) → feedback+
yes, it was tested with < n engines sync'ed, including 1 engine and 0 engines. It behaves correctly in those cases as well.
Attachment #569714 - Flags: feedback?(philipp)
Comment on attachment 569714 [details] [diff] [review]
part1(v4.1) progress bar on mobile

r+ without the dump statements ;)
Attachment #569714 - Flags: feedback?(philipp) → review+
Attachment #569558 - Attachment is obsolete: true
https://hg.mozilla.org/services/services-central/rev/72827a90dbf6

STR for QA are in comment 12
Whiteboard: [fixed in services]
verified with s-c on Galaxy Tab, progress bar works as expected and closes on automatically on completion.
Whiteboard: [fixed in services] → [verified in services]
https://hg.mozilla.org/mozilla-central/rev/72827a90dbf6
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
Component: Firefox Sync: UI → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.