be smarter about when to reset the search box after a New/Edit filter operation in the filter list editor

RESOLVED FIXED in Thunderbird 18.0

Status

MailNews Core
Filters
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: aceman, Assigned: aceman)

Tracking

({polish, ux-interruption})

Trunk
Thunderbird 18.0
polish, ux-interruption
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

5.87 KB, patch
realRaven
: feedback+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
Bug 450302 added the searching of filters inside the filter list dialog.
In bug 780473 I try to optimize the speed of rebuildFilterList function. It also allowed onFindFilter function to work without building a full list of filters and then removing lines from it. It works invisibly on the filterlist array. I think it will be possible to use this to determine whether after a New Filter or Edit Filter operation the searchbox must me reset to reveal the new filter. The onFindFilter function can be queried if the new/edited filter would be shown under current search condition (value of searchbox). Only reset the box if the filter would be hidden.
The new onFindFilter function is cheap, even with 10000 filters it returns the result instantaneously. So there should be no perf loss from this change. The point of the bug is less user confusion and less resetting of his search term.

I can make the patch if bug 780473 lands.
(Assignee)

Comment 1

5 years ago
There is also a possibility taken from bug 450302 comment 50: Leave the searchterm (box) intact but still show the new/editer filter even if it does not match now (basically do not redraw the list). The list will be redrawn at next operation that requires it (like move/delete) and the new/edited filter will be hidden then.
(Assignee)

Comment 2

5 years ago
Axel, Bwinton, what do you think?
The second possibility (in comment 1) seems a little strange, since then we would have the filter not showing what's actually being filtered, and things will disappear from people's view seemingly randomly.

I could see how option one makes sense, though.  Axel, what do you think?
If you mean Only reset the box if the currently  edited / new filter would be hidden, that would make perfect sense to me. It is a convenience function, for advanced users, so it doesn't have to be consistent. As long as search term and list results are always consistent, that is okay with me.
(Assignee)

Comment 5

5 years ago
Yes, that is the intention. I'll make a patch soon.
(Assignee)

Comment 6

5 years ago
Created attachment 657354 [details] [diff] [review]
patch
Attachment #657354 - Flags: ui-review?(bwinton)
Attachment #657354 - Flags: review?(mkmelin+mozilla)
Attachment #657354 - Flags: feedback?(axelg)
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
Keywords: polish, ux-interruption

Comment 7

5 years ago
Comment on attachment 657354 [details] [diff] [review]
patch

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

Looks good to me. r=mkmelin

::: mail/base/content/FilterListDialog.js
@@ +877,5 @@
>  
>    // Rematch everything in the list, remove what doesn't match the search box.
>    let rows = gCurrentFilterList.filterCount;
>    let matchingFilterList = [];
> +  let filter;

move this declaration inside the for loop as it's only used there
Attachment #657354 - Flags: review?(mkmelin+mozilla) → review+
(Assignee)

Comment 8

5 years ago
Created attachment 659837 [details] [diff] [review]
patch v2

FYI Magnus, declaring the variable before the loop is faster than inside it. But I have benchmarked the onFindFilter function and it does not matter much in this case (the slowdown is negligible).
Attachment #657354 - Attachment is obsolete: true
Attachment #657354 - Flags: ui-review?(bwinton)
Attachment #657354 - Flags: feedback?(axelg)
Attachment #659837 - Flags: ui-review?(bwinton)
Attachment #659837 - Flags: feedback?(axelg)
Comment on attachment 659837 [details] [diff] [review]
patch v2

Seems good, thanks!  ui-r=me!

Later,
Blake.
Attachment #659837 - Flags: ui-review?(bwinton) → ui-review+
(Assignee)

Comment 10

5 years ago
Axel, can you comment here even if you can't grant f+ yet?
(Assignee)

Updated

5 years ago
Blocks: 543419
(Assignee)

Comment 11

5 years ago
Created attachment 661605 [details] [diff] [review]
patch v3

Unbitrotted patch.
Attachment #659837 - Attachment is obsolete: true
Attachment #659837 - Flags: feedback?(axelg)
Attachment #661605 - Flags: feedback?(axelg)
Comment on attachment 661605 [details] [diff] [review]
patch v3

this works really well, very slick and just the right amount of changing if the user chooses to rename / name outside of the filter. I especially like the fact that the search box stays if I only tweak some conditions without changing the filter name. Please land!
Attachment #661605 - Flags: feedback?(axelg) → feedback+
(Assignee)

Comment 13

5 years ago
Thanks:)
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/0629a3f8aad6
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite-
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 18.0
You need to log in before you can comment on or make changes to this bug.