220891 - Create Profile Wizard overhaul

Status: VERIFIED FIXED

(SeaMonkey :: Startup & Profiles, defect)

Description

[Stefan Borggraefe]

The Create Profile Wizard currently doesn't use the <wizard> XUL-element, but
some other ancient wizard-implementation.

I ported it over to user <wizard>. This has several benefits:
- the other wizard implementation can be removed from the tree as it is used
nowhere else
- it looks consistent to the Account Wizard
- code reuse

The attached patch also fixes bug 29676, bug 63587, bug 106962, bug 130809 and
bug 203147.

I also rearranged the buttons on the second wizard page a bit and changed the
button text "Use Default" to "Use Default Folder" so it is now clear to the user
what the button actually covers (e.g. is does not set default region settings).

I changed the error handling so the user gets feedback to his input immediatly
and the Finish-button is disabled when there is something wrong. If this patch
ever gets checked in I could also change the Account Wizard input handling to
this behaviour, so the both Wizards in Mozilla are consistent.

The only thing I don't like about this patch is the fact that the initial window
size is specified by width="640" height="450". I would rather use style="width:
40em; height: 30em;", but <wizard> doesn't seem to support this. Because of this
I choosed a rather big initial window size so it is big enough when you use
larger fonts and made it resizable. If you know a way to specify the initial
window size of a wizard using em, please let me know!

Comment 1

[Stefan Borggraefe]

Attachment: Patch as described in comment #0.

Comment 2

[Stefan Borggraefe]

Attachment: Screenshot of the first wizard page with the patch applied

Comment 3

[Stefan Borggraefe]

Attachment: Screenshot of the second wizard page with the patch applied

Notice: when you enter the second wizard page the Profile name is now selected
and has the focus. I believe most of the times the user wants to change the
name from "Default User" to something other. Now you can save some mouse clicks
to do this.

Comment 4

[Stefan Borggraefe]

Attachment: Screenshot of the first wizard page with input error and the patch applied

When the user inputs something wrong he gets notified immidiatly by the red
text at the bottom of the wizard. The Finish button is disabled, too. When the
input is ok again, the black default text reappears. The input is checked after
each keypress in the Profile Name textbox.

Comment 5

[Christian Hamacher]

This is great! Thanks for fixing this - it greatly improves consistency in
appearance, especially if you're not using the Classic theme.

One small question: in the second wizard page, above the text input box it says
'Enter New Profile name', with the first three words capitalized. Is this
intentional (in the sense that 'New Profile' is a proper name that should be
capitalized), or should it rather be 'Enter new profile name'?

Comment 6

[Stefan Borggraefe]

Christian: I haven't touched this string. But I agree with you, that the "New"
should rather be "new". I would change it before this gets checked in.

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

Why do you specify a width/height at all? Can't you let the wizard size itself
automatically?

Comment 8

[Stefan Borggraefe]

biesi: When I don't specify the width and height, the window is opened much to
small and I get vertical and horizontal scrollbars for the wizard's content.

Comment 9

[[e-mail]]

Comment on attachment 132446 [details] [diff] [review]
Patch as described in comment #0.

This is just from reading it, I haven't tried it as such.

>-<?xul-overlay href="chrome://global/content/wizardOverlay.xul"?>
> <?xul-overlay href="chrome://global/content/dialogOverlay.xul"?>
Surely you can get rid of both?

>-<window title="&newprofile.title;" orient="vertical"
>+<wizard id="createProfileWizard"
>+        title="&newprofile.title;" orient="vertical"
Do you need orient?
>+        width="640" height="450"
I notice that the new account wizard uses entities for this.
>         class="color-dialog"
Do you still need this?
>         xmlns:html="http://www.w3.org/1999/xhtml"
>         xmlns:rdf="http://www.w3.org/1999/02/22-rdf-syntax-ns#"
>         xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>-        onclose="onCancel();"
>-        onload="Startup('newProfile1_1','content');">
>+        onwizardcancel="onCancel();"
>+        onwizardfinish="onFinish();"
>+        persist="screenX screenY width height">
I don't think you want to do this.

>+  <wizardpage id="createProfile"
>+              onpageshow="init();"
>+              onpagehide="enableNextButton();">
This seems like an odd place to do this.

>+    <textbox id="ProfileName" value="&pnl2.defaultPName.label;"
>+             oninput="updateProfileName();"
>+             onkeyup="checkCurrentInput();"/>
Why two event handlers?

>+      <hbox align="start" class="indent" flex="1" overflow="auto">
No, style="overflow: auto;"

>+  gWizard = document.getElementById("createProfileWizard");
Actually document.documentElement works fine (you could even elimindate
gWizard).

>+  profileNameTextbox.setSelectionRange(0, profileNameTextbox.textLength);
.select() should work.

>+      // later change to
Where do these odd comments come from?

>+    clearFolderDisplay();
>+    var value = document.createTextNode( sfile.path );
>+    folderDisplayElement.appendChild( value );
It's probably more efficient to keep a single text node as the child of the
folder display element and change its data as necessary.

>+  if (trim(currentInput) == "") {
That's a waste of a trim, because you're only checking for non-whitespace, if
you really want to use a trimmed value then save the result, otherwise use
if (!/\S/.test(currentInput)) {

>+    errorMessage.setAttribute("class", "error");
Can also use .className here.

>+  else if (hasInvalidChars(currentInput) != -1) {
>+    var aString = gProfileManagerBundle.getString("invalidCharA");
>+    var bString = gProfileManagerBundle.getString("invalidCharB");
>+    errorMessage.value = aString + gInvalidChars[hasInvalidChars(currentInput)] + bString;
You're calling hasInvalidChars twice here... but again regexps make things much
simpler, just use
if (/([/\\*:?<>|\"])/.test(currentInput)) {
If the test succeeds, RegExp.$1 contains the invalid character.
Also, the way the error string is built up is odd, you really should fix it to
use getFormattedString instead.

>+  window.openDialog('chrome://communicator/content/profile/createProfileWizard.xul', 'CPW', 'centerscreen,chrome,modal,titlebar,resizable');
Again, I don't think the wizard should be resizable.

Comment 10

[timeless]

Comment on attachment 132446 [details] [diff] [review]
Patch as described in comment #0.

If you have time after you finish this round, could you work on a panel 
replacement for the locale chooser dialog?

if (...) {
 ...
 return
} /* this is called an 'else-after-return' and you shouldn't do it.
   * to correct this, you just promote the else block by removing the
   * else (and its braces if it has them and they are serving no other
   * purpose).
   */
else {
 ...
}

There are two other variants which concern me,
if (...) {
 ...
} else
 return
/* return-after-else. you can usually change it to:
if (!...)
 return
...
*/
and
function (...) {
 if (...) {
  ...
 } else ... {
  ...
  return ...;
 }
 /* no return at end of function */
}
usually fixing else-after-return and return-after-else avoids this.

fileSpec ... nsIFile. FileSpec is an old interface which was replaced by 
nsIFile, please rename variables named 'fileSpec' to something meaningful 
like profileRoot.

+  <wizardpage id="explanation">
+    <description>&pnl1.p1.text;</description>
+    <description>&pnl1.p2.text;</description>
+    <description>&pnl1.p3.text;</description>
+    <spacer flex="1"/>
+    <description>&pnl1.p4.text;</description>
+  </wizardpage>

please give your entities descriptive names, they're an interface contract 
for localizers.

+ foo
\ No newline at end of file
^ please add a newline to the end of the file

Comment 11

[Stefan Borggraefe]

I'll work on the issues spotted by Neil and timeless. I agree with most of your
comments, but I'm not totally conviced concerning the resizability of the window.

Neil: Why shouldn't the window be resizable and store its position? As I said in
comment #0 I know of no way to specify a size that is right for all users with
different font sizes. So at least the user should be able to resize the window,
when it is to big/small for him. Also the persistence of the position and size
makes sense in my opinion: When you create several Profiles you only have to
resize the window to your preferred size once.


timeless wrote:
> If you have time after you finish this round, could you work on a panel 
> replacement for the locale chooser dialog?

File a bug! ;-) What exactly do you mean with panel replacement? Two dropdown
boxes on the second wizard page for Language and Content? A third wizard page?
Do you have a concrete idea?

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

I'm not sure, but isn't there a window.sizeToContent() function which should do
what you want?

Comment 13

[Stefan Borggraefe]

Found some more bugs my patch fixes.

Comment 14

[Stefan Borggraefe]

biesi: Using sizeToContent() results in the same window size as leaving out the
width and height attributes from the <wizard>-element.

Comment 15

[Stefan Borggraefe]

Attachment: Patch V2

Changed everything Neil spotted except the resizablility issue and did some
clean-ups as timeless suggested. Also I changed the following:

- Rename Profile now uses the same function to check the validity of the
profile name as Create Profile.
- The Create Profile wizard used to fail silently when the chosen folder wasn't
writable. Now an alert is shown and the wizard remains open, so the user may
choose another folder.
- Removed some unused strings. (checked via lxr)
- Removed obsolete files from the Delete Profile window to also fix bug 185087
along the way.

I tested this patch with Windows XP and Linux.

Comment 16

[[e-mail]]

Comment on attachment 132785 [details] [diff] [review]
Patch V2

>+<wizard id="createProfileWizard"
>+        title="&newprofile.title;"
>         xmlns:html="http://www.w3.org/1999/xhtml"
>         xmlns:rdf="http://www.w3.org/1999/02/22-rdf-syntax-ns#"
>         xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>-        onclose="onCancel();"
>-        onload="Startup('newProfile1_1','content');">
>+        onwizardcancel="return onCancel();"
>+        onwizardfinish="return onFinish();"
>+        width="&window.width;" height="&window.height;"
>+        persist="screenX screenY width height">
I'm not convinced that resizing and persisting is right.

>+  <wizardpage id="explanation" onpagehide="enableNextButton();">
onpageshow, perchance? As it stands if I click Back() I get stuck.

>+    <description>profileCreationIntro.text</description> 
Forgot your &;s? Also, please trim those trailing spaces.

>+    <vbox id="dirbox" flex="1">
I'm not sure that you need this...

>+      <hbox align="start" class="indent" flex="1">
>+        <description id="ProfileDir"/>
>+      </hbox>
>+      <spacer flex="1"/>
The description doesn't wrap properly (change <hbox align="start"> to <vbox>?),
and needs to be overflowable. Since you have flex="1" on the hbox you probably
don't need a spacer.

>+function setDisplayToDefaultFolder()
> {
>-  if( frame_id == "" ) {
>-    dump("Please supply a content_frame ID!");
>+  setDisplayToFolder(gProfile.defaultProfileParentDir);
>+  document.getElementById("useDefault").disabled = true;
>+}
You might want to move the focus...

>+  gLastPathSeparatorPos = profileRoot.path.length;
>+  profileRoot.append(profileName.value);
>+  gProfileRoot = profileRoot;
Sorry, this doesn't work when profileName.value is blank.

>+  if (aRootFolder.path != gProfile.defaultProfileParentDir.path)
>+    document.getElementById("useDefault").disabled = false;
And what if you do choose the default folder?

>+  finishText.value = errorMessage;
This way doesn't wrap (I bet error messages are twice as long in German ;-) try
using .SetTextContent(errorMessage) instead.

>+  document.documentElement.canAdvance = canAdvance;
>+  finishButton.disabled = !canAdvance;
It might be worthwhile creating a third page, i.e. "Welcome", "Settings",
"Completing" or something.

>+    if (profileLocation != null && profileLocation.exists())
It's a bit late to check for null here...

>+<!ENTITY window.width                 "640">
>+<!ENTITY window.height                "460">
These are a bit large... try 500x380 (same as new mail account).

Comment 17

[Stefan Borggraefe]

Attachment: Patch V2.1

Addresses all of Neil's comments. Furthermore it adds access keys to the Create
Profile Wizard.

Comment 18

[[e-mail]]

Comment on attachment 133006 [details] [diff] [review]
Patch V2.1

Note: some of my comments only quote one part of the patch, although they may
need you to alter another part too.

>+        style="&window.size;">
Hmmm... any particular reason why you didn't use the default size?

>+
>+
One blank line is enough.

>+    <hbox align="end">
Didn't spot this before... what's it for?

>+  if (gFirstTime) {
Can't you use an onload handler for this?

>+    // Display the default profile folder.
>+    gFolderText = document.getElementById("ProfileDir");
>+    gFolderText.appendChild(document.createTextNode(gProfileRoot.path));
You can put in a dummy value e.g. * in the XUL instead.

>+    gProfileRoot = gDefaultProfileLocation;
>+    
>+    setDisplayToFolder(gProfileRoot);
Can't you use setDisplayToDefaultFolder()?

>+    gPathSeparator = gProfileRoot.path.charAt(gLastPathSeparatorPos);
Presumably gLastPathSeparatorPos is just gDefaultProfileLocation.path.length?

>+  window.openDialog("chrome://communicator/content/profile/selectLang.xul",
>+                    "", "centerscreen,modal,resizable,titlebar",
>+                    selectedLanguage, selectedRegion);
Not resizable, please.

>+  // Disable the "Default Folder..." button when te default profile folder
Typo "te" ;-)

>+  // This is always false on Windows, because the %APPDATA% part of the
>+  // default profile folder returned by gProfile.defaultProfileParentDir
>+  // is in capital letters while fp.file returns mixed-case.
Don't say this, so we can file a bug on gProfile to return it mixed-case.

>+  document.getElementById("useDefault").disabled = 
>+    (aRootFolder.path == gProfile.defaultProfileParentDir.path);
Use .equals

>+  var profDir  = document.getElementById("ProfileDir");
>+  var profDirRootFolder = profDir.getAttribute("rootFolder");
gProfileRoot.path, surely?

>+  var currentInput = document.getElementById("ProfileName").value;
>+  var errorMessage = checkProfileName(currentInput);
>+  var errorMessageClass = "error";
>+  var finishButton = document.documentElement.getButton("finish");
>+  var finishText = document.getElementById("finishtext");
>+  var canAdvance = false;
>+
>+  updateProfileDisplay();
>+
>+  if (!errorMessage) {
>+    errorMessageClass = "";
>+    errorMessage = gProfileManagerBundle.getString("profileFinishText");
>+    canAdvance = true;
>   }
I'd like to see this in a different order;
var errorMessage should be where updateProfileDisplay is,
updateProfileDisplay() should be here.

>+  finishButton.disabled = !canAdvance;
Didn't you like my 3rd page idea? (The MailNews New Account Wizard has a
summary page).

>+    var profileLocation = Components.classes["@mozilla.org/file/local;1"].createInstance();
>+
>+    if (profileLocation)
>+        profileLocation = profileLocation.QueryInterface(Components.interfaces.nsILocalFile);
I know you copied this but it should say
createInstance(Components.interfaces.nsILocalFile);

>-  window.openDialog('chrome://communicator/content/profile/createProfileWizard.xul', 'CPW', 'centerscreen,chrome,modal=yes,titlebar=yes');
>+  var createProfileWizard = window.openDialog('chrome://communicator/content/profile/createProfileWizard.xul', 'CPW', 'centerscreen,chrome,modal,titlebar');
Why the variable?

Comment 19

[Stefan Borggraefe]

>>+        style="&window.size;">
> Hmmm... any particular reason why you didn't use the default size?

Yes, I think the wizard should be a bit wider than the default size so the user
doesn't see a scrollbar when using a longer path for the profile. After testing
this a bit more, I think a width of 45em would be even better.

>>+    gPathSeparator = gProfileRoot.path.charAt(gLastPathSeparatorPos);
> Presumably gLastPathSeparatorPos is just gDefaultProfileLocation.path.length?

At this point they are equal. But the global varible gLastPathSeparator is
needed and its value is changed when another folder is selected. Since the
variable already has a correct value at this point, I think it is more legible
to use it here instead of gDefaultProfileLocation.path.length.

>>+  // This is always false on Windows, because the %APPDATA% part of the
>>+  // default profile folder returned by gProfile.defaultProfileParentDir
>>+  // is in capital letters while fp.file returns mixed-case.
> Don't say this, so we can file a bug on gProfile to return it mixed-case.

I filed bug 221872 and added the bug number to this comment.

>>+  finishButton.disabled = !canAdvance;
> Didn't you like my 3rd page idea? (The MailNews New Account Wizard has a
> summary page).

In my opinion a summarizing page is not needed currently since there is only one
wizard page where data is collected. In the New Account Wizard the user enters
data in many wizard pages, so it makes sense to show him a summarizing page at
the end. I like the idea of presenting the user such a page when finishing a
wizard as soon as there are two or more pages to enter data.

I'm currently testing a patch which addresses all the other review comments.

Comment 20

[Stefan Borggraefe]

Attachment: Patch V2.2

Addressed Neil's review comments and did some more code clean-up so the code is
easier to understand. I actually got rid of gLastPathSeparatorPos. ;-)

I think there doesn't exists a situation where window.opener is undefined
because the Create Profile Wizard is modal, so I deleted the if-conditions
testing this. I hope this is correct.

Comment 21

[[e-mail]]

Comment on attachment 133324 [details] [diff] [review]
Patch V2.2

Unfortunately you were wrong about the opener, because -CreateProfile can open
the create profile wizard.

>+
>+
Oops ;-)

>+  setDisplayToDefaultFolder();
>+  gPathSeparator = getPathSeparator();
Hmm... this is far too complicated for what it does.
Would it be too slow to save the current profile root file XPCOM object, and
clone it and append the profile name every time we update the profile display?
(Then if it doesn't work for C:\ we can blame the file XPCOM object :-)

>+  if (gProfileRoot != gPathSeparator)
>+    gProfileDisplay.data = gProfileRoot + gPathSeparator + currentProfileName;
>+  else
>+    // Don't add second path sepatator when gProfileRoot consists only of a profile separator, e.g. "/".
>+    gProfileDisplay.data = gProfileRoot + currentProfileName;
Not sure what's going on here.

>+  // Add new profile to the list in the Profile Manager.
>+  window.opener.CreateProfile(profileName, gProfileRoot.path);
My fault I think, gProfileRoot is currently a string, not a file :-/

Comment 22

[[e-mail]]

>function getPathSeparator() {
Also, { on its own line.

Comment 23

[Stefan Borggraefe]

Attachment: Patch V2.3

Adressing Neil's review comments. Otherwise unchanged. Using a nsIFile object
instead of a string for the profile display simplifies the code even more. :-)

Comment 24

[[e-mail]]

Comment on attachment 133366 [details] [diff] [review]
Patch V2.3

>+    var profileLocation = Components.classes["@mozilla.org/file/local;1"].createInstance(Components.interfaces.nsILocalFile);
>+
>+    profileLocation.initWithPath(profileRoot);
I think you can use profileLocation = profileRoot.clone() here.

Comment 25

[[e-mail]]

Comment on attachment 133366 [details] [diff] [review]
Patch V2.3

>+var gProfile = Components.classes["@mozilla.org/profile/manager;1"].getService().QueryInterface(Components.interfaces.nsIProfileInternal);;
.getService(Components.interfaces.nsIProfileInternal); (only one ; too ;-)

>+  profileName.select();
>+  profileName.focus();
Hmm... this isn't supposed to be necessary... never mind...

>+  // Check whether a profile with the same name already exists.
>+  if (gProfile.profileExists(profileNameToCheck))
>+    return gProfileManagerBundle.getString("profileExists");
> 
>+  // Check whether all characters in the profile name are allowed.
>+  if (/([\\*:?<>|\/\"])/.test(profileNameToCheck))
>+    return gProfileManagerBundle.getFormattedString("invalidChar", [RegExp.$1]);
Probably swap these two checks around?

>+    var profileLocation = Components.classes["@mozilla.org/file/local;1"].createInstance(Components.interfaces.nsILocalFile);
>+
>+    profileLocation.initWithPath(profileRoot);
Isn't this just profileRoot.clone(); ?

>+  window.openDialog('chrome://communicator/content/profile/createProfileWizard.xul',
>+                    'CPW', 'centerscreen,chrome,modal,titlebar');
Might as well remove the window name, it can cause issues.

>+<!ENTITY window.size                  "width: 45em; height: 30em;">
This turned out to be very slightly too short on Windows Classic - the overflow
on the vbox wouldn't work.

Comment 26

[Stefan Borggraefe]

Attachment: Patch V2.4

Addressing Neil's latest review comments, except this one:

>>+  profileName.select();
>>+  profileName.focus();
>Hmm... this isn't supposed to be necessary... never mind...

When I don't call profileName.focus() the cursor and the selection background
aren't drawn, although the profile name is selected (when I press Del it is
erased).

Comment 27

[[e-mail]]

Comment on attachment 133478 [details] [diff] [review]
Patch V2.4

>+var gProfile = Components.classes["@mozilla.org/profile/manager;1"].getService().QueryInterface(Components.interfaces.nsIProfileInternal);
getService(Components.interfaces.nsIProfileInternal)

>+<!ENTITY window.size                  "width: 47em; height: 30em;">
The window is too short, not too narrow!

Comment 28

[[e-mail]]

Any chance we can remove wizardOverlay.css too?

Comment 29

[Stefan Borggraefe]

Attachment: Patch V2.5

Hopefully final patch. ;-)

Comment 30

[Stefan Borggraefe]

Attachment: Patch V2.6

Neil found another file that could be removed: newProfile1_2.css

This patch removes the code that refers to this file and is otherwise unchanged
compared to Patch V2.5.

Comment 31

[[e-mail]]

Comment on attachment 133882 [details] [diff] [review]
Patch V2.6

OK, I think this is the one!

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 133882 [details] [diff] [review]
Patch V2.6

sr mainly because I trust neil and timeless.

Comment 33

[Stefan Borggraefe]

Checked in by timeless.

Marking FIXED.

Comment 34

[Grace Bush]

verified on build 2003111908