Closed Bug 442248 Opened 16 years ago Closed 16 years ago

In <msgMail3PaneWindow.js>, "Error: gMsgFolderSelected is undefined"

Categories

(SeaMonkey :: MailNews: Message Display, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
seamonkey2.0a1

People

(Reporter: sgautherie, Assigned: neil)

Details

Attachments

(2 files, 4 obsolete files)

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.0.1pre) Gecko/2008062703 Thunderbird/3.0a2pre] (nightly) (W2Ksp4)

No bug.


[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.0.1pre) Gecko/2008062701 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

1. Go offline.
2. Create (dumb) IMAP account.
2r. Each opening of 3-pane window:
{{
Error: gMsgFolderSelected is undefined
Source File: chrome://messenger/content/msgMail3PaneWindow.js
Line: 253
}}
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.0.2pre) Gecko/2008070402 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

Venkman call stack is
{{
#0: function anonymous(event=XPComponent:{5}, folder=XPComponent:{133}) in <chrome://messenger/content/msgMail3PaneWindow.js> line 253
 1: performBiff / x-jsd:native-code
#2: function loadStartFolder(initialUri=null:null) in <chrome://messenger/content/msgMail3PaneWindow.js> line 842
#3: function (null)() in <chrome://messenger/content/msgMail3PaneWindow.js> line 750
}}

NB: (In Venkman)
Use 'Stop for Errors' (or a BreakPoint),
but not 'Stop for Exceptions',
otherwise the Inbox folder is not selected and this error doesn't show up.

I hope the patch for (POP3) bug 442251 will fix this too: let's wait and see.
Depends on: 442251
(In reply to comment #1)
> I hope the patch for (POP3) bug 442251 will fix this too: let's wait and see.

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.0.2pre) Gecko/2008071115 SeaMonkey/2.0a1pre] (SEA-WIN32-TBOX-trunk) (W2Ksp4)

I was wrong to write that :-<
No longer depends on: 442251
Attached patch (Av1) <msgMail3PaneWindow.js> (obsolete) — Splinter Review
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.0.1pre) Gecko/2008070203 Thunderbird/3.0a2pre] (nightly) (W2Ksp4)

From the comment 1 stack analysis, the code at these 3 places is nearly-enough identical, so I don't know why TB doesn't show this bug.


[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.0.2pre) Gecko/2008071115 SeaMonkey/2.0a1pre] (SEA-WIN32-TBOX-trunk) (W2Ksp4)

Anyway, adding a |CheckOnline()| call/test fixes it for SM.

NB: Fwiw, I tried to change and move SM
|setTimeout("loadStartFolder(gStartFolderUri);", 0);|
to TB
|loadStartFolder(gStartFolderUri);|
but that didn't help.

PS: If you find a better/underlying fix, you're welcome...
Assignee: mail → sgautherie.bz
Status: NEW → ASSIGNED
Attachment #329176 - Flags: review?
Attachment #329176 - Flags: review? → review?(mnyromyr)
(In reply to comment #3)

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.0.2pre) Gecko/2008071103 Thunderbird/3.0a2pre] (nightly) (W2Ksp4)

> I don't know why TB doesn't show this bug.

But, as a hint, TB (offline/online) selects the imap account itself whereas SM selects the Inbox folder...
Comment on attachment 329176 [details] [diff] [review]
(Av1) <msgMail3PaneWindow.js>

>Index: msgMail3PaneWindow.js
>===================================================================
>         if (!initialUri && isLoginAtStartUpEnabled && gLoadStartFolder
>             && !defaultServer.isDeferredTo &&
>-            defaultServer.rootFolder == defaultServer.rootMsgFolder)
>+            defaultServer.rootFolder == defaultServer.rootMsgFolder &&
>+            CheckOnline())
>           defaultServer.performBiff(msgWindow);

The Offline() check should be second.
Also, for enhanced readability, put each condition on its own line and embrace ;-) the biff call.

r=me with that.
Attachment #329176 - Flags: review?(mnyromyr) → review+
Attached patch (Av1a) <msgMail3PaneWindow.js> (obsolete) — Splinter Review
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.0.2pre) Gecko/2008071502 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

Av1, with comment 5 suggestion(s),
plus (while there):

*Add a comment for |var gLoadStartFolder|.
*Remove a useless |var folderTree|.
*Remove superfluous |var isLoginAtStartUpEnabled|.
*Merge if's !
 *and fix a few nits nearby...
*Add a missing |defaultAccount = null;| !
Attachment #329176 - Attachment is obsolete: true
Attachment #330515 - Flags: review?(mnyromyr)
Comment on attachment 330515 [details] [diff] [review]
(Av1a) <msgMail3PaneWindow.js>

Nice!

>Index: msgMail3PaneWindow.js
>===================================================================
>+        // |initialUri| is null on startup.
>+        if (initialUri)
>             startFolderResource = RDF.GetResource(initialUri);
>         else
>         {

r=me with braces around the initial if branch.
Attachment #330515 - Flags: review?(mnyromyr) → review+
Attached patch (Av1b) <msgMail3PaneWindow.js> (obsolete) — Splinter Review
Av1a, with comment 7 suggestion(s).
Attachment #330515 - Attachment is obsolete: true
Component: MailNews: Account Manager → MailNews: Main Mail Window
Flags: in-litmus-
Keywords: checkin-needed
OS: Windows 2000 → All
Hardware: PC → All
Target Milestone: --- → seamonkey2.0alpha
We *do* require sr as long as noone says differently.
As you were told repeatedly... ;-)
Keywords: checkin-needed
Attachment #330684 - Flags: superreview?(jag)
(In reply to comment #9)
> We *do* require sr as long as noone says differently.

Indeed, sorry, didn't pay attention on this one :-(

> As you were told repeatedly... ;-)
Attachment #330684 - Flags: superreview?(jag) → superreview?(neil)
Flags: in-litmus- → in-testsuite-
IIRC:
{{
[10:03]	<NeilAway>	sgautherie: actually I meant to look up my original rewrite of loadStartFolder
}}
Attached patch Original rewrite (obsolete) — Splinter Review
* Removed useless folder tree variable
* Switched from RDF resources to URI strings
* If only news accounts, select news server account central by default
* Removed initialUri check when performing biff, since
  isLoginAtStartupEnabled can't be set if initialUri is.
Attachment #334876 - Attachment is obsolete: true
Attachment #334878 - Flags: review?(mnyromyr)
Attachment #334878 - Flags: review?(mnyromyr) → review+
Comment on attachment 334878 [details] [diff] [review]
Proposed patch
[Checkin: Comment 16]

>+++ b/mailnews/base/resources/content/msgMail3PaneWindow.js	Thu Aug 21 13:33:40 2008 +0100
> function loadStartFolder(initialUri)
> {
...
>     var startFolderResource = null;

This one can go as well.

>+                if (inboxFolder)
>+                    initialUri = inboxFolder.URI;

No need to heed the 4 space indent in the leaf here.

>+        if (isLoginAtStartUpEnabled && gLoadStartFolder &&
>+            !defaultServer.isDeferredTo &&
>             defaultServer.rootFolder == defaultServer.rootMsgFolder)

Probably worth to put each && clause on its own line for enhanced readability?
Attachment #330684 - Flags: superreview?(neil)
Comment on attachment 330684 [details] [diff] [review]
(Av1b) <msgMail3PaneWindow.js>

Sorry, we don't need this.
Pushed changeset dd10c695faf9 to comm-central.
Assignee: sgautherie.bz → neil
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
QA Contact: message-display
Resolution: --- → FIXED
Attachment #334878 - Attachment description: Proposed patch → Proposed patch [Checkin: Comment 16]
V.Fixed between
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b1pre) Gecko/20080907003401 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)
and
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b1pre) Gecko/20080908013336 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)
Status: RESOLVED → VERIFIED
Comment on attachment 334878 [details] [diff] [review]
Proposed patch
[Checkin: Comment 16]

(I'm checking for the parts of my patch which still apply.)

>+        SelectFolder(initialUri);
> 
>         // Perform biff on the server to check for new mail, except for imap
>-        // or a pop3 account that is deferred or deferred to,
>-        // or the case where initialUri is non-null (non-startup)
>-        if (!initialUri && isLoginAtStartUpEnabled && gLoadStartFolder
>-            && !defaultServer.isDeferredTo &&
>+        // or a pop3 account that is deferred or deferred to.
>+        if (isLoginAtStartUpEnabled && gLoadStartFolder &&
>+            !defaultServer.isDeferredTo &&
>             defaultServer.rootFolder == defaultServer.rootMsgFolder)
>           defaultServer.performBiff(msgWindow);
>-
>-        SelectFolder(startFolder.URI);

Does |performBiff()| need to be executed after |SelectFolder()| only ?
Or could it (still) be done before ... in the try+if block ?
(In reply to comment #18)
> Does |performBiff()| need to be executed after |SelectFolder()| only ?
I think it's better this way.
Av1b, unbitrotted/merged,

and extended:
*Remove superfluous variables.
*Add/Update documentation.
Attachment #330684 - Attachment is obsolete: true
Attachment #338872 - Flags: superreview?(neil)
Attachment #338872 - Flags: review?(mnyromyr)
Comment on attachment 338872 [details] [diff] [review]
(Av2) <msgMail3PaneWindow.js> additional doc and fix
[Checkin: Comment 23]

>           if (gNotifyDefaultInboxLoadedOnStartup && (folder.flags & 0x1000))
Hmm, this 0x1000 should be nsMsgFolderFlags.Inbox - I thought I'd changed all of these in bug 436044...

>                 var inboxFolder = rootMsgFolder.getFolderWithFlags(0x1000);
Whoa, I missed another :-(
Attachment #338872 - Flags: superreview?(neil) → superreview+
(In reply to comment #21)
> this 0x1000 should be nsMsgFolderFlags.Inbox

I filed bug 455549.
Attachment #338872 - Flags: review?(mnyromyr) → review+
Comment on attachment 338872 [details] [diff] [review]
(Av2) <msgMail3PaneWindow.js> additional doc and fix
[Checkin: Comment 23]

"Hunk #3 succeeded at 765 with fuzz 2 (offset 2 lines)."

http://hg.mozilla.org/comm-central/rev/42f6a631882a
Attachment #338872 - Attachment description: (Av2) <msgMail3PaneWindow.js> additional doc and fix → (Av2) <msgMail3PaneWindow.js> additional doc and fix [Checkin: Comment 23]
You need to log in before you can comment on or make changes to this bug.