382792 - can not switch Profile

Status: VERIFIED FIXED

(SeaMonkey :: Startup & Profiles, defect)

Description

[baffclan]

Attachment: screen shot

can not switch Profile

Build identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a5pre) Gecko/2007053014 SeaMonkey/2.0a1pre

Reproducible: always

Steps to Reproduce :
1. Tools -> Switch Profile
2. title bar("Page Load...") only profile manager

Comment 2

[Mark Banner (:standard8)]

Yep its a problem, we need to decide what we're doing with this. That option may go away, though its unclear at the moment.

Comment 6

[Philip Kent]

I can confirm this bug on Ubuntu Linux 7.04. The error shown in the window is:
The file jar:file///home/philip/Desktop/seamonkey/chrome/comm.jar!/content/communicator/profile/profileSelection.xul cannot be found.

After further investigation, there is no profile folder in comm.jar.

Comment 7

[Robert Kaiser]

No need to confirm this. We know the problem and know why it's happening, it's just not straightforward to fix.

Comment 8

[Trel]

I don't see it here, but I did have a suggestion in the one I reported.  If it cannot be made to function as it does now, rather than remove the feature, that menu item can be made to restart the browser with a -profilemanager switch.  (Of course it could warn the user first before restarting the browser)

Comment 9

[Philip Kent]

Wouldn't a browser restart be required anyway?

I personally would go with that way as it would probably be more clean

Comment 10

[Trel]

(In reply to comment #9)
> Wouldn't a browser restart be required anyway?
> 
> I personally would go with that way as it would probably be more clean
> 

Well in 1.1.3, it doesn't restart unless you change profiles.  My suggestion would cause a restart before bringing up the manager to begin with.  It wouldn't be ideal, but if no solution is found otherwise, it would be better than removing the ability to call up the profile manager from within the browser.

Comment 11

[[e-mail]]

(In reply to comment #10)
>Well in 1.1.3, it doesn't restart unless you change profiles.
Actually if you must know, in 1.1.3 it doesn't restart at all. Instead it just closes all your old windows and opens new windows using the new profile.

That isn't directly possible with a toolkit backend but I think it should be possible to make toolkit do a real restart with a different profile.

Comment 12

[Kai Engert (:KaiE:)]

There has once been a time when a vendor wanted to embed Gecko, and completely switch to a different account, including all security data. This required us to do a lot of work in mozilla/security/manager/ssl in order to ensure the C library NSS gets shut down and re-initialized correctly, without conflicts caused by different load orders related to XPCOM etc.

As of today, it's supposed to work from the security code's perspective.

I think it would be nice if we could preserve this behavior, if the need returns for something like this in the future.

The "switch profile" is a helpful mechanism to test whether it indeed still works, I'd prefer we keep it in some way.

Comment 13

[[e-mail]]

(In reply to comment #12)
>The "switch profile" is a helpful mechanism to test whether it indeed still
>works, I'd prefer we keep it in some way.
This is not possible with the current implementation of XPCOM since there is no way to force (profile-specific) extension components to unload. While future implementations may support this the best I can do with the current toolkit is to shutdown the existing process and launch a new one for the new profile.

In theory the original design could have been for user-specific components like it was for plugins (although they are now profile-specific too).

Comment 14

[[e-mail]]

Attachment: Provisional patch

This is based on the old XPFE profile manager, but with the guts rewritten to use toolkit interfaces. All eyes welcome!

Comment 15

[jag (Peter Annema)]

Comment on attachment 276129 [details] [diff] [review]
Provisional patch

Wow, there's barely any consistency in the property and entity names.

I know you're just copying this, but would you mind changing them to something along these lines:

renameTitle
renamePrompt
deleteTitle
deletePrompt
deleteFiles
dontDeleteFiles
manageTitle
selectTitle
switchText
nameEmpty
exists
invalidTitle
dirLocked

windowTitle.label
profileManager.label
availableProfiles.label
profileManagerText.label

I'm on the fence on sticking "Profile(s)" in everything, or leaving it out since that should be clear from being in the profile manager.

For the button IDs, please use lowerCamelCase too. Hrm, and the bracing style in the js files is inconsistent... Shall I just file a new bug on cleaning all that up?

Comment 16

[[e-mail]]

(In reply to comment #15)
>nameEmpty
>exists
invalidChar and these two are actually used by createProfileWizard.js
(I'm reusing the code that returns an error message for invalid profile names).

Comment 17

[jag (Peter Annema)]

What do you think about the rest though?

Comment 18

[[e-mail]]

Attachment: Addressed jag's comments

Don't try to interdiff the patches ;-)

Comment 19

[Robert Kaiser]

Comment on attachment 276469 [details] [diff] [review]
Addressed jag's comments

I still feel a bit uncomfortable with using two different windows for the two profile managers (at startup or from the menu) and duplicate all of this.
I guess there's no other way to do this? We can't make startup using this version of the window, right?

Comment 20

[Robert Kaiser]

Comment on attachment 276469 [details] [diff] [review]
Addressed jag's comments

>Index: common/profile/profileSelection.js
>===================================================================

>+  gProfileBundle = document.getElementById("bundle_profileManager");

This does not work, it's bundle_profile actually.

While switching itself works with this, the completely different look compared to the profile manager we have at startup looks strange and maybe even confusing to users. Can we do something about that?
Additionally, the "don't ask at startup" option looks somewhat out of place for me in this dialog, what use does it have there?

Comment 21

[[e-mail]]

(In reply to comment #19)
>(From update of attachment 276469 [details] [diff] [review])
>I still feel a bit uncomfortable with using two different windows for the two
>profile managers (at startup or from the menu) and duplicate all of this.
bsmedberg asked me to write it as a separate window.

>I guess there's no other way to do this? We can't make startup using this
>version of the window, right?
We probably can, but I'd want to look into that separately, please.

(In reply to comment #20)
>(From update of attachment 276469 [details] [diff] [review])
>>+  gProfileBundle = document.getElementById("bundle_profileManager");
>This does not work, it's bundle_profile actually.
Sorry, I was renaming a bunch of stuff and missed this one reference.

>Additionally, the "don't ask at startup" option looks somewhat out of place for
>me in this dialog, what use does it have there?
This dialog was based on the XPFE dialog whaich has that checkbox.

Comment 22

[Robert Kaiser]

Could we make the XUL more similar to the toolkit one, and maybe even use its theme file as a base, with a tiny theme file of our own only slightly extending it as/if necessary?We should at least achieve a consistent look of both dialogs for the user, even now (and minimize the amount of duplicate skin definitions we as well as custom themes need).
Maybe we could even reuse some of the JS, but that may be up to a further cleanup later.

Comment 23

[[e-mail]]

Attachment: Completely replace toolkit profile manager

Assuming that's what we really want to do...

Comment 24

[jag (Peter Annema)]

Comment on attachment 277251 [details] [diff] [review]
Completely replace toolkit profile manager

Minor nit: AcceptDialog() (initial uppercase, like the rest of the file)

Comment 26

[Karsten Düsterloh]

Comment on attachment 277251 [details] [diff] [review]
Completely replace toolkit profile manager

I'm in favour of this patch version.

(From update of attachment 277251 [details] [diff] [review])

Firstly, some visual nits, based on Linux Classic (I'll attach a screenshot):
- The main dialog's header has a grey frame around the white, which it shouldn't have.
- The actual Create Profile Wizard's header is completely grey - which it was before, too, but for consistency it should be white also.
- The layout of the standard buttons at the lower edge is just weird, eg wrt spacing.

Secondly, functional nits:
- Up to now, if you create a new profile named X and Choose Folder Y, the profile will end up with some path like Y/X/Z.slt/stuff. The toolkit way of creating Y/stuff in this case is quite annoying IMO, especially for long time Mozilla users. I'd really like _not_ to see that, I'm at war with that since the early toolkit days...
- We didn't do this before, but we should inform the user that choosing another (old, probably disconnected) profile path will *not* overwrite but reconnect the profile - people occasionally ask how to do that...
- Double clicking a profile name in the main dialog doesn't start the profile, throwing: "Error: onStart is not defined
Source File: chrome://communicator/content/profile/profileSelection.js
Line: 344" instead
- Deleting a profile doesn't check if the user has created another profile in the same place (see reconnect above), so deleting it will kill the other profile, too! Also, we maybe should check for this situation when creating profiles as well...

Thirdly, coding nits:

>Index: common/profile/profileSelection.js
>===================================================================
>+var gPromptService = Components.classes["@mozilla.org/embedcomp/prompt-service;1"].getService(Components.interfaces.nsIPromptService);
...
>+  gProfileService = Components.classes["@mozilla.org/toolkit/profile-service;1"].getService(Components.interfaces.nsIToolkitProfileService);

Break long lines.

>+function AddItem(aProfile, aProfileToSelect)
...
>+  tree.lastChild.appendChild(treeitem);

Is treechildren actually guaranteed to be the last child?

>+  try {
>+    var env = Components.classes["@mozilla.org/process/environment;1"].getService(Components.interfaces.nsIEnvironment);
...
>+    var app = Components.classes["@mozilla.org/toolkit/app-startup;1"].getService(Components.interfaces.nsIAppStartup);

Break long lines.

>+  var newName = { value: profileName };
>+  if (gPromptService.prompt(window, dialogTitle, msg, newName, null, { value: 0 }) && newName.value != 
>+      gProfileBundle.getString("deleteFiles"), null, { value: 0 });

The spaces after { and before } look particularly strange, given that we don't do this for ( etc. (and you don't do it in IsProfileLocked either), so just drop 'em. 

>+// test to see whether the profile is in use
>+function IsProfileLocked(aProfile)
>+{
>+  try {
>+    aProfile.lock({}).unlock();

This is known to be bogus if the profile is deleted (also see functional nits above).

>+function HandleClickEvent(aEvent)
>+{
>+  if (aEvent.button == 0 && aEvent.target.parentNode.view.selection.count != 0 && onStart()) {

Should be OnStart(), surely.

>Index: common/profile/profileSelection.xul
>===================================================================
>+        spacerflex="1"

Mmh? Seems to be dead cruft...

>Index: locales/en-US/chrome/common/profile/profileSelection.dtd
>===================================================================
>+<!ENTITY            manage.label     "Manage Profiles...">
>+<!ENTITY            deleteButton.label      "Delete Profile...">
>+<!ENTITY            renameButton.label      "Rename Profile...">
>+<!ENTITY            createButton.label      "Create Profile...">

Current trunk efforts lean towards using a real ellipsis (&#8230;) here, but me no l10n... ;-)

Comment 27

[Karsten Düsterloh]

Attachment: Attachment 277251 with Classic theme on Linux

Comment 28

[Robert Kaiser]

I dislike the white area as a whole and I think our dialog should look as similar to the toolkit one as possible, as that one is functional and looks better than that own one actually - and it fits the wizards that we are using from there.
IMHO, we should even use their CSS, at least as a base, and get our separate stuff to be as tiny as possible, adding only what we really need.

I disagree with Karsten on the directory creation, I think creating an additional directory inside the chosen one is counter-intuitive and not what most users want. We only create the salted dirs so that a profile paths aren't completely deterministic for the outside world, in case of a custom dir it is already indeterministic due to the custom location, adding salting is unnecessary there.

Things like the warning about reconnecting the profile belong into toolkit's version, not our self-bred version. Our own version should only have the purpose of making the menu option work, everything else should match the toolkit version, IMHO.

Comment 29

[[e-mail]]

(In reply to comment #26)
>(From update of attachment 277251 [details] [diff] [review])
>Firstly, some visual nits, based on Linux Classic (I'll attach a screenshot):
>- The main dialog's header has a grey frame around the white, which it
>shouldn't have.
>- The actual Create Profile Wizard's header is completely grey - which it was
>before, too, but for consistency it should be white also.
>- The layout of the standard buttons at the lower edge is just weird, eg wrt
>spacing.
The Create Profile Wizard is toolkit's wizard, so the colours there are whatever toolkit gives us. Same goes for the standard buttons. I'd have to look to see how the dialog header worked in xpfe.

>Secondly, functional nits:
>- Up to now, if you create a new profile named X and Choose Folder Y, the
>profile will end up with some path like Y/X/Z.slt/stuff. The toolkit way of
>creating Y/stuff in this case is quite annoying IMO, especially for long time
>Mozilla users. I'd really like _not_ to see that, I'm at war with that since
>the early toolkit days...
Again, I'm limited to the toolkit APIs for this.

>- We didn't do this before, but we should inform the user that choosing another
>(old, probably disconnected) profile path will *not* overwrite but reconnect
>the profile - people occasionally ask how to do that...
But the profile manager is not the correct place to do that.

>- Double clicking a profile name in the main dialog doesn't start the profile,
>throwing: "Error: onStart is not defined
>Source File: chrome://communicator/content/profile/profileSelection.js
>Line: 344" instead
Oops. Should be AcceptDialog (and so should acceptDialog).

>- Deleting a profile doesn't check if the user has created another profile in
>the same place (see reconnect above), so deleting it will kill the other
>profile, too! Also, we maybe should check for this situation when creating
>profiles as well...
I think this is too much of an edge case.

> >+function AddItem(aProfile, aProfileToSelect)
> ...
> >+  tree.lastChild.appendChild(treeitem);
> 
> Is treechildren actually guaranteed to be the last child?
It's there in the XUL and I never remove or add anything anywhere else...

>>+// test to see whether the profile is in use
>>+function IsProfileLocked(aProfile)
>>+{
>>+  try {
>>+    aProfile.lock({}).unlock();
>This is known to be bogus if the profile is deleted (also see functional nits
>above).
Bogus in that it will complain that the profile is in use? Should I change the message to "is in use or has been deleted?"

>>+        spacerflex="1"
>Mmh? Seems to be dead cruft...
Yeah, that died with XPFE - toolkit always flexes the spacer if you have an extra2 button.

>Current trunk efforts lean towards using a real ellipsis (…)
Is there a bug# for that?

Comment 30

[Karsten Düsterloh]

(In reply to comment #28)
> IMHO, we should even use their CSS, at least as a base, and get our separate
> stuff to be as tiny as possible, adding only what we really need.

I'm fine either way, it just should look consistently.

> I disagree with Karsten on the directory creation, I think creating an
> additional directory inside the chosen one is counter-intuitive

The dialog is about creating *profiles*, not *profile data*. I expect to be able to put several profiles into the same directory. I surely do not expect that these will put their data just there!

> and not what most users want.

I strongly doubt that. In any case, Joe User usually doesn't care for paths and such anyway - and as Neil pointed out, we're set with Toolkit's way here. 
(Did I already say I hate it?)

> We only create the salted dirs so that a profile paths aren't
> completely deterministic for the outside world, in case of a custom dir it is
> already indeterministic due to the custom location, adding salting is
> unnecessary there.

Salting isn't my point. 
I don't care if it's  Y/X/Z.slt/stuff or Y/X/stuff, as long as it's not  Y/stuff.

> Things like the warning about reconnecting the profile belong into toolkit's
> version, not our self-bred version.

Okay, agreed.



(In reply to comment #29)
[Toolkit profile location.]
> Again, I'm limited to the toolkit APIs for this.

We then really should at least tell the user that his *data* will be stored in this very directory, not the profile as Mozilla and SM used to do!

> >- We didn't do this before, but we should inform the user that choosing another
> >(old, probably disconnected) profile path will *not* overwrite but reconnect
> >the profile - people occasionally ask how to do that...
> But the profile manager is not the correct place to do that.

Huh?! This is the place where you manage your profiles, so where else would this informational text(!) be of interest?

> >- Deleting a profile doesn't check if the user has created another profile in
> >the same place (see reconnect above), so deleting it will kill the other
> >profile, too! Also, we maybe should check for this situation when creating
> >profiles as well...
> I think this is too much of an edge case.

Okay, and probably better be fixed in toolkit anyway.

> >>+// test to see whether the profile is in use
> >>+function IsProfileLocked(aProfile)
> >>+{
> >>+  try {
> >>+    aProfile.lock({}).unlock();
> >This is known to be bogus if the profile is deleted (also see functional nits
> >above).
> Bogus in that it will complain that the profile is in use?

Yes.

> Should I change the message to "is in use or has been deleted?"

The actual problem is just that it can't be found - it could have been moved or the path (eg. a symlink) may have changed. Maybe use "removed" instead of "deleted"? Or do we have a chance to distinguish the two cases?

> >Current trunk efforts lean towards using a real ellipsis (…)
> Is there a bug# for that?

Bug 390282 (Toolkit), bug 373623 (FF/TB), bug 389303 (Calendar).

Comment 31

[[e-mail]]

Comment on attachment 277251 [details] [diff] [review]
Completely replace toolkit profile manager

>+<!ENTITY            profileManager.title "&brandShortName; Profile Manager">
>+
>+<!ENTITY            manage.label     "Manage Profiles...">
>+<!ENTITY            manage.accesskey "M">
>+<!ENTITY            select.label     "Use Profile">
>+<!ENTITY            start.label      "Start &vendorShortName;">
KaiRo, I just copied the .dtd from the old profile manager, but as per bug 369388, shouldn't this be &brandShortName; too?

Comment 32

[Robert Kaiser]

Sure we need the brand name and not the vendor name here, that's a bug in the old files.

Could we pull in profileSelection.dtd and profileSelection.properties from toolkit so that we avoid localizers localizing the same string once again here? We can add any additional strings we need in our own files, but I'd like to take the base strings from there.

And I repeat, I'd like us to also pull in the CSS from toolkit - http://mxr.mozilla.org/mozilla/source/toolkit/profile/skin/profileSelection.css has only very few rules, and we only need to put everything additional in our own CSS file.

Comment 33

[[e-mail]]

(In reply to comment #32)
>Sure we need the brand name and not the vendor name here, that's a bug in the
>old files.
Fixed.

>Could we pull in profileSelection.dtd and profileSelection.properties from
>toolkit so that we avoid localizers localizing the same string once again here?
>We can add any additional strings we need in our own files, but I'd like to
>take the base strings from there.
Pulling profileSelection.dtd would be easy but pulling in .properties is harder as you then have to have a second stringbundle and remember which one to use.

>And I repeat, I'd like us to also pull in the CSS from toolkit -
>http://mxr.mozilla.org/mozilla/source/toolkit/profile/skin/profileSelection.css
>has only very few rules
None of which are any use...

Comment 34

[Robert Kaiser]

(In reply to comment #33)
> Pulling profileSelection.dtd would be easy but pulling in .properties is harder
> as you then have to have a second stringbundle and remember which one to use.

Sure, but I'd think that's worth it.

> >And I repeat, I'd like us to also pull in the CSS from toolkit -
> >http://mxr.mozilla.org/mozilla/source/toolkit/profile/skin/profileSelection.css
> >has only very few rules
> None of which are any use...

Can't be hard to make them be of use, i.e. change our dialog to match the toolkit one more. I already hate enough that we apparently need our own.

Comment 35

[[e-mail]]

(In reply to comment #34)
> Can't be hard to make them be of use

The first one can't be made of use without backing out a previous fix.

The second one would need us to change <vbox> to <box orient="vertical">.

The third one would need us to remove a <separator>.

Comment 36

[[e-mail]]

Attachment: Updated patch

Comment 37

[Robert Kaiser]

Comment on attachment 282238 [details] [diff] [review]
Updated patch

>Index: common/profile/profileSelection.xul
>===================================================================
>+<dialog id="profileWindow"
>+        xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>+        title="&windowTitle.label;"

Could you use &windowtitle.label; instead, as that is already defined in toolkit? We can remove our string from the DTD then.

>+  <dialogheader class="header-large" id="header" title="&profileManager.title;" description="&windowTitle.label;"/>

And another &windowtitle.label; here.

Comment 38

[[e-mail]]

(In reply to comment #37)
>>+  <dialogheader class="header-large" id="header" title="&profileManager.title;" description="&windowTitle.label;"/>
>And another &windowtitle.label; here.
It doesn't make sense to change this one. (And if we changed the other one, it would be to "Select User Profile - &brandShortName;" anyway.)

Comment 39

[Karsten Düsterloh]

Comment on attachment 282238 [details] [diff] [review]
Updated patch

>Index: common/profile/profileSelection.js
>===================================================================
>+// Delete the profile, with the delete flag set as per instruction above.
>+function DeleteProfile(deleteFiles)

Please adhere to the aParameter meme.

>Index: locales/en-US/chrome/common/profile/profileSelection.properties
>===================================================================
>+dirLocked=%S cannot use the profile "%S" because it is in use.\n\nPlease choose another profile or create a new one.

This is still a lie if the profile has been deleted or is otherwise damaged.
We shouldn't lie. ;-)


But what is more serious: "don't ask at startup" is broken. SM is meant to start with your last used profile, but it starts with the default (first?) one regardless...

Comment 40

[[e-mail]]

Attachment: Addressed review comments

Comment 41

[Karsten Düsterloh]

Comment on attachment 282983 [details] [diff] [review]
Addressed review comments

r=me with the updated dirLocked text we talked about on IRC.

Comment 42

[jag (Peter Annema)]

Comment on attachment 282983 [details] [diff] [review]
Addressed review comments

sr=jag

You might want to support VK_BACKSPACE on Mac to delete profiles.

Comment 43

[jag (Peter Annema)]

VK_BACK, of course.

Comment 44

[[e-mail]]

Fix checked in.

(In reply to comment #42)
>You might want to support VK_BACKSPACE on Mac to delete profiles.

(In reply to comment #43)
>VK_BACK, of course.

VK_BACK_SPACE, apparently.

Comment 45

[Serge Gautherie (:sgautherie)]

V.Fixed between
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a9pre) Gecko/2007100303 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)
and
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a9pre) Gecko/2007100402 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

Comment 46

[baffclan]

profile folder is change?
Submitted bug 398533.