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

VERIFIED FIXED in Thunderbird 12.0

Status

MailNews Core
Filters
--
minor
VERIFIED FIXED
7 years ago
5 years ago

People

(Reporter: Frank Spade, Assigned: aceman)

Tracking

1.9.2 Branch
Thunderbird 12.0

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [filter-mgmt])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

7 years ago
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]
(Assignee)

Comment 2

6 years ago
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.
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-
(Assignee)

Comment 4

6 years ago
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.
(Assignee)

Comment 5

6 years ago
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 6

6 years ago
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)
(Assignee)

Comment 7

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

Comment 8

6 years ago
(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.
(Assignee)

Comment 9

6 years ago
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.
(Assignee)

Comment 11

6 years ago
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
(Assignee)

Comment 12

6 years ago
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)
Attachment #574155 - Attachment is obsolete: true
Attachment #580195 - Flags: ui-review?(bwinton)
(Assignee)

Comment 13

6 years ago
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)
(Assignee)

Comment 14

6 years ago
Created attachment 580496 [details] [diff] [review]
v2, patch implementing both features
Attachment #580195 - Attachment is obsolete: true
Attachment #580496 - Flags: review?(bwinton)
Attachment #580195 - Flags: feedback?(bwinton)
(Assignee)

Updated

6 years ago
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+
(Assignee)

Comment 16

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

Thanks, now to the code review:)
Attachment #580496 - Flags: review?(dbienvenu)

Comment 17

6 years ago
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+
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
Checked in: http://hg.mozilla.org/comm-central/rev/0eef83152b2d
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 12.0
(Assignee)

Updated

5 years ago
Status: RESOLVED → VERIFIED
(Assignee)

Updated

5 years ago
Duplicate of this bug: 620391
You need to log in before you can comment on or make changes to this bug.