Closed
Bug 783110
Opened 12 years ago
Closed 12 years ago
be smarter about when to reset the search box after a New/Edit filter operation in the filter list editor
Categories
(MailNews Core :: Filters, defect)
MailNews Core
Filters
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 18.0
People
(Reporter: aceman, Assigned: aceman)
References
Details
(Keywords: polish, ux-interruption)
Attachments
(1 file, 2 obsolete files)
5.87 KB,
patch
|
realRaven
:
feedback+
|
Details | Diff | Splinter Review |
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.
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.
Comment 3•12 years ago
|
||
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?
Comment 4•12 years ago
|
||
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.
Attachment #657354 -
Flags: ui-review?(bwinton)
Attachment #657354 -
Flags: review?(mkmelin+mozilla)
Attachment #657354 -
Flags: feedback?(axelg)
Status: NEW → ASSIGNED
Keywords: polish,
ux-interruption
Comment 7•12 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+
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 9•12 years ago
|
||
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•12 years ago
|
||
Axel, can you comment here even if you can't grant f+ yet?
Assignee | ||
Comment 11•12 years ago
|
||
Unbitrotted patch.
Attachment #659837 -
Attachment is obsolete: true
Attachment #659837 -
Flags: feedback?(axelg)
Attachment #661605 -
Flags: feedback?(axelg)
Comment 12•12 years ago
|
||
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+
Comment 14•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 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.
Description
•