Closed Bug 596885 Opened 9 years ago Closed 8 years ago

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

Categories

(MailNews Core :: Filters, defect, minor)

1.9.2 Branch
defect
Not set
minor

Tracking

(Not tracked)

VERIFIED FIXED
Thunderbird 12.0

People

(Reporter: frank, Assigned: aceman)

References

Details

(Whiteboard: [filter-mgmt])

Attachments

(1 file, 2 obsolete files)

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.
I couldn't find a duplicate.
Status: UNCONFIRMED → NEW
Component: General → Filters
Ever confirmed: true
Product: Thunderbird → MailNews Core
QA Contact: general → filters
Severity: enhancement → minor
Summary: Move focus to newly created filter → Move focus to newly created filter [UI]
Attached patch patch (obsolete) — Splinter Review
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.
Assignee: nobody → acelists
Status: NEW → ASSIGNED
Attachment #574155 - Flags: ui-review?(bwinton)
Attachment #574155 - Flags: review?(dbienvenu)
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.
Attachment #574155 - Flags: ui-review?(bwinton) → ui-review-
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.
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.
OS: Windows 7 → All
Hardware: x86_64 → All
Whiteboard: [filter-mgmt]
Version: unspecified → 1.9.2 Branch
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).
Attachment #574155 - Flags: review?(dbienvenu)
Why? Var is used everywhere around that code... What is the difference?
(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.
Thanks David, I can do that.

Bwinton, can you answer my questions so I can continue?
(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.
OK, I'll try it.
Summary: Move focus to newly created filter [UI] → [UI] Move focus to newly created filter; select previous available filter after deletion
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)
Attachment #574155 - Attachment is obsolete: true
Attachment #580195 - Flags: ui-review?(bwinton)
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.
Attachment #580195 - Flags: ui-review?(bwinton) → feedback?(bwinton)
Attachment #580195 - Attachment is obsolete: true
Attachment #580496 - Flags: review?(bwinton)
Attachment #580195 - Flags: feedback?(bwinton)
Attachment #580496 - Flags: review?(bwinton) → ui-review?(bwinton)
Comment on attachment 580496 [details] [diff] [review]
v2, patch implementing both features

Looks good to me!

Thanks,
Blake.
Attachment #580496 - Flags: ui-review?(bwinton) → ui-review+
Comment on attachment 580496 [details] [diff] [review]
v2, patch implementing both features

Thanks, now to the code review:)
Attachment #580496 - Flags: review?(dbienvenu)
Comment on attachment 580496 [details] [diff] [review]
v2, patch implementing both features

sorry for the delay, thx for the patch!
Attachment #580496 - Flags: review?(dbienvenu) → review+
Keywords: checkin-needed
Checked in: http://hg.mozilla.org/comm-central/rev/0eef83152b2d
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 12.0
Status: RESOLVED → VERIFIED
Duplicate of this bug: 620391
You need to log in before you can comment on or make changes to this bug.