543419 - Message Filters list not updated when already open

Status: RESOLVED FIXED

(MailNews Core :: Filters, defect)

Description

[Malte Forkel]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; de; rv:1.9.1.7) Gecko/20091221 Firefox/3.5.7
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; de; rv:1.9.1.7) Gecko/20100111 Lightning/1.0b1 Thunderbird/3.0.1

When the Message Filter window is open from creating a filter, it is not updated to show a filter created via Create Filter from Message. 

Reproducible: Always

Steps to Reproduce:
1. Create a filter, e.g. via Create Filter from Message
2. Window Message Filters opens automatically
3. Create another filter, e.g. via Create Filter from Message
Actual Results:  
Second filter created is not shown in list Message Filters

Expected Results:  
Second filter created is shown in list Message Filters

The second filter is shown once on closes and reopens the Message Filters window

Comment 1

[Kent James (:rkent)]

I don't know if this is TB-specific or not, but to find it in the future it needs to be in Mailnews Core/Filters

Comment 2

[:aceman]

I can confirm this is still in TB10, Win XP. If the Filters dialog is minimized, then it is restored and focused after the filter is created, but it doesn't contain the new filter.

Comment 5

[:aceman]

I get this in the Error console:
Error: TypeError: desiredWindow.refresh is not a function
Source File: chrome://messenger/content/mailWindowOverlay.js
Line: 3145

Error: An error occurred executing the cmd_createFilterFromMenu command: TypeError: desiredWindow.refresh is not a function
Source File: chrome://global/content/globalOverlay.js
Line: 95

Depends on which Create filter from menu item I use (there is also one in the context menu of From and Subject in a message view)

Comment 6

[:aceman]

Attachment: WIP patch

This seems to do it. There is infrastructure to open/refresh the filter list dialog after a new filter is created. But the filter list had no function of "refresh" defined so it silently failed (only with error in Error console).
So add the function and do what is needed.

Comment 7

[:aceman]

I am not sure if this affects seamonkey too and if they have a equivalent of OpenOrFocusWindow(args, windowType, chromeURL) function that calls window.refresh() on the filter list dialog.

Comment 8

[Jens Hatlak (:InvisibleSmiley)]

(In reply to :aceman from comment #7)
> I am not sure if this affects seamonkey too and if they have a equivalent of
> OpenOrFocusWindow(args, windowType, chromeURL) function that calls
> window.refresh() on the filter list dialog.

http://mxr.mozilla.org/comm-central/source/suite/mailnews/mailWindowOverlay.js#2996

Comment 9

[:aceman]

Thanks, so it seems it does not have anything differing from TB and my solution should work as is. I'll produce a proper patch soon.

Comment 10

[:aceman]

Attachment: patch v2

Comment 11

[:aceman]

Comment on attachment 649609 [details] [diff] [review]
patch v2

Also reset the search box on refresh, in the same way it is must be done in onNewFilter().

Comment 12

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 649609 [details] [diff] [review]
patch v2

Review of attachment 649609 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not a TB reviewer and not an official SM MailNews reviewer either. Since the file you're changing lives in TB land (is not shared), you'll need to ask an official TB reviewer. If you choose to adapt the SM version of the file, too, you can ask Karsten (:Mnyromyr) for review of that part. Note that we don't have the filter search box yet.

::: mail/base/content/FilterListDialog.js
@@ +107,5 @@
>  
> +function refresh()
> +{
> +    // This is called from OpenOrFocusWindow() if the dialog is already open.
> +    // Just redraw the list.

I'd probably put this up as a C-style function comment (like with the next function).

@@ +112,5 @@
> +    // New filters could have been created by operations outside the dialog.
> +    rebuildFilterList(gCurrentFilterList);
> +
> +    // As we really don't know what has changed, clear the search box
> +    // so that the changed/added filters are surely visible.

Well, this might come a little surprising to users - after all, they entered something there and you'll clear it without giving them the change to copy it to the clipboard or something.

Comment 13

[Jens Hatlak (:InvisibleSmiley)]

s/change/chance/

Comment 14

[:aceman]

Oh sorry, I overlooked this is TB only. Most of the filter files are in /mailnews so I automatically CC some SM guys...

Maybe you need to do same fix in SM version of the file.

We clear the searchbox on many ocassions (it is a new feature for several days only) so I just keep consistency. Surely we need to think about optimizing this resetting, but not in this bug.

Comment 15

[Jens Hatlak (:InvisibleSmiley)]

(In reply to :aceman from comment #14)
> Oh sorry, I overlooked this is TB only.

Neither the issue nor this bug is, only your proposed fix (no offense!).

Basically everything under mail/ is TB-only, everything under suite/ is SM-only, and the rest of c-c has a high probability to be shared.

> Most of the filter files are in
> /mailnews so I automatically CC some SM guys...

Much appreciated.

> Maybe you need to do same fix in SM version of the file.

Well, eventually someone should, yes, and it might be me. Or it could be you, right here. :-)

Comment 16

[:aceman]

I can't do it as SM has a completely different implementation of the list so I don't know what your 'rebuild' command is.
Please do the SM version.

Comment 17

[Kent James (:rkent)]

Comment on attachment 649609 [details] [diff] [review]
patch v2

So I tried this briefly, and it seems to work, so I am fine with it.

The question I had though is the interaction with attempts to improve the performance of the filter list dialog. The more important problem is to make sure that we have a usable interface for people with lots of filters, which frankly we have not had ever since the de-rdf work that changed all of this.

Does it make it significantly harder to support faster editing of the filter list dialog if we have to maintain the ability to add a filter separately while it is open? I would rather WONTFIX this bug (perhaps just overwriting the filter list with the list in the dialog, losing the added message filter) since this is an edge case, if it makes it more difficult to support rapid editing of a long filter list.

But I could not generate a failure in my brief tests, so perhaps this is not an issue.

Comment 18

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

Comment on attachment 649609 [details] [diff] [review]
patch v2

Much better.  Thanks!

ui-r=me.

Comment 19

[:aceman]

(In reply to Kent James (:rkent) from comment #17)
> The question I had though is the interaction with attempts to improve the
> performance of the filter list dialog. The more important problem is to make
> sure that we have a usable interface for people with lots of filters, which
> frankly we have not had ever since the de-rdf work that changed all of this.
> 
> Does it make it significantly harder to support faster editing of the filter
> list dialog if we have to maintain the ability to add a filter separately
> while it is open?
> I would rather WONTFIX this bug (perhaps just overwriting
> the filter list with the list in the dialog, losing the added message
> filter) since this is an edge case, if it makes it more difficult to support
> rapid editing of a long filter list.

I have not seen any bugs/proposals on how to speed the dialog up, except my bug 780473. So with the absence of any proposals, I can't answer that and I do not see any problem implementing the bug here. Having the refresh function and comments in the code will make future optimizers be aware of the out of dialog filter additions and they will have to think about it.

Comment 20

[:aceman]

This depends on bug 780473 as that changes the arguments to rebuildFilterList.

Comment 21

[Axel Grude [:realRaven]]

Comment on attachment 649609 [details] [diff] [review]
patch v2

works fine for me. please land if you can! thanks :)

Comment 22

[:aceman]

Congrats to Axel, for getting the proper bugzilla rights to give f+:)

Anyway, I'd wait with this until bug 783110 is landed, from which I'll use the new searchbox resetting function. Even when we reset the searchbox here undonditionally.

Comment 23

[:aceman]

Attachment: patch v3

So this is an uptodate version. And we need to reset search before list refresh in the new flow :)

Comment 24

[:aceman]

:InvisibleSmiley, as there is no rebuildFilterList() is the SM version I have no idea what to do there. You'll need to produce the SM version yourself.

Comment 25

[Magnus Melin [:mkmelin]]

Comment on attachment 662608 [details] [diff] [review]
patch v3

Review of attachment 662608 [details] [diff] [review]:
-----------------------------------------------------------------

Please use 2 space indention for new code (though yes the file is a bit mixed style).
Other than that, looks good! r=mkmelin

Comment 26

[:aceman]

Attachment: patch v4

Thanks.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/baf24d731142