534448 - Folder dropdown for newsgroup filters no longer shows server

Status: RESOLVED FIXED

(MailNews Core :: Filters, defect)

Description

[Kent James (:rkent)]

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.

Comment 1

[Kent James (:rkent)]

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.

Comment 2

[Kent James (:rkent)]

Attachment: disable _teardown call when we need headlabels

Comment 3

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

Attachment: A slightly different approach.

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.

Comment 4

[Kent James (:rkent)]

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.

Comment 5

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

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.

Comment 6

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

Attachment: A sort of WIP-ish unit test.

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.

Comment 7

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

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?

Comment 8

[Kent James (:rkent)]

(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).

Comment 9

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

(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.

Comment 10

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

Attachment: (Checked in) More tests, with a little cleanup.

And here's the patch which includes a test for bug 520457.

Thanks,
Blake.

Comment 11

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

Attachment: (Checked in) Something more like rkent's original patch.

This is basically like rkent's original patch, except it never calls _teardown.

The tests still pass, though.

Comment 12

[Kent James (:rkent)]

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?

Comment 13

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

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

[David :Bienvenu]

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?

Comment 15

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

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.

Comment 16

[Mark Banner (:standard8)]

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.

Comment 17

[Mark Banner (:standard8)]

Andrew, can you take a look at this please?

Comment 18

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

Comment on attachment 419338 [details] [diff] [review]
(Checked in) Something more like rkent's original patch.

sure, why not.

Comment 19

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

Comment on attachment 419052 [details] [diff] [review]
A slightly different approach.

this was scarier.

Comment 20

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

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.

Comment 21

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

Comment on attachment 419052 [details] [diff] [review]
A slightly different approach.

(Obsoleting, to take it out of view.)

Comment 22

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

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.

Comment 23

[Kent James (:rkent)]

If this does not make 3.1.0, hopefully we could add it to 3.1.1 because it is an annoying bug.

Comment 24

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

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.

Comment 25

[Mark Banner (:standard8)]

Blake: Can you construct a single patch for the 1.9.2 branch (from all your landings) and attach and request approval please?

Comment 26

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

Attachment: (Checked in) All the patches, for 1.9.2.

Here you go.

Comment 27

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

Checked in as http://hg.mozilla.org/releases/comm-1.9.2/rev/3d2b8eca5482

Comment 28

[Alex Ihrig [:Thunderbird_Mail_DE]]

In 3.1.1 RC build 1 (win32 build) Bug 520457 appears again (using -safe-mode).

Comment 29

[Mark Banner (:standard8)]

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).

Comment 30

[:aceman]

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.

Comment 31

[:aceman]

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.

Comment 32

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

I'm not sure what info you need from me here.  It looks like you should just move ahead with bug 871266.