Last Comment Bug 783110 - be smarter about when to reset the search box after a New/Edit filter operation in the filter list editor
: be smarter about when to reset the search box after a New/Edit filter operati...
Status: RESOLVED FIXED
: polish, ux-interruption
Product: MailNews Core
Classification: Components
Component: Filters (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Thunderbird 18.0
Assigned To: :aceman
:
:
Mentors:
Depends on: 450302 780473
Blocks: 543419
  Show dependency treegraph
 
Reported: 2012-08-15 15:29 PDT by :aceman
Modified: 2012-09-17 18:04 PDT (History)
6 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (5.94 KB, patch)
2012-08-31 10:43 PDT, :aceman
mkmelin+mozilla: review+
Details | Diff | Splinter Review
patch v2 (5.90 KB, patch)
2012-09-10 13:29 PDT, :aceman
bwinton: ui‑review+
Details | Diff | Splinter Review
patch v3 (5.87 KB, patch)
2012-09-16 08:52 PDT, :aceman
axelg: feedback+
Details | Diff | Splinter Review

Description :aceman 2012-08-15 15:29:43 PDT
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 2012-08-16 01:59:05 PDT
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 2012-08-27 06:00:42 PDT
Axel, Bwinton, what do you think?
Comment 3 Blake Winton (:bwinton) (:☕️) 2012-08-27 06:41:45 PDT
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] 2012-08-30 15:04:08 PDT
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 2012-08-30 15:14:38 PDT
Yes, that is the intention. I'll make a patch soon.
Comment 6 :aceman 2012-08-31 10:43:35 PDT
Created attachment 657354 [details] [diff] [review]
patch
Comment 7 Magnus Melin 2012-09-05 13:02:25 PDT
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 2012-09-10 13:29:26 PDT
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).
Comment 9 Blake Winton (:bwinton) (:☕️) 2012-09-11 17:22:08 PDT
Comment on attachment 659837 [details] [diff] [review]
patch v2

Seems good, thanks!  ui-r=me!

Later,
Blake.
Comment 10 :aceman 2012-09-12 00:03:43 PDT
Axel, can you comment here even if you can't grant f+ yet?
Comment 11 :aceman 2012-09-16 08:52:07 PDT
Created attachment 661605 [details] [diff] [review]
patch v3

Unbitrotted patch.
Comment 12 Axel Grude [:realRaven] 2012-09-17 13:43:24 PDT
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 2012-09-17 13:53:10 PDT
Thanks:)
Comment 14 Ryan VanderMeulen [:RyanVM] 2012-09-17 18:04:50 PDT
https://hg.mozilla.org/comm-central/rev/0629a3f8aad6

Note You need to log in before you can comment on or make changes to this bug.