"Get New Messages for > All accounts" is missing

RESOLVED FIXED in Thunderbird 3.0rc1

Status

RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: bugzilla2007, Assigned: bwinton)

Tracking

({regression})

unspecified
Thunderbird 3.0rc1
regression
Dependency tree / graph
Bug Flags:
blocking-thunderbird3 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [no l10n impact])

Attachments

(2 attachments, 4 obsolete attachments)

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...?
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.
Blocks: 465269
Flags: blocking-thunderbird3?
Keywords: regression
Summary: "Get New Messages for > All accounts" is missing in TB3 → "Get New Messages for > All accounts" is missing
(Assignee)

Updated

9 years ago
Assignee: nobody → bwinton
Status: NEW → ASSIGNED
(Assignee)

Comment 2

9 years ago
Created attachment 400556 [details] [diff] [review]
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.
Attachment #400556 - Flags: superreview?(bienvenu)
Attachment #400556 - Flags: review?(bugmail)
Attachment #400556 - Flags: approval-thunderbird3?

Comment 3

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

this fixes the issue for me.
Attachment #400556 - Flags: superreview?(bienvenu) → superreview+

Comment 4

9 years ago
though Neil might have an opinion on the best way to do this.
(Assignee)

Comment 5

9 years ago
Created attachment 400569 [details] [diff] [review]
[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.
Attachment #400569 - Flags: superreview?(bienvenu)
Attachment #400569 - Flags: review?(bugmail)
Attachment #400569 - Flags: approval-thunderbird3?
(Assignee)

Comment 6

9 years ago
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 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.
Attachment #400569 - Flags: review?(bugmail) → review-
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.
Attachment #400569 - Flags: approval-thunderbird3? → approval-thunderbird3+

Updated

9 years ago
Attachment #400569 - Flags: superreview?(bienvenu) → superreview+

Comment 9

9 years ago
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

9 years ago
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 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.
Attachment #400569 - Flags: review- → review+
Flags: blocking-thunderbird3? → blocking-thunderbird3+
Target Milestone: --- → Thunderbird 3.0rc1
Attachment #400556 - Attachment is obsolete: true
Attachment #400556 - Flags: review?(bugmail)
Attachment #400556 - Flags: approval-thunderbird3?

Updated

9 years ago
Whiteboard: [no l10n impact]
(Assignee)

Comment 12

9 years ago
Created attachment 401067 [details] [diff] [review]
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.
Attachment #401067 - Flags: superreview?(bienvenu)
Attachment #401067 - Flags: review?(bugmail)
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.
Attachment #401067 - Flags: superreview?(bienvenu)
Attachment #401067 - Flags: review?(bugmail)
Attachment #401067 - Flags: review-
(Assignee)

Updated

9 years ago
Attachment #400569 - Attachment description: A patch that fixes the bug in the previous patch. → [checked-in] A patch that fixes the bug in the previous patch.

Updated

9 years ago
Depends on: 517134
(Assignee)

Comment 14

9 years ago
Created attachment 401445 [details] [diff] [review]
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.
Attachment #401067 - Attachment is obsolete: true
Attachment #401445 - Flags: superreview?(bienvenu)
Attachment #401445 - Flags: review?(bugmail)
(Assignee)

Updated

9 years ago
Whiteboard: [no l10n impact] → [no l10n impact][patch up]
(Assignee)

Updated

9 years ago
Attachment #401445 - Flags: superreview?(bienvenu)
Attachment #401445 - Flags: review?(bugmail)
(Assignee)

Comment 15

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

Found a bug.  Will fix.
(Assignee)

Comment 16

9 years ago
Created attachment 401850 [details] [diff] [review]
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.
Attachment #401445 - Attachment is obsolete: true
Attachment #401850 - Flags: superreview?(bienvenu)
Attachment #401850 - Flags: review?(bugmail)
(Assignee)

Updated

9 years ago
Depends on: 517617
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.
Attachment #401850 - Flags: review?(bugmail) → review+

Updated

9 years ago
Attachment #401850 - Flags: superreview?(bienvenu) → superreview+

Comment 18

9 years ago
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.
(Assignee)

Comment 19

9 years ago
Created attachment 402011 [details] [diff] [review]
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.
Attachment #401850 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Keywords: checkin-needed
Whiteboard: [no l10n impact][patch up] → [no l10n impact]
Checked in: http://hg.mozilla.org/comm-central/rev/2aea95c35322
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Duplicate of this bug: 518586
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.
(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.
You need to log in before you can comment on or make changes to this bug.