Closed Bug 783110 Opened 9 years ago Closed 9 years ago
be smarter about when to reset the search box after a New/Edit filter operation in the filter list editor
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.
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.
Yes, that is the intention. I'll make a patch soon.
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).
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+
Axel, can you comment here even if you can't grant f+ yet?
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+
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 18.0
You need to log in before you can comment on or make changes to this bug.