596885 - [UI] Move focus to newly created filter; select previous available filter after deletion

Status: VERIFIED FIXED

(MailNews Core :: Filters, defect)

Description

[Frank Spade]

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]]

I couldn't find a duplicate.

Comment 2

[:aceman]

Attachment: 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) (:☕️)]

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]

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]

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]

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]

Why? Var is used everywhere around that code... What is the difference?

Comment 8

[David :Bienvenu]

(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]

Thanks David, I can do that.

Bwinton, can you answer my questions so I can continue?

Comment 10

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

(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]

OK, I'll try it.

Comment 12

[:aceman]

Attachment: 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]

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]

Attachment: v2, patch implementing both features

Comment 15

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

Comment on attachment 580496 [details] [diff] [review]
v2, patch implementing both features

Looks good to me!

Thanks,
Blake.

Comment 16

[:aceman]

Comment on attachment 580496 [details] [diff] [review]
v2, patch implementing both features

Thanks, now to the code review:)

Comment 17

[David :Bienvenu]

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)]

Checked in: http://hg.mozilla.org/comm-central/rev/0eef83152b2d