560937 - move all setup into a single wizard, and clean up interactions

Status: VERIFIED FIXED

(Firefox :: Sync, defect, P1)

Description

[Mike Connor [:mconnor]]

Attachment: wip (very rough)

Comment 1

[Mike Connor [:mconnor]]

Attachment: works okay, needs much cleanup

Comment 2

[Mike Connor [:mconnor]]

Attachment: v0.9

Comment 3

[Ed Lee :Mardak]

Comment on attachment 440833 [details] [diff] [review]
v0.9

>+++ b/source/chrome/content/preferences/fx-prefs.js
>-  resetSync: function() {
>-    this.handleExpanderClick();
>-    // Trigger the move to page 2
>-    this._resettingSync = true;
Are we getting rid of this functionality? The ui still has the reset sync link.

>     let ready = false;
>     switch (this.page) {
>       case "0":
This whole isReady function is unused from fx-prefs and seems to be inlined into fx-setup anyway.

>+++ b/source/chrome/content/preferences/fx-setup.js
> var gWeaveSetup = {
>+  _settingUpNew: true,
This probably doesn't need to be explicitly declared here as it'll get set.

>+    this.wizard.getButton("cancel").label = "Later";
Needs a string

>+  changePassword: function () {
>+    Weave.Utils.openGenericDialog("ChangePassword");
>+  changePassphrase: function () {
>+    Weave.Utils.openGenericDialog("ChangePassphrase");
Are these needed here?

>   onPageShow: function() {
>     switch (this.wizard.currentPage.pageIndex) {
>+      case 0:
>+      case 1:
>+      case 5:

Magic numbers makes this hard to review and probably edit.. maybe we could stick some consts at the top const PAGE_CAPTCHA = 4;

Also, might make switching pages a bit saner, e.g., wizard.pageIndex = PAGE_EXISTING;

>+    this._handleNoScript(false);
>     Weave.Svc.Prefs.reset("firstSync");
>+    Weave.Status.service == Weave.STATUS_OK;
>     Weave.Service.login();
>+    Weave.Service.syncOnIdle(1);
Probably should add a persistLogin call when finishing the wizard.

>+++ b/source/chrome/content/preferences/fx-setup.xul
>+  <wizardpage id="useExisting" 
>+              label="Enter Account Information"
>+  <wizardpage id="mergeOptionsChoice" label="How do you want to start off?">
Got some strings here and elsewhere..

>+++ b/source/chrome/content/sync.js
>-<!ENTITY setup.page1Title.label     "&setup.generalTitle.label; - Page 1 of 4">
During the user study, one comment was that it was nice to know how many more pages there would be.

Comment 4

[Mike Connor [:mconnor]]

(In reply to comment #3)
> (From update of attachment 440833 [details] [diff] [review])
> >+++ b/source/chrome/content/preferences/fx-prefs.js
> >-  resetSync: function() {
> >-    this.handleExpanderClick();
> >-    // Trigger the move to page 2
> >-    this._resettingSync = true;
> Are we getting rid of this functionality? The ui still has the reset sync link.

nope, just refactored.

> >     let ready = false;
> >     switch (this.page) {
> >       case "0":
> This whole isReady function is unused from fx-prefs and seems to be inlined
> into fx-setup anyway.

removed.

> >+++ b/source/chrome/content/preferences/fx-setup.js
> > var gWeaveSetup = {
> >+  _settingUpNew: true,
> This probably doesn't need to be explicitly declared here as it'll get set.

removed.

> >+    this.wizard.getButton("cancel").label = "Later";
> Needs a string

Fixed

> >+  changePassword: function () {
> >+    Weave.Utils.openGenericDialog("ChangePassword");
> >+  changePassphrase: function () {
> >+    Weave.Utils.openGenericDialog("ChangePassphrase");
> Are these needed here?

changepassphrase and resetpassword (added) are, changepassword is not.

> >   onPageShow: function() {
> >     switch (this.wizard.currentPage.pageIndex) {
> >+      case 0:
> >+      case 1:
> >+      case 5:
> 
> Magic numbers makes this hard to review and probably edit.. maybe we could
> stick some consts at the top const PAGE_CAPTCHA = 4;
> 
> Also, might make switching pages a bit saner, e.g., wizard.pageIndex =
> PAGE_EXISTING;

Good point, done.

> >+    this._handleNoScript(false);
> >     Weave.Svc.Prefs.reset("firstSync");
> >+    Weave.Status.service == Weave.STATUS_OK;
> >     Weave.Service.login();
> >+    Weave.Service.syncOnIdle(1);
> Probably should add a persistLogin call when finishing the wizard.

We have one for each setup flow, but we can/should leave this until the end. Fixed.

> >+++ b/source/chrome/content/preferences/fx-setup.xul
> >+  <wizardpage id="useExisting" 
> >+              label="Enter Account Information"
> >+  <wizardpage id="mergeOptionsChoice" label="How do you want to start off?">
> Got some strings here and elsewhere..

fixed, I think.

> >+++ b/source/chrome/content/sync.js
> >-<!ENTITY setup.page1Title.label     "&setup.generalTitle.label; - Page 1 of 4">
> During the user study, one comment was that it was nice to know how many more
> pages there would be.

Not sure this is long enough to rate that, especially for a single comment, we can revisit later.

Comment 5

[Mike Connor [:mconnor]]

Attachment: v0.95

Comments addressed.  Only thing left is to figure out how to handle wrong PP/PW.

Comment 6

[Mike Connor [:mconnor]]

Attachment: v0.97 (add change/reset password links on login failure)

I know this isn't right for PP errors.

Comment 7

[Mike Connor [:mconnor]]

Attachment: v1

Comment 8

[Ed Lee :Mardak]

Comment on attachment 440927 [details] [diff] [review]
v1

r=Mardak w/ pastebin fix for showing failed secret phrase message/link

>+++ b/source/chrome/content/preferences/fx-prefs.xul
>-                  <button id="connectButton" oncommand="gWeavePane.handleConnectCommand()"
>-                          disabled="true"/>
>-                  <image id="connect-throbber"
>-                         src="chrome://global/skin/icons/loading_16.png"
>-                         hidden="true"/>
>+                  <hbox>
>+                    <button id="connectButton" oncommand="gWeavePane.handleConnectCommand()"
>+                            disabled="true"/>
>+                    <image id="connect-throbber"
>+                           src="chrome://global/skin/icons/loading_16.png"
>+                           hidden="true"/>
>+                  </hbox>
Funky stretching magic happens with images inside boxes. I forget what you need to add to the hbox or the image to get it the right size..

Comment 9

[Mike Connor [:mconnor]]

http://hg.mozilla.org/labs/weave/rev/daaab883742a

Comment 10

[Tony Chung [:tchung]]

Verified fix on weave1.3b1