Last Comment Bug 596885 - [UI] Move focus to newly created filter; select previous available filter after deletion
: [UI] Move focus to newly created filter; select previous available filter aft...
Status: VERIFIED FIXED
[filter-mgmt]
:
Product: MailNews Core
Classification: Components
Component: Filters (show other bugs)
: 1.9.2 Branch
: All All
: -- minor (vote)
: Thunderbird 12.0
Assigned To: :aceman
:
Mentors:
: 620391 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-09-15 23:31 PDT by Frank Spade
Modified: 2012-06-28 04:45 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (987 bytes, patch)
2011-11-13 07:08 PST, :aceman
bwinton: ui‑review-
Details | Diff | Review
v2, patch implementing both features (4.10 KB, patch)
2011-12-08 14:22 PST, :aceman
no flags Details | Diff | Review
v2, patch implementing both features (4.10 KB, patch)
2011-12-09 12:07 PST, :aceman
mozilla: review+
bwinton: ui‑review+
Details | Diff | Review

Description Frank Spade 2010-09-15 23:31:51 PDT
User-Agent:       Mozilla/4.0 (compatible; MSIE 8.0; Windows NT 6.1; WOW64; Trident/4.0; GTB6.5; SLCC2; .NET CLR 2.0.50727; .NET CLR 3.5.30729; .NET CLR 3.0.30729; Media Center PC 6.0; MDDC; AskTB5.6)
Build Identifier: Thunderbird/3.1.3

It would be a nice little improvement, if the focus would switch to the newly created filter after adding a filter, so one could hit apply right away and not have to switch focus first.

Reproducible: Always

Steps to Reproduce:
1. Create a new filter rule
2. Look where the focus rests
3. Naturally one would want to apply that rule right away, but has to move the focus first.
Actual Results:  
Focus is still on the last selected filter rule.

Expected Results:  
Focus should rest on the newly created filter rule.

This is obviously a minor enhancement, but helpful when you are in the process of creating and applying filter rules.
Comment 1 Ludovic Hirlimann [:Usul] 2010-09-20 00:28:27 PDT
I couldn't find a duplicate.
Comment 2 :aceman 2011-11-13 07:08:02 PST
Created attachment 574155 [details] [diff] [review]
patch

I created a patch, here are my notes:
When a new filter is added, it is created at the top of the list.
The current selection is cleared and the new filter is selected.
It does not have the blue background, just the dotted outline. I believe that is part of bug 642810.
It calls full rebuildFilterList twice. Maybe that is not the most efficient way. It could probably be done with one call and then manage the selection similarly to the last commands of rebuildFilterList (including updateButtons). But I think my solution is more robust w.r.t. future changes. Or it could be done with adding a new argument to rebuildFilterList, to skip rebuilding the list elements, just manage the selection (focus).
Please review.
Comment 3 Blake Winton (:bwinton) (:☕️) 2011-11-15 14:13:37 PST
Comment on attachment 574155 [details] [diff] [review]
patch

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

I like this, but...

It would be nice if we could go to the previously selected filter when we deleted a filter, too, and since you're mucking about in this code already...  ;)

Thanks,
Blake.
Comment 4 :aceman 2011-11-15 14:24:55 PST
Previously selected filter? Delete will delete all selected filters. What was selected previously? I do not understand.
And what does it have to do with this bug?:)
I am mucking about there but I can only do so much... But I can look into what you are saying if you explain it more.
Comment 5 :aceman 2011-11-15 23:13:57 PST
Do you mean after delete select the filter just before the selection that was removed?
A
B
C (selected)
D
E (selected)

After Delete:

A
B (selected)
D

?
That is something I should be able to do.
Comment 6 David :Bienvenu 2011-11-22 12:03:26 PST
Comment on attachment 574155 [details] [diff] [review]
patch

clearing review request based on ui-review- (but please use let instead of var in your next patch).
Comment 7 :aceman 2011-11-22 12:11:44 PST
Why? Var is used everywhere around that code... What is the difference?
Comment 8 David :Bienvenu 2011-11-22 12:44:22 PST
(In reply to :aceman from comment #7)
> Why? Var is used everywhere around that code... What is the difference?

essentially, let is a local variable, var is global. let is newish, which is why old code uses var.
Comment 9 :aceman 2011-12-01 08:19:16 PST
Thanks David, I can do that.

Bwinton, can you answer my questions so I can continue?
Comment 10 Blake Winton (:bwinton) (:☕️) 2011-12-08 06:56:38 PST
(In reply to :aceman from comment #5)
> Do you mean after delete select the filter just before the selection that
> was removed?
> A
> B
> C (selected)
> D
> E (selected)
> 
> After Delete:
> 
> A
> B (selected)
> D
> 
> ?
> That is something I should be able to do.

Sorry for the delay, yes, that's exactly what I meant.

Thanks,
Blake.
Comment 11 :aceman 2011-12-08 13:59:10 PST
OK, I'll try it.
Comment 12 :aceman 2011-12-08 14:22:52 PST
Created attachment 580195 [details] [diff] [review]
v2, patch implementing both features

I also cleaned up most of "onDeleteFilter" (trailing spaces, var->let).
I also did the optimization I mentioned in comment 2 (split out view/scroll position without rebuilding the whole index list), now that there are at least 2 consumers (maybe others could be found, where full rebuildFilterList is not necessary)
Comment 13 :aceman 2011-12-08 23:53:59 PST
Comment on attachment 580195 [details] [diff] [review]
v2, patch implementing both features

Sorry, not ready for review, I need to check if the stuff with firstVisibleRowIndex is correct. It seems we actually want to reset it after New or Delete, not preserve it.
Comment 14 :aceman 2011-12-09 12:07:52 PST
Created attachment 580496 [details] [diff] [review]
v2, patch implementing both features
Comment 15 Blake Winton (:bwinton) (:☕️) 2011-12-13 11:56:10 PST
Comment on attachment 580496 [details] [diff] [review]
v2, patch implementing both features

Looks good to me!

Thanks,
Blake.
Comment 16 :aceman 2011-12-13 12:02:30 PST
Comment on attachment 580496 [details] [diff] [review]
v2, patch implementing both features

Thanks, now to the code review:)
Comment 17 David :Bienvenu 2012-01-05 15:02:24 PST
Comment on attachment 580496 [details] [diff] [review]
v2, patch implementing both features

sorry for the delay, thx for the patch!
Comment 18 Mark Banner (:standard8) 2012-01-09 00:53:04 PST
Checked in: http://hg.mozilla.org/comm-central/rev/0eef83152b2d
Comment 19 :aceman 2012-06-28 04:45:20 PDT
*** Bug 620391 has been marked as a duplicate of this bug. ***

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