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

Status: RESOLVED FIXED

(MailNews Core :: Filters, defect)

Description

[:aceman]

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.

Comment 1

[:aceman]

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 2

[:aceman]

Axel, Bwinton, what do you think?

Comment 3

[Blake Winton (:bwinton) (:☕️)]

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

[Axel Grude [:realRaven]]

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.

Comment 5

[:aceman]

Yes, that is the intention. I'll make a patch soon.

Comment 6

[:aceman]

Attachment: patch

Comment 7

[Magnus Melin [:mkmelin]]

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

Comment 8

[:aceman]

Attachment: 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).

Comment 9

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 659837 [details] [diff] [review]
patch v2

Seems good, thanks!  ui-r=me!

Later,
Blake.

Comment 10

[:aceman]

Axel, can you comment here even if you can't grant f+ yet?

Comment 11

[:aceman]

Attachment: patch v3

Unbitrotted patch.

Comment 12

[Axel Grude [:realRaven]]

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!

Comment 13

[:aceman]

Thanks:)

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/0629a3f8aad6