Closed
Bug 534448
Opened 15 years ago
Closed 14 years ago
Folder dropdown for newsgroup filters no longer shows server
Categories
(MailNews Core :: Filters, defect)
MailNews Core
Filters
Tracking
(blocking-thunderbird3.1 .1+, thunderbird3.1 .1-fixed)
RESOLVED
FIXED
Thunderbird 3.3a1
People
(Reporter: rkent, Assigned: bwinton)
References
Details
(Keywords: regression)
Attachments
(3 files, 3 obsolete files)
11.67 KB,
patch
|
asuth
:
review+
|
Details | Diff | Splinter Review |
960 bytes,
patch
|
asuth
:
review+
Bienvenu
:
superreview+
|
Details | Diff | Splinter Review |
13.48 KB,
patch
|
standard8
:
approval-thunderbird3.1.1+
|
Details | Diff | Splinter Review |
In TB 2, you could select a newsgroup server in the filter list editor, as well as folders. In TB3 you can only select folders. There is a workaround: If you select the newsgroup server in the folder tree, then ask to edit filters, that server shows up as the edited item.
Reporter | ||
Updated•14 years ago
|
Assignee: nobody → kent
Status: NEW → ASSIGNED
Reporter | ||
Updated•14 years ago
|
Keywords: regression
Reporter | ||
Comment 1•14 years ago
|
||
This is a regression from Bug 520457, as that clears the children of a menupop when we need those children to display the nntp server.
Blocks: 520457
Reporter | ||
Comment 2•14 years ago
|
||
Attachment #419049 -
Flags: review?(bwinton)
Reporter | ||
Updated•14 years ago
|
blocking-thunderbird3.0: --- → ?
Whiteboard: [needs r bwinton]
Assignee | ||
Comment 3•14 years ago
|
||
So, I like your patch, with the one caveat that it could break folder-popups with a headlabels attribute that were in the toolbar. Yeah, we don't have any now (I grepped through the code), but extensions might, and we might add one in the future (possibly replacing the showFileHereLabel, since they seem kinda similar to me). So I dug a little into what was going on there, and the change that got rid of those was for bug 518586. Now, I noticed that the only way we would have a labels variable at line 511 of folderWidgets.xml was if it was set on line 458, which is in an "if (this.hasAttribute("headlabels"))" block, so what do you think of just pretending that we didn't generate those two labels, so that we don't clear them out on line 748? That would have the entirely-theoretical advantage that if we ended up adding a headlabels-attributed folder-popup into the toolbar, it wouldn't break as badly. I think. :) So, which way would you like to pursue, or shall we get more input from other people? Thanks, Blake.
Attachment #419052 -
Flags: review?(kent)
Reporter | ||
Comment 4•14 years ago
|
||
Comment on attachment 419052 [details] [diff] [review] A slightly different approach. This approach is fine with me, so I'll give my OK (which isn't worth much though, I am not a peer). But I'll let bwinton decide at this point how to get any additional reviews that are needed. Moving assign to him.
Attachment #419052 -
Flags: review?(kent) → review+
Reporter | ||
Updated•14 years ago
|
Assignee: kent → bwinton
Whiteboard: [needs r bwinton]
Reporter | ||
Updated•14 years ago
|
Attachment #419049 -
Attachment is obsolete: true
Attachment #419049 -
Flags: review?(bwinton)
Assignee | ||
Comment 5•14 years ago
|
||
Comment on attachment 419052 [details] [diff] [review] A slightly different approach. So, to get more people looking at it, I'll just ask for reviews. But while I'm doing that, I'm also going to ask the reviewers to check out the other attachment (https://bugzilla.mozilla.org/attachment.cgi?id=419049), and let me know which patch seems like a better way to go. And I'll attach some new mozmill tests for this regression as a separate patch. Thanks, Blake.
Attachment #419052 -
Flags: superreview?(bienvenu)
Attachment #419052 -
Flags: review?(bugmail)
Assignee | ||
Comment 6•14 years ago
|
||
It fails without the other patch, and passes with the other patch, but it seems kind of hacky. asuth? Any ideas on how I could get the sub-popup to pop up? I've left in the stuff that doesn't work as comments, so you can see what I've tried. Thanks, Blake.
Attachment #419273 -
Flags: review?(bugmail)
Comment 7•14 years ago
|
||
While I would like to minimize the amount of time we spend on this cursed widget, I'd like to better understand the actual problem. It sounds like _teardown is getting called at an unexpected time (like after the popup's display is initiated, or some horror where it thinks it is in a wrapper) and for some reason all we need displayed is the server, so we ignore the slaughter of the hapless folders (or there simply are none?). While the horrors of toolbar customization are unlikely to intrude into the filter dialog (making whack-a-mole not too bad), the root cause could perhaps bite more important use-cases. rkent, is the filter dialog doing anything that would generate events on the folders getting displayed?
Reporter | ||
Comment 8•14 years ago
|
||
(In reply to comment #7) > It sounds like > _teardown is getting called at an unexpected time This call to _teardown that is causing the difficulties was added very recently in the regressing bug 520457. I would not say that call is unexpected. The addition of the _teardown call in bug 520457, and the issues that that bug was trying to solve, are really the mystery here. I understand how the news server is supposed to get added to the list. I don't understand bug 520457. > > rkent, is the filter dialog doing anything that would generate events on the > folders getting displayed? Not that I know of, but I am not really an expert on this code. I'm just the guy trying to keep filters from breaking. (And thank you Blake for offerring to do a mozmill test of this).
Assignee | ||
Comment 9•14 years ago
|
||
(In reply to comment #7) > While I would like to minimize the amount of time we spend on this cursed > widget, I'd like to better understand the actual problem. Hopefully the unit tests will help us minimize the amount of time we spend on this widget in the future. :) I also just had the thought that, since this widget has so many special cases that do different things, it might be better to have a few different, more specialized widgets, so that we don't try to do everything in one place. (In reply to comment #8) > > It sounds like _teardown is getting called at an unexpected time > This call to _teardown that is causing the difficulties was added very recently > in the regressing bug 520457. I would not say that call is unexpected. The > addition of the _teardown call in bug 520457, and the issues that that bug was > trying to solve, are really the mystery here. I understand how the news server > is supposed to get added to the list. I don't understand bug 520457. I can help with that. When you have the folder-dropdown in the toolbar, and the toolbar customization window is shown and dismissed, the folder-dropdown's constructor is called again, but none of the children have been deleted, and worse still, the _initialized has been re-set to false. So we call _teardown to get rid of the children we created which shouldn't be there. (I think the key comment in that bug was: "I don't really know why the menu is getting re-created after customization, keeping its children but not its fields, but with this patch we clear out the generated entries") I think we also saw something similar with some menus. There was a partial fix in bug 518586. But, I see in the patch for bug 516353 that we are calling already calling _teardown if we're in a wrapper, so we might be able to just remove the call to _teardown at line 267. (By which I mean that I've removed it in my local copy, recompiled, and it all seems to work. It would be cool if someone tested it under Linux or Windows, though.) And yeah, that looks like it's basically reverting the patch for bug 520457, but I'm okay with that, since the bug that was attempting to fix doesn't seem to crop back up. > (And thank you Blake for offering to do a mozmill test of this). You're very welcome. That widget needs a lot of tests. :) I've actually just moved the test into a different directory (filters -> folder-widget), so that we have a place to add more of them as they crop up. And for my first addition, I'm going to add a test for bug 520457, to make sure that we won't be breaking it if we go for something like rkent's fix. Later, Blake.
Assignee | ||
Comment 10•14 years ago
|
||
And here's the patch which includes a test for bug 520457. Thanks, Blake.
Attachment #419273 -
Attachment is obsolete: true
Attachment #419337 -
Flags: review?(bugmail)
Attachment #419273 -
Flags: review?(bugmail)
Assignee | ||
Comment 11•14 years ago
|
||
This is basically like rkent's original patch, except it never calls _teardown. The tests still pass, though.
Attachment #419338 -
Flags: review?(bugmail)
Reporter | ||
Comment 12•14 years ago
|
||
Comment on attachment 419338 [details] [diff] [review] (Checked in) Something more like rkent's original patch. I don't understand this. You have simply backed out your patch for bug 520457. Won't that cause the original issue to reappear?
Assignee | ||
Comment 13•14 years ago
|
||
I would have thought so, but we've got a test that shows the original issue not reappearing, and I can't replicate it. My hunch is that the fix for bug 516353 also fixes bug 520457, but that's just a hunch. If this patch looks too risky, I would be happy to go with your patch. :) Thanks, Blake.
Comment 14•14 years ago
|
||
Comment on attachment 419052 [details] [diff] [review] A slightly different approach. Is this review request still valid, or do you think the third patch is better?
Assignee | ||
Comment 15•14 years ago
|
||
Comment on attachment 419052 [details] [diff] [review] A slightly different approach. I'm removing sr?bienvenu until asuth weighs in on which patch we want to go with.
Attachment #419052 -
Flags: superreview?(bienvenu)
Comment 16•14 years ago
|
||
I think we should probably fix this in 3.0.x as it is a UI regression and something useful for those who do set up filters on newsgroups (though I expect that is not too often). For those who already have filters on servers, I suspect this will be annoying, therefore setting blocking status to needed.
blocking-thunderbird3.0: ? → needed
Whiteboard: [needs review asuth]
Comment 17•14 years ago
|
||
Andrew, can you take a look at this please?
Updated•14 years ago
|
Whiteboard: [needs review asuth] → [needs review asuth][waiting for mozmill issues to be resolved]
Comment 18•14 years ago
|
||
Comment on attachment 419338 [details] [diff] [review] (Checked in) Something more like rkent's original patch. sure, why not.
Attachment #419338 -
Flags: review?(bugmail) → review+
Comment 19•14 years ago
|
||
Comment on attachment 419052 [details] [diff] [review] A slightly different approach. this was scarier.
Attachment #419052 -
Flags: review?(bugmail) → review-
Comment 20•14 years ago
|
||
Comment on attachment 419337 [details] [diff] [review] (Checked in) More tests, with a little cleanup. This will need to be watched and potentially backed out depending on the whims of the tinderboxes, of course.
Attachment #419337 -
Flags: review?(bugmail) → review+
Assignee | ||
Comment 21•14 years ago
|
||
Comment on attachment 419052 [details] [diff] [review] A slightly different approach. (Obsoleting, to take it out of view.)
Attachment #419052 -
Attachment is obsolete: true
Assignee | ||
Comment 22•14 years ago
|
||
Comment on attachment 419338 [details] [diff] [review] (Checked in) Something more like rkent's original patch. And re-asking for sr, now that we know which patch we want to go with.
Attachment #419338 -
Flags: superreview?(bienvenu)
Updated•14 years ago
|
Attachment #419338 -
Flags: superreview?(bienvenu) → superreview+
Assignee | ||
Updated•14 years ago
|
Whiteboard: [needs review asuth][waiting for mozmill issues to be resolved] → [needs landing, and watching tinderboxes]
Reporter | ||
Comment 23•14 years ago
|
||
If this does not make 3.1.0, hopefully we could add it to 3.1.1 because it is an annoying bug.
Assignee | ||
Comment 24•14 years ago
|
||
Checked in to trunk as <http://hg.mozilla.org/comm-central/rev/ade11dffbf45> and <http://hg.mozilla.org/comm-central/rev/9d3ca83c525c>. But those busted trunk, due to a bug in <http://hg.mozilla.org/comm-central/rev/f82d14d6c31d>, so I also pushed a bustage-fix as <http://hg.mozilla.org/comm-central/rev/46f64bad58e9>. Thanks, Blake.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing, and watching tinderboxes]
Comment 25•14 years ago
|
||
Blake: Can you construct a single patch for the 1.9.2 branch (from all your landings) and attach and request approval please?
blocking-thunderbird3.1: --- → .1+
Flags: in-testsuite+
Target Milestone: --- → Thunderbird 3.2a1
Assignee | ||
Comment 26•14 years ago
|
||
Here you go.
Attachment #453067 -
Flags: approval-thunderbird3.1.1?
Assignee | ||
Updated•14 years ago
|
Attachment #419337 -
Attachment description: More tests, with a little cleanup. → (Checked in) More tests, with a little cleanup.
Assignee | ||
Updated•14 years ago
|
Attachment #419338 -
Attachment description: Something more like rkent's original patch. → (Checked in) Something more like rkent's original patch.
Updated•14 years ago
|
Attachment #453067 -
Flags: approval-thunderbird3.1.1? → approval-thunderbird3.1.1+
Assignee | ||
Comment 27•14 years ago
|
||
Checked in as http://hg.mozilla.org/releases/comm-1.9.2/rev/3d2b8eca5482
Assignee | ||
Updated•14 years ago
|
Attachment #453067 -
Attachment description: All the patches, for 1.9.2. → (Checked in) All the patches, for 1.9.2.
Comment 28•14 years ago
|
||
In 3.1.1 RC build 1 (win32 build) Bug 520457 appears again (using -safe-mode).
Updated•14 years ago
|
status-thunderbird3.1:
--- → .1-fixed
Comment 29•14 years ago
|
||
Given the fact this is fixed in 3.1.x and we're now offering updates to 3.1.x I don't think this is worth back porting to the 3.0.x series (we're also not seeing any complaints about it afaik).
blocking-thunderbird3.0: needed → ---
Comment 30•11 years ago
|
||
Attachment https://bugzilla.mozilla.org/attachment.cgi?id=419338 seems to undo the change done in bug 520457 so that the bug happens again. I can see it on TB 23, Linux. And the test that should make this visible seems to be broken. To not reopen this old bug, I propose handling this all in bug 871266.
Blocks: 871266
Comment 31•11 years ago
|
||
The test I am fixing in bug 871266 is from comment 10 here. Bwinton says he can't see reappearing of bug 520457 with it. Maybe it is because the test is broken (at least today). My proposal is that the _teardown() should be done, but if the popup needs to be populated, regenerate it immediately, not wait until it is shown again. This will also be needed for features (in other bugs) that want to ask the widget which folder is currently selected. Today, if a folder changes, the widget gets an event, it tears itself down, but does not regenerate.
Flags: needinfo?(bwinton)
Assignee | ||
Comment 32•11 years ago
|
||
I'm not sure what info you need from me here. It looks like you should just move ahead with bug 871266.
Flags: needinfo?(bwinton)
You need to log in
before you can comment on or make changes to this bug.
Description
•