Last Comment Bug 543419 - Message Filters list not updated when already open
: Message Filters list not updated when already open
Status: RESOLVED FIXED
: ux-userfeedback
Product: MailNews Core
Classification: Components
Component: Filters (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Thunderbird 18.0
Assigned To: :aceman
:
Mentors:
: 541322 549542 561601 910663 (view as bug list)
Depends on: 780473 783110
Blocks: TB2SM 862739 363982
  Show dependency treegraph
 
Reported: 2010-02-01 01:09 PST by Malte Forkel
Modified: 2013-09-04 14:20 PDT (History)
14 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
WIP patch (580 bytes, patch)
2012-08-07 03:41 PDT, :aceman
no flags Details | Diff | Splinter Review
patch v2 (1.39 KB, patch)
2012-08-07 05:50 PDT, :aceman
bwinton: ui‑review+
rkent: feedback+
axelg: feedback+
Details | Diff | Splinter Review
patch v3 (1.40 KB, patch)
2012-09-19 10:32 PDT, :aceman
mkmelin+mozilla: review+
Details | Diff | Splinter Review
patch v4 (1.39 KB, patch)
2012-09-28 12:34 PDT, :aceman
acelists: review+
Details | Diff | Splinter Review

Description Malte Forkel 2010-02-01 01:09:49 PST
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) 2010-02-01 10:32:56 PST
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 2011-10-25 08:06:48 PDT
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 3 :aceman 2011-10-25 08:06:56 PDT
*** Bug 561601 has been marked as a duplicate of this bug. ***
Comment 4 :aceman 2012-08-07 02:49:41 PDT
*** Bug 541322 has been marked as a duplicate of this bug. ***
Comment 5 :aceman 2012-08-07 03:36:40 PDT
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 2012-08-07 03:41:52 PDT
Created attachment 649588 [details] [diff] [review]
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 2012-08-07 03:44:22 PDT
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) 2012-08-07 04:14:05 PDT
(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 2012-08-07 04:54:17 PDT
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 2012-08-07 05:50:57 PDT
Created attachment 649609 [details] [diff] [review]
patch v2
Comment 11 :aceman 2012-08-07 05:52:10 PDT
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) 2012-08-07 06:02:44 PDT
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) 2012-08-07 06:04:46 PDT
s/change/chance/
Comment 14 :aceman 2012-08-07 06:10:51 PDT
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) 2012-08-07 06:19:56 PDT
(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 2012-08-07 06:27:06 PDT
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) 2012-08-07 09:56:44 PDT
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) (:☕️) 2012-08-08 12:27:04 PDT
Comment on attachment 649609 [details] [diff] [review]
patch v2

Much better.  Thanks!

ui-r=me.
Comment 19 :aceman 2012-08-13 01:51:30 PDT
(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 2012-08-28 14:54:50 PDT
This depends on bug 780473 as that changes the arguments to rebuildFilterList.
Comment 21 Axel Grude [:realRaven] 2012-09-16 05:29:11 PDT
Comment on attachment 649609 [details] [diff] [review]
patch v2

works fine for me. please land if you can! thanks :)
Comment 22 :aceman 2012-09-16 05:45:14 PDT
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 2012-09-19 10:32:13 PDT
Created attachment 662608 [details] [diff] [review]
patch v3

So this is an uptodate version. And we need to reset search before list refresh in the new flow :)
Comment 24 :aceman 2012-09-19 10:40:35 PDT
: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 2012-09-28 12:25:52 PDT
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 2012-09-28 12:34:31 PDT
Created attachment 666025 [details] [diff] [review]
patch v4

Thanks.
Comment 27 Ryan VanderMeulen [:RyanVM] 2012-09-29 06:35:00 PDT
https://hg.mozilla.org/comm-central/rev/baf24d731142
Comment 28 Wayne Mery (:wsmwk, NI for questions) 2013-02-26 04:05:10 PST
*** Bug 549542 has been marked as a duplicate of this bug. ***
Comment 29 :aceman 2013-09-04 14:20:57 PDT
*** Bug 910663 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.