392529 - delete key doesn't delete selected mail filter(s)

Status: VERIFIED FIXED

(MailNews Core :: Filters, enhancement)

Description

[Wayne Mery (:wsmwk)]

delete key should delete selected filter(s)

follow up to bug 145290 and bug 357470

Comment 1

[Jens Hatlak (:InvisibleSmiley)]

Attachment: Proposed patch

The attached patch adds delete (Delete) and edit (Enter/Return) actions and fixes the toggle action (at least here on Windows pressing Space gave 0 for event.keyCode; jcranmer kindly told me to go for .which).

Comment 2

[Jens Hatlak (:InvisibleSmiley)]

Attachment: Updated, tested patch

Seems I was too optimistic. event.which reports Delete as 0. :-(

Comment 3

[David :Bienvenu]

Comment on attachment 356410 [details] [diff] [review]
Updated, tested patch

Neil might have some thoughts on this...

Comment 4

[[e-mail]]

Comment on attachment 356410 [details] [diff] [review]
Updated, tested patch

>+  const key = event.which || event.keyCode;
event.which only exists for backward compatibility. You should use charCode for all characters and keyCode for other keys. Seeing as the previous code already used keyCode I guess you might as well just switch on event.keyCode instead.

Comment 5

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #4)
> (From update of attachment 356410 [details] [diff] [review])
> >+  const key = event.which || event.keyCode;
> event.which only exists for backward compatibility. You should use charCode for
> all characters and keyCode for other keys. Seeing as the previous code already
> used keyCode I guess you might as well just switch on event.keyCode instead.

See comment 1 for why keyCode alone won't work (the previous code wasn't working!) and SpatialNavigation.js line 92 and the two LoggingPane.js tests for examples that do it like I did. I checked that any || combination of keyCode and charCode works, though so if you want me to change it either way I will do it.

Comment 6

[[e-mail]]

(In reply to comment #5)
> See comment 1 for why keyCode alone won't work
Sorry, I'd overlooked that. I guess keyCode || charCode will do, but you're really not supposed to do that...

Comment 7

[Jens Hatlak (:InvisibleSmiley)]

Attachment: Cleaner approach, SM part

As discussed on IRC: new patch making clear which property is checked when rather than using event.which||event.keyCode fall back.

It only appeared to me now that this is forked code; TB patch coming up.

Comment 8

[Jens Hatlak (:InvisibleSmiley)]

Attachment: Cleaner approach, TB part

TB doesn't have global button variables, thus I introduced local variables holding the disabled state with the same logic as in updateButtons(). Yet this code is untested (I have no TB set up).

Comment 9

[[e-mail]]

Comment on attachment 357588 [details] [diff] [review]
Cleaner approach, SM part

>+  if (event.charCode == KeyEvent.DOM_VK_SPACE)
Although I'd still prefer " ".charCodeAt(0)

Comment 10

[Jens Hatlak (:InvisibleSmiley)]

Attachment: Cleaner approach, TB part v2

First I forgot to copy the filter variable, then I found this can be done much simpler...

Comment 11

[[e-mail]]

Comment on attachment 357590 [details] [diff] [review]
Cleaner approach, TB part v2

My review doesn't apply here.

Comment 12

[David :Bienvenu]

Comment on attachment 357590 [details] [diff] [review]
Cleaner approach, TB part v2

thx for the patch - can you use let instead of var for "list", and get rid of the temp vars for deleteButton and editButton, e.g.,

if (!document.getElementById("editButton").disabled)

r=me with those changes...

Comment 13

[Jens Hatlak (:InvisibleSmiley)]

Attachment: TB part v3, r=bienvenu [Checkin: Comment 17]

Review comments addressed

Comment 14

[Karsten Düsterloh]

Comment on attachment 357588 [details] [diff] [review]
Cleaner approach, SM part

r=me with the superfluous braces on the inner for loop removed.

Comment 15

[Jens Hatlak (:InvisibleSmiley)]

Attachment: SM part v2, r=mnyromyr, sr=neil [Checkin: Comment 16]

Review comments addressed, sr=neil over IRC

Comment 16

[Serge Gautherie (:sgautherie)]

Comment on attachment 359587 [details] [diff] [review]
SM part v2, r=mnyromyr, sr=neil
[Checkin: Comment 16]


http://hg.mozilla.org/comm-central/rev/15c15e191d58

Comment 17

[Serge Gautherie (:sgautherie)]

Comment on attachment 359560 [details] [diff] [review]
TB part v3, r=bienvenu
[Checkin: Comment 17]


http://hg.mozilla.org/comm-central/rev/c215a361c0df

Comment 18

[Wayne Mery (:wsmwk)]

v.fixed thunderbird
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b3pre) Gecko/20090202 Shredder/3.0b2pre