Message Filters list not updated when already open

RESOLVED FIXED in Thunderbird 18.0

Status

MailNews Core
Filters
RESOLVED FIXED
8 years ago
6 months ago

People

(Reporter: Malte Forkel, Assigned: aceman)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs, {ux-userfeedback})

Trunk
Thunderbird 18.0
ux-userfeedback
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

1.39 KB, patch
aceman
: review+
Details | Diff | Splinter Review
(Reporter)

Description

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

8 years ago
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
Status: UNCONFIRMED → NEW
Component: General → Filters
Ever confirmed: true
Product: Thunderbird → MailNews Core
QA Contact: general → filters
Version: unspecified → Trunk
(Assignee)

Comment 2

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

Updated

6 years ago
Duplicate of this bug: 561601
(Assignee)

Updated

6 years ago
Depends on: 363982
(Assignee)

Updated

5 years ago
Duplicate of this bug: 541322
(Assignee)

Comment 5

5 years ago
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)
Assignee: nobody → acelists
Severity: minor → normal
Keywords: ux-userfeedback
OS: Windows 2000 → All
Hardware: x86 → All
(Assignee)

Comment 6

5 years ago
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.
Attachment #649588 - Flags: feedback?(kent)
(Assignee)

Comment 7

5 years ago
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.
Status: NEW → ASSIGNED
(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
(Assignee)

Comment 9

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

Comment 10

5 years ago
Created attachment 649609 [details] [diff] [review]
patch v2
Attachment #649588 - Attachment is obsolete: true
Attachment #649588 - Flags: feedback?(kent)
Attachment #649609 - Flags: review?(jh)
Attachment #649609 - Flags: feedback?(kent)
(Assignee)

Comment 11

5 years ago
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().
Attachment #649609 - Flags: ui-review?(bwinton)
Attachment #649609 - Flags: feedback?(axelg)
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.
Attachment #649609 - Flags: review?(jh)
s/change/chance/
(Assignee)

Comment 14

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

Comment 16

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

5 years ago
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.
Attachment #649609 - Flags: feedback?(kent) → feedback+

Updated

5 years ago
Blocks: 360488
Comment on attachment 649609 [details] [diff] [review]
patch v2

Much better.  Thanks!

ui-r=me.
Attachment #649609 - Flags: ui-review?(bwinton) → ui-review+
(Assignee)

Comment 19

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

Comment 20

5 years ago
This depends on bug 780473 as that changes the arguments to rebuildFilterList.
Depends on: 780473
(Assignee)

Updated

5 years ago
Blocks: 363982
No longer depends on: 363982
Comment on attachment 649609 [details] [diff] [review]
patch v2

works fine for me. please land if you can! thanks :)
Attachment #649609 - Flags: feedback?(axelg) → feedback+
(Assignee)

Comment 22

5 years ago
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.
Depends on: 783110
(Assignee)

Comment 23

5 years ago
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 :)
Attachment #649609 - Attachment is obsolete: true
Attachment #662608 - Flags: review?(mkmelin+mozilla)
(Assignee)

Comment 24

5 years ago
: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

5 years ago
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
Attachment #662608 - Flags: review?(mkmelin+mozilla) → review+
(Assignee)

Comment 26

5 years ago
Created attachment 666025 [details] [diff] [review]
patch v4

Thanks.
Attachment #662608 - Attachment is obsolete: true
Attachment #666025 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/baf24d731142
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite-
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 18.0
Duplicate of this bug: 549542
(Assignee)

Updated

4 years ago
Blocks: 862739
(Assignee)

Updated

4 years ago
Duplicate of this bug: 910663
(Assignee)

Updated

6 months ago
Depends on: 1328855
You need to log in before you can comment on or make changes to this bug.