516353 - "Get New Messages for > All accounts" is missing

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Thomas D. (:thomas8)]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a1pre) Gecko/20090912 Shredder/3.1a1pre

TB 2 has a command that checks for new messages on ALL accounts, both in the "Get Mail" button dropdown and in the File > Get new messages for - Menu.
Unfortunately, that command has disappeared in TB3...
Don't tell me it's a feature...?

Comment 1

[Mark Banner (:standard8)]

This is a regression from bug 465269, it appears the folder location menu is now clearing out all items at the wrong time (or not accounting for keeping some items).

Seeing as the folder location menu on the toolbar and the various Get Mail menus use the same widget, we need to account for this.

Comment 2

[Blake Winton (:bwinton) (:☕️)]

Attachment: A patch to not delete our children quite so often.

But to do that, we'll need to change _initialized to an attribute, so that it'll get copied when we customize the toolbar.

Note: There's still a bug with sub-folders showing the root elements after you customize the toolbar, because the _parentFolder field also gets magically transmuted into null when you customize the toolbar, but we could probably push the fix for that off to rc1.

Thanks,
Blake.

Comment 3

[David :Bienvenu]

Comment on attachment 400556 [details] [diff] [review]
A patch to not delete our children quite so often.

this fixes the issue for me.

Comment 4

[David :Bienvenu]

though Neil might have an opinion on the best way to do this.

Comment 5

[Blake Winton (:bwinton) (:☕️)]

Attachment: [checked-in] A patch that fixes the bug in the previous patch.

So this is the previous patch with the addition of converting _parentFolder into an attribute, so that it gets saved, and we avoid the bug I mentioned in the first patch.

I'm not sure whether we want to go with this, or with the previous patch, so I'm not marking this as obsoleting the previous patch.

Thanks,
Blake.

Comment 6

[Blake Winton (:bwinton) (:☕️)]

As a side note, both patches remove all the account entries from the Get Messages button when you customize the toolbar.  I'm working on it.

Comment 7

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 400569 [details] [diff] [review]
[checked-in] A patch that fixes the bug in the previous patch.

The get mail button thing breaks after I bring up the customize dialog.  The customize dialog is a trouble-maker.

Comment 8

[Mark Banner (:standard8)]

Comment on attachment 400569 [details] [diff] [review]
[checked-in] A patch that fixes the bug in the previous patch.

a=Standard8 for whichever patch we go with for b4.

Comment 9

[David :Bienvenu]

Comment on attachment 400569 [details] [diff] [review]
[checked-in] A patch that fixes the bug in the previous patch.

some spaces after '?' and ':' would be nice. Most people prefer the '?' on the prev line (not me though :-) )

Comment 10

[David :Bienvenu]

http://hg.mozilla.org/comm-central/rev/5bddaf7248c8 pushed - not marking fixed because there's more to do, but this is what we want for b4.

Comment 11

[Mark Banner (:standard8)]

Comment on attachment 400569 [details] [diff] [review]
[checked-in] A patch that fixes the bug in the previous patch.

Andrew gave r+ for this over irc for beta 4 as it is restoring the functionality for users the best we can at the moment.

Comment 12

[Blake Winton (:bwinton) (:☕️)]

Attachment: Handle the post-customization case, too.

(In reply to comment #7)
> The get mail button thing breaks after I bring up the customize dialog.  The
> customize dialog is a trouble-maker.

Man, is it ever.  I don't know why closing the pane silently deletes some of my children, but at least now we seem to handle that case a little better.

Thanks,
Blake.

Comment 13

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 401067 [details] [diff] [review]
Handle the post-customization case, too.

Okay, so I've looked at this more.  I think the folder widget is making its own troubles and the toolbar customization code is actually fairly blameless.  The folder is doing two things that I think are leading to trouble:

1) Using cloneNode on itself.  We should stop doing that.

2) Forcing itself to be created when its parent is a menulist.  We should not do this when it is in a wrapper.  The check '/wrapper-.*/.test(this.parentNode.parentNode.parentNode.id)' should be sufficient.

With this done, we can avoid trying to persist state in attributes.  Once we stop cloning ourselves, and once we stop trying to be a live XBL binding in the toolbar palette, there should no longer be any 'live' instances that should not be live.

Please make those changes and lose the kludgey attribute stuff.

Comment 14

[Blake Winton (:bwinton) (:☕️)]

Attachment: Let's try a different tack on this.

(In reply to comment #13)
> (From update of attachment 401067 [details] [diff] [review])
> Okay, so I've looked at this more.  I think the folder widget is making its
> own troubles and the toolbar customization code is actually fairly blameless.

Sure.  Not totally blameless, but fairly blameless.  :)

> The folder is doing two things that I think are leading to trouble:
> 1) Using cloneNode on itself.  We should stop doing that.

Stopped.

> 2) Forcing itself to be created when its parent is a menulist.  We should not
> do this when it is in a wrapper.  The check
> '/wrapper-.*/.test(this.parentNode.parentNode.parentNode.id)' should be
> sufficient.

Checked.

With these two changes, we were still getting created multiple times, with new field values and with old children, so while this was some good cleanup, it didn't fix the original problem.

But, now that I can tell when it's being put into a wrapper, I can clear out its children, and then when it gets re-created, we only have one copy of them, which is what we want.

> With this done, we can avoid trying to persist state in attributes.  Once we
> stop cloning ourselves, and once we stop trying to be a live XBL binding in
> the toolbar palette, there should no longer be any 'live' instances that
> should not be live.
> 
> Please make those changes and lose the kludgey attribute stuff.

Done, and done.

As a side note, I've also tested this with the newsgroup message folder stuff, and the newsgroups appear.  And when I add and remove servers, it all seems to work correctly.

Finally, since we're no longer using GetMsgFolderFromUri, I think this might also fix bug 517134, without applying Kent's patch.

Thanks,
Blake.

Comment 15

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 401445 [details] [diff] [review]
Let's try a different tack on this.

Found a bug.  Will fix.

Comment 16

[Blake Winton (:bwinton) (:☕️)]

Attachment: The previous patch, with the bug fixed.

So the regex asuth gave works most of the time, but not for the File menu-button, because it's got its menupopup wrapped in another node, so we didn't see that it was being wrapped.  So now we loop through all our parents, checking for wrappers.

Thanks,
Blake.

Comment 17

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 401850 [details] [diff] [review]
The previous patch, with the bug fixed.

It's in the file menu too?  This menu sure gets around.

Thank you for the stick-to-itiveness on this bug and driving the ghosts out.

Comment 18

[David :Bienvenu]

Comment on attachment 401850 [details] [diff] [review]
The previous patch, with the bug fixed.

period at the end of this sentence:

+        // If we are a child of a menulist, and we aren't in a wrapper, we
+        // need to build our content right away, otherwise the menulist
+        // won't have proper sizing

"because" here:

+        // But if we're in a wrapper, remove our children, cause we're
+        // getting re-created when the toolbar customization closes.

Comment 19

[Blake Winton (:bwinton) (:☕️)]

Attachment: The previous patch, with bienvenu's nits fixed.

(In reply to comment #17)
> It's in the file menu too?  This menu sure gets around.

Yeah, and the message and newsgroup filters too, apparently.

> Thank you for the stick-to-itiveness on this bug and driving the ghosts out.

My pleasure.  :)

(In reply to comment #18)
> period at the end of this sentence:

Fixed.

> "because" here:

And fixed.

Thanks,
Blake.

Comment 20

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/2aea95c35322

Comment 22

[[:Aureliano Buendía]]

patch is landed?

Here the bug is still open on

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.4pre) Gecko/20090926 Lightning/1.0pre Shredder/3.0pre ID:20090926031214

STR:
1. rx click on toolbar and select "custumize..."
2. on "customize toolbar" window click done button.

goto "Get Mail" toolbar button: "get all new messages" is missing.

When restart TB, all is fine.

Comment 23

[Mark Banner (:standard8)]

(In reply to comment #22)
> patch is landed?
> 
> Here the bug is still open on

That's a slightly different scenario (this one was that get all new messages was completely missing all the time), please file a new bug for it.