Issue warning to restart (due to Bug 2654) when Local Directory setting is changed

RESOLVED FIXED in Thunderbird 14.0

Status

MailNews Core
Account Manager
--
enhancement
RESOLVED FIXED
14 years ago
3 years ago

People

(Reporter: World, Assigned: aceman)

Tracking

({uiwanted, ux-error-prevention})

Trunk
Thunderbird 14.0
uiwanted, ux-error-prevention
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [gs], URL)

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

14 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Win 9x 4.90; en-US; rv:1.6a) Gecko/20031028
Build Identifier: Mozilla/5.0 (Windows; U; Win 9x 4.90; en-US; rv:1.6a) Gecko/20031028

Not only many bugs are already closed as DUPE of longlived Bug 2654, but also at
least following bugs are currently opened, which seem to be DUPEs of Bug 2654.
  143485 : Copies & Folders not saved
  189212 : Multiple Mail accounts: Won't save "Copies & Folders"
  221386 : Mozilla doesnt save the " Copies and Folders " page in the " Mail and
     
           Newsgroups Account Settings "
  224369 : Copies & Folders settings not saved
In addtion, many duplicated bugs are expected to be opened periodicaly because
many many questions due to Bug 2654 are found in forums of MozillaZine.

If Bug 2654 is not resolved in near future, warning message should be displayed
when Copies & Folder setting is changed in order to force user to restart of
Mozilla.

Dialog example:
 You changed Copies & Folder setting.
 Sorry but Mozilla currently need to be restarted due to Bug 2654.
 Reply OK to restart Mozilla now, or CANCEL to discard your change.



Reproducible: Always

Steps to Reproduce:

Comment 1

13 years ago
*** Bug 229133 has been marked as a duplicate of this bug. ***
(Reporter)

Updated

13 years ago
Summary: Issue warning to restart due to Bug 2654 when Copies & Folders is changed → Issue warning to restart(due to Bug 2654) when Local Directry setting is changed

Updated

13 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Issue warning to restart(due to Bug 2654) when Local Directry setting is changed → Issue warning to restart (due to Bug 2654) when Local Directory setting is changed
Product: Browser → Seamonkey

Updated

12 years ago
Assignee: sspitzer → mail
(Reporter)

Updated

11 years ago
Component: MailNews: Account Manager → MailNews: Backend
Product: Mozilla Application Suite → Core
(Reporter)

Comment 2

11 years ago
Changed to "Core/MailNews: Backend", because Bug 2654 is "Core/MailNews: Backend" and because repeatedly opened DUP bugs of Bug 2654 were for Thunderbird.
Please note that Thunderbird case is worse than Mozilla/SeaMonkey case since still no description about Bug 2654 in Release Notes of Thunderbird, even though all Release Notes of Mozilla/SeaMonkey has pointer to Bug 2654.

(Correction of comment #0. "Copies & Folder" should be "Local Directory".)
Dialog example:
 You changed "Local Directory" setting.
 Sorry but Mozilla currently need to be restarted due to Bug 2654.
 Reply OK to restart Mozilla now, or CANCEL to discard your change.
Assignee: mail → nobody
QA Contact: nbaca → backend
Product: Core → MailNews Core

Updated

7 years ago
Whiteboard: [gs]
(Reporter)

Comment 3

7 years ago
Setting dependency to Bug 2654 for ease of tracking and search, as this bug will be automatically disapperas if Bug 2654 will be fixed, although such Depends on/Blocks: chain(it should be for dependency of patches) is not valid at B.M.O.
(Assignee)

Comment 4

5 years ago
Bwinton, what do you think about this?
Keywords: uiwanted, ux-error-prevention
OS: Other → All
Hardware: x86 → All
(Assignee)

Updated

5 years ago
Component: Backend → Account Manager
Depends on: 2654
QA Contact: backend → account-manager
My gut feeling is that most people don't change this setting, so it's not the highest-priority thing I know of, but if someone wanted to fix it, I wouldn't stop them…
(Assignee)

Comment 6

5 years ago
Exactly because of that there could be a warning, because once somebody changes it, not many people would be able to help him what is going on. See the many dupes of bug 2654.
If this field is done in javascript as the other pref panels, I could try to popup a dialog. Is there any other place we show a dialog in the Account manager (so that I could copy it)?
Assignee: nobody → acelists
(Assignee)

Comment 7

5 years ago
Yes, there is a warning dialog when I try to delete Server name in Account settings.
(Assignee)

Comment 8

5 years ago
Heh, and the warning text there is misleading :) It says an account with that servername and username already exists. Another bug.
(Assignee)

Comment 9

5 years ago
Created attachment 589320 [details] [diff] [review]
patch

So here is the patch. It does this:
- adds a warning when the Local Directory is changed. The warning is shown whether user click OK directly, or changes to another account (some other warnings are not shown in that case, I don't know why).
- distinguishes between and empty server/user name and a duplicate server+user name
- adds a "&brandshortname;" title consistently to all warning dialogs in checkUserServerChanges function. Sometimes the title was set to null.
Attachment #589320 - Flags: ui-review?(bwinton)
Attachment #589320 - Flags: feedback?(iann_bugzilla)
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
Comment on attachment 589320 [details] [diff] [review]
patch

Review of attachment 589320 [details] [diff] [review]:
-----------------------------------------------------------------

Uh, I guess that was more of a Review than a UI-Review, but as it happens the things I'm asking for are fairly small, so ui-r=me with them fixed.  ;)

Thanks,
Blake.

::: mail/locales/en-US/chrome/messenger/prefs.properties
@@ +4,5 @@
>  accountExists=A mail or newsgroup account with the same user name and server name already exists. Click Back and enter a different server name, or click Cancel.
>  modifiedAccountExists=An account with that user name and server name already exists. Please enter a different user name and/or server name.
>  userNameChanged=Your User Name has been updated. You may also need to update your Email Address and/or User Name associated with this account.
>  serverNameChanged=The server name setting has changed. Please verify that any folders used by filters exist on the new server.
> +# LOCALIZATION NOTE (localDirectoryChanged): %1$S is program name (&brandshortname;)

I would prefer this localization note to use "&brandShortName;" instead of "&brandshortname;".

@@ +5,5 @@
>  modifiedAccountExists=An account with that user name and server name already exists. Please enter a different user name and/or server name.
>  userNameChanged=Your User Name has been updated. You may also need to update your Email Address and/or User Name associated with this account.
>  serverNameChanged=The server name setting has changed. Please verify that any folders used by filters exist on the new server.
> +# LOCALIZATION NOTE (localDirectoryChanged): %1$S is program name (&brandshortname;)
> +localDirectoryChanged=The path to Local Directory has changed. You must restart %1$S for this change to take effect and correctly receive messages.

I think this should be either "The path to the Local Directory", or "The Local Directory path".  And I think we want to remove "and correctly receive messages".

@@ -6,2 +6,5 @@
> >  userNameChanged=Your User Name has been updated. You may also need to update your Email Address and/or User Name associated with this account.
> >  serverNameChanged=The server name setting has changed. Please verify that any folders used by filters exist on the new server.
> > +# LOCALIZATION NOTE (localDirectoryChanged): %1$S is program name (&brandshortname;)
> > +localDirectoryChanged=The path to Local Directory has changed. You must restart %1$S for this change to take effect and correctly receive messages.
> > +serverNameEmpty=The server name and user name must not be empty.

This sounds a little strange to me.  How about "Neither the server name nor the user name can be empty."?

::: mailnews/base/prefs/content/AccountManager.js
@@ +259,2 @@
>          Cc["@mozilla.org/embedcomp/prompt-service;1"]
>            .getService(Ci.nsIPromptService).alert(window, alertTitle, alertMsg);

(Along the lines of a comment below, we could change this to Services.prompt, but since you're not touching this code, we don't need to.)

@@ +314,1 @@
>                             .getService(Ci.nsIMsgAccountManager);

I think we can (and should) use mailServices.accounts for this.

@@ +329,2 @@
>        if (showAlert) {
> +        Cc["@mozilla.org/embedcomp/prompt-service;1"]

This seems like another good candidate for Services.prompt.

@@ +361,5 @@
>            serverChangeText = userChangeText;
>  
>        if (serverChangeText != undefined) {
> +          Cc["@mozilla.org/embedcomp/prompt-service;1"]
> +            .getService(Ci.nsIPromptService)

Blah blah blah Services.prompt.  ;)

@@ +373,5 @@
> +  let oldLocalPath = null;
> +  let newLocalPath = null;
> +  for (let i = 0; i < pageElements.length; i++) {
> +    if (pageElements[i].id) {
> +      let vals = pageElements[i].id.split(".");

I think, instead of splitting, then checking whether it's server and localPath, it might be more efficient to say:
if (/^server\.localPath\./.test(pageElements[i].id)
  oldLocalPath = accountValues.server.localPath.path; // both return nsILocalFile
  newLocalPath = getFormElementValue(pageElements[i]).path;
}

(I think that works out, because you know that type is "server" and slot is "localPath".)

@@ +389,5 @@
> +    let brandName = document.getElementById("bundle_brand").getString("brandShortName");
> +    let alertText = document.getElementById("bundle_prefs")
> +                            .getFormattedString("localDirectoryChanged", [brandName]);
> +    Cc["@mozilla.org/embedcomp/prompt-service;1"]
> +      .getService(Ci.nsIPromptService)

Services.prompt.
Attachment #589320 - Flags: ui-review?(bwinton) → ui-review+
(Assignee)

Comment 11

5 years ago
Thanks for the review, I'll fix it. But there is exactly one usage of Services.prompt in /mail and 0 in /mailnews (there are many in /suite). Are you sure I should use it? I would be out of place compared to the rest of the codebase.

Comment 12

5 years ago
(In reply to :aceman from comment #11)
> Thanks for the review, I'll fix it. But there is exactly one usage of
> Services.prompt in /mail and 0 in /mailnews (there are many in /suite). Are
> you sure I should use it? I would be out of place compared to the rest of
> the codebase.

I say definitely use it. Most of the code that uses prompts predates Services.jsm, so that's why they're not using it. (For bonus points, a followup bug to switch everything to use Services.prompts would be nice.)
(Assignee)

Comment 13

5 years ago
And what is the advantage of this approach? Speed, memory, shorter code?

Comment 14

5 years ago
Code cleanliness mostly, but I wouldn't be surprised if it were a tiny bit faster too.
(Assignee)

Comment 15

5 years ago
But the js files now need to 'Components.utils.import("resource://gre/modules/Services.jsm");'. Doesn't that add some overhead? Or is that module always loaded (by other components) so it is shared?
(In reply to :aceman from comment #15)
> But the js files now need to
> 'Components.utils.import("resource://gre/modules/Services.jsm");'. Doesn't
> that add some overhead? Or is that module always loaded (by other
> components) so it is shared?

Modules are shared so although it is listed, it won't be re-loaded. The other advantage is because the service and its values are then cached in js, we don't have to keep crossing the js/c++ boundary to get the service each time.
(Assignee)

Comment 17

5 years ago
I have tried the Services.prompt method in bug 719456 and it seems to work.

So, assign me to that global convert bug :)
(Assignee)

Comment 18

5 years ago
Created attachment 590371 [details] [diff] [review]
patch v2, updates per comment 10
Attachment #589320 - Attachment is obsolete: true
Attachment #590371 - Flags: ui-review?(bwinton)
Attachment #590371 - Flags: review?(iann_bugzilla)
Attachment #589320 - Flags: feedback?(iann_bugzilla)

Comment 19

5 years ago
Comment on attachment 590371 [details] [diff] [review]
patch v2, updates per comment 10

> // Check if the user and/or host names have been changed and
> // if so check if the new names already exists for an account.
Nit: This comment will need updating to reflect additional functionality.

> function checkUserServerChanges(showAlert) {
>   const Cc = Components.classes;
>   const Ci = Components.interfaces;
>+  var alertTitle = document.getElementById("bundle_brand")
>+                           .getString("brandShortName");
Maybe better to have this variable called something like shortName.
You also define:
var bundle = document.getElementById("bundle_prefs");
here and use it throughout the function

>   if (smtpService.defaultServer) {
>     try {
>       var smtpHostName = top.frames["contentFrame"].document.getElementById("smtp.hostname");
>       if (smtpHostName && hostnameIsIllegal(smtpHostName.value)) {
>         var alertMsg = document.getElementById("bundle_prefs")
You can use bundle here instead.

>                                .getString("enterValidHostname");
>         Cc["@mozilla.org/embedcomp/prompt-service;1"]
>           .getService(Ci.nsIPromptService).alert(window, alertTitle, alertMsg);
You changed the other prompt service and not this one :(

>+  var checkUser = true;
>   // There is no username defined for news so reset it.
>+  if (newType == "nntp") {
>     oldUser = newUser = "";
>+    checkUser = false;
>+  }

Define the variable alertText at the start of the if rather defining it several times throughout the function.
>   // If something is changed then check if the new user/host already exists.
>   if ( (oldUser != newUser) || (oldHost != newHost) ) {
>+    let rollback = false;
>+    if ((checkUser && (newUser == "")) || (newHost == "")) {
>+        rollback = true;
>         var alertText = document.getElementById("bundle_prefs")
>+                                .getString("serverNameEmpty");
No need for var here now and again you can also now use bundle here instead.
Nit: indentation
>+    } else {
>+      let newServer = MailServices.accounts.findRealServer(newUser, newHost, newType, 0);
>+      if (newServer) {
You could inline and remove the need for defining newServer.

>+          rollback = true;
>+          var alertText = document.getElementById("bundle_prefs")
>+                                  .getString("modifiedAccountExists");
No need for var here now and again you can also now use bundle here instead.
Nit: indentation
>       }
>+    }
>+    if (rollback) {
You don't actually need to define and use the variable rollback, you could just check that alertText is not null.
>+      if (showAlert)
>+        Services.prompt.alert(window, alertTitle, alertText);
>       // Restore the old values before return
>       if (newType != "nntp")
Just test checkUser instead.
>         setFormElementValue(pageElements[uIndx], oldUser);
>       setFormElementValue(pageElements[hIndx], oldHost);
>       return false;
>     }
> 
>     // If username is changed remind users to change Your Name and Email Address.
>-    // If serve name is changed and has defined filters then remind users to edit rules.
>+    // If server name is changed and has defined filters then remind users to edit rules.
>     if (showAlert) {
>-      var account = currentAccount
>+      var account = currentAccount;
>       var filterList;
>       if (account && (newType != "nntp")) {
Test checkUser rather than newType
>         var server = account.incomingServer;
>         filterList = server.getEditableFilterList(null);
>       }
>       var userChangeText, serverChangeText;
>       var bundle = document.getElementById("bundle_prefs");
This was defined at the beginning of the function now.

>+  if (oldLocalPath && newLocalPath && (oldLocalPath != newLocalPath)) {
>+    let brandName = document.getElementById("bundle_brand").getString("brandShortName");
We already have this from the beginning of the function either as alertTitle or shortName.

>+    let alertText = document.getElementById("bundle_prefs")
>+                            .getFormattedString("localDirectoryChanged", [brandName]);
No need for the let here.

Hope that all makes sense, r- for the moment.
Attachment #590371 - Flags: review?(iann_bugzilla) → review-
(Assignee)

Comment 20

5 years ago
(In reply to Ian Neal from comment #19)
> Comment on attachment 590371 [details] [diff] [review]

I have intentionally not equated alertText with brandname into a single variable. They contain the same text now, but later the alertText could contain something better then generic program name.

Maybe I should do it already, make alertText = "&brandShortName; Account settings" or something like that.

In the other things I agree with you, thanks for review.
(Assignee)

Comment 21

5 years ago
I mean alertTitle.
(Assignee)

Comment 22

5 years ago
Created attachment 590568 [details] [diff] [review]
patch v3, updates per comment 19

Patch updated with all the nits. Also alertTitle now has better text not equal brandname, I used the existing string of pane name ("Server settings"). Also Cc and Ci were not needed anymore.
Attachment #590371 - Attachment is obsolete: true
Attachment #590568 - Flags: ui-review?(bwinton)
Attachment #590568 - Flags: review?(iann_bugzilla)
Attachment #590371 - Flags: ui-review?(bwinton)

Comment 23

5 years ago
Comment on attachment 590568 [details] [diff] [review]
patch v3, updates per comment 19

>+  const prefBundle = document.getElementById("bundle_prefs");
>+  const alertTitle = prefBundle.getString("prefPanel-server");
Not 100% sure we can use const for both of these, so maybe worth double checking with standard8 or neil.

>+  var alertText = null;
>   if (smtpService.defaultServer) {
>     try {
>       var smtpHostName = top.frames["contentFrame"].document.getElementById("smtp.hostname");
>       if (smtpHostName && hostnameIsIllegal(smtpHostName.value)) {
>+        alertText = prefBundle.getString("enterValidHostname");
>+        Services.prompt.alert(window, alertTitle, alertText);
You could inline the final argument and remove the need to use alertText variable, then you would not need to reset it to null. Alternatively stick to using alertMsg variable.
Also we do not seem to check if any other hostname is legal but you might want to do that in a separate bug.
Plus we currently have two copies of the hostnameIsIllegal function (one in amUtils.js and one in aw-server.js but I think only the former is now used as AccountWizard.xul loads both scripts), the function needs some work to make it actually do better detection of illegal hostnames - probably best done in a separate bug (bug 80855 perhaps)

r=me with the first issue checked and comments on where you think the illegal hostname work should be done.
Attachment #590568 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Comment 24

5 years ago
I do not touch the logic of checking the legality of the server. So I do not 
currently know how it works.

I'll take bug 80855 and try to fix the duplicates there.

This patch is already far away from the original problem :)
(Assignee)

Updated

5 years ago
Attachment #590568 - Flags: review?(mbanner)
Comment on attachment 590568 [details] [diff] [review]
patch v3, updates per comment 19

I think I have to mark this ui-r-, but mostly on a technicality and request.

First the request.  I think we should give the user the option of restarting now or later in that warning dialog.  Would you be up for a patch that implements that?

And next the technicality.  When I saw the dialog box, I thought clicking "OK" _would_ restart Thunderbird, and was alarmed by the lack of a "I'll do it later!" button.  I think you can fix this with better wording.  (Maybe say something about how they will have to restart Thunderbird themselves, and we're not doing it now?  I dunno.  If you post some choices, I'll pick my favourite, and then give you a ui-r for that.)

Thanks,
Blake.
Attachment #590568 - Flags: ui-review?(bwinton) → ui-review-
(Assignee)

Comment 26

5 years ago
I could add the restart if I find out how to call a restart of TB.

Comment 27

5 years ago
Why would you like to give the user the option not to restart now as it will mess things up? Just let it be "restart" or "cancel the whole change".
(Assignee)

Comment 28

5 years ago
Ok, I have found out how to do a restart.
So can anybody decide what to do?

The Local Directory path has changed. TB needs to restart now for this change to take effect.

[Restart] [Restart later]
Comment on attachment 590568 [details] [diff] [review]
patch v3, updates per comment 19

Cancelling review until we've got a ui-review+
Attachment #590568 - Flags: review?(mbanner)

Comment 30

5 years ago
(In reply to :aceman from comment #28)
> The Local Directory path has changed. TB needs to restart now for this
> change to take effect.
> 
> [Restart] [Restart later]

The problem is it takes affect, but doesn't work properly until restart, right?
(Assignee)

Comment 31

5 years ago
Yes. I immediately takes effect only partly (for some code paths). More in bug 2654.

Do you suggest better wording?

Comment 32

5 years ago
I'd suggest just letting the user chose to restart or cancel, as i don't thing the UI should let the user shoot himself in the foot. Ever so often programs claim they need to restart to work properly, and often it's not really necessary. As it's really necessary here, there should be no choice.

 [Apply & Restart] [Cancel]
(Assignee)

Comment 33

5 years ago
Ah, you mean when the user uses "Cancel" then revert the change in the field (in the same way invalid hostname/username are silently reverted)?

Comment 34

5 years ago
Yes.
(Assignee)

Comment 35

5 years ago
Thanks, will do.
(Assignee)

Comment 36

5 years ago
So what about this:

The Local Directory path has changed. Would you like to restart %1$S now for this change to take effect?

[Yes] [Revert]

This would reuse standard buttons and strings. Or do you really want [Apply & Restart] ?

Comment 37

5 years ago
It's best to use verbs for buttons, not yes/no type of buttons.

How about

%1$S needs to restart to apply the change to the Local Directory.

 [Restart] [Cancel]
(Assignee)

Comment 38

5 years ago
I wanted to use some of the standard buttons as defined here:
http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/components/prompts/src/nsPrompter.js#173
But OK, I can add a new "Restart" string.
(Assignee)

Comment 39

5 years ago
Bwinton, there are now several proposals in the comments here. You can choose now.

The rest of the code is prepared, with restart or reverting the value in case of "cancel".
Cool.  I think I agree with Magnus' last proposal of Restart and Cancel.
And since that was my main request, I'm going to give it a ui-r=me, sight-unseen.  ;)
(Assignee)

Comment 41

5 years ago
Magnus, are you authorized to give reviews in this part? Or should I try standard8 again? :)
(Assignee)

Comment 42

5 years ago
Created attachment 596129 [details] [diff] [review]
patch v4, with restart
Attachment #590568 - Attachment is obsolete: true
Attachment #596129 - Flags: ui-review?(bwinton)
Attachment #596129 - Flags: review?(iann_bugzilla)
Comment on attachment 596129 [details] [diff] [review]
patch v4, with restart

Review of attachment 596129 [details] [diff] [review]:
-----------------------------------------------------------------

You didn't _really_ need to ask me for a ui-review, but since you did, I like it.

Of course, I did go through the code, and found the following thing you probably want to change…

::: mailnews/base/prefs/content/AccountManager.js
@@ +257,5 @@
> +                                                      "restart");
> +  if (!cancelQuit.data)
> +    Components.classes["@mozilla.org/toolkit/app-startup;1"]
> +              .getService(nsIAppStartup)
> +              .quit(nsIAppStartup.eRestart | nsIAppStartup.eAttemptQuit);

Services.startup.quit, naturally…  ;)
Attachment #596129 - Flags: ui-review?(bwinton) → ui-review+
(Assignee)

Comment 44

5 years ago
Created attachment 596164 [details] [diff] [review]
patch v5

Thanks, must have overlooked that one in Services.jsm.
Attachment #596129 - Attachment is obsolete: true
Attachment #596164 - Flags: ui-review+
Attachment #596164 - Flags: review?(iann_bugzilla)
Attachment #596129 - Flags: review?(iann_bugzilla)

Comment 45

5 years ago
Comment on attachment 596164 [details] [diff] [review]
patch v5

>+localDirectoryChanged=%1$S needs to restart now to apply the change to the Local directory.
This doesn't sound quite correct, I think it needs to be:
"1$S now needs to restart to apply the change to the Local directory setting."

r=me with that fixed/addressed
Attachment #596164 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Comment 46

5 years ago
Created attachment 596401 [details] [diff] [review]
patch v6

Sure.
Attachment #596164 - Attachment is obsolete: true
Attachment #596401 - Flags: ui-review+
Attachment #596401 - Flags: review?(mkmelin+mozilla)
(Assignee)

Updated

5 years ago
Blocks: 577775

Comment 47

5 years ago
Comment on attachment 596401 [details] [diff] [review]
patch v6

I'm only a reviewer for mail/
Attachment #596401 - Flags: review?(mkmelin+mozilla)
(Assignee)

Updated

5 years ago
Attachment #596401 - Flags: review?(mbanner)
(Assignee)

Updated

5 years ago
Blocks: 738810
Comment on attachment 596401 [details] [diff] [review]
patch v6

Review of attachment 596401 [details] [diff] [review]:
-----------------------------------------------------------------

Generally seems fine, however it has suffered some bitrot. I tried fixing it locally and then tried testing this on a pop3 account and on local folders, but when I selected a different folder I got:

Error: currentServer.localPath is null
Source File: chrome://messenger/content/amUtils.js
Line: 73

::: mailnews/base/prefs/content/AccountManager.js
@@ +248,4 @@
>    return true;
>  }
>  
> +function restartProgram() {

I think you could just change this to be Application.restart() (see extApplication.js)
Attachment #596401 - Flags: review?(mbanner) → review-
(Assignee)

Comment 49

5 years ago
Created attachment 609414 [details] [diff] [review]
patch v7

Fixes the bitrot and uses Application.restart();
It seems I am the only one changing amUtils.js recently:
http://hg.mozilla.org/comm-central/log/fef65eed3690/mailnews/base/prefs/content/amUtils.js and I didn't touch currentServer.localPath .
I suspect it is a problem from the IM landing. After I deleted my IM account (of type JS test) the error vanished and I could OK the Account manager (when it restarted).
Attachment #596401 - Attachment is obsolete: true
Attachment #609414 - Flags: review?(mbanner)
(In reply to :aceman from comment #49)
> I suspect it is a problem from the IM landing. After I deleted my IM account
> (of type JS test) the error vanished and I could OK the Account manager
> (when it restarted).

Thanks, I filed bug 739555.
(Assignee)

Comment 51

5 years ago
(In reply to :aceman from comment #49)
> After I deleted my IM account
> (of type JS test) the error vanished and I could OK the Account > manager (when it restarted).
I mean "and THEN it restarted TB".
Attachment #609414 - Flags: review?(mbanner) → review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
http://hg.mozilla.org/comm-central/rev/b08d54e349a4

(And let me just say that I love your patches. So painless to check in!)
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite-
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 14.0
Comment on attachment 609414 [details] [diff] [review]
patch v7

Review of attachment 609414 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/base/prefs/content/AccountManager.js
@@ +61,5 @@
>   */
>  
>  Components.utils.import("resource:///modules/iteratorUtils.jsm");
> +Components.utils.import("resource://gre/modules/Services.jsm");
> +Components.utils.import("resource://gre/modules/mailServices.js");

mailServices.js is not in gre.
Attachment #609414 - Flags: feedback-
(Assignee)

Updated

5 years ago
Blocks: 739931
(Assignee)

Comment 54

5 years ago
Thanks, filed separately as there is one other occurrence of it.
Strange that it works regardless.
No longer blocks: 739931
Depends on: 739931
(In reply to :aceman from comment #54)
> Thanks, filed separately as there is one other occurrence of it.

The other was already filed. Seems like overkill, but anyway.

Updated

5 years ago
No longer blocks: 577775
(Assignee)

Updated

5 years ago
Blocks: 577775
(Assignee)

Updated

3 years ago
Blocks: 2654
No longer depends on: 2654
You need to log in before you can comment on or make changes to this bug.