Closed Bug 392529 Opened 14 years ago Closed 12 years ago

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

Categories

(MailNews Core :: Filters, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
mozilla1.9.1b3

People

(Reporter: wsmwk, Assigned: InvisibleSmiley)

Details

Attachments

(2 files, 5 obsolete files)

delete key should delete selected filter(s)

follow up to bug 145290 and bug 357470
Severity: normal → enhancement
OS: Windows XP → All
Hardware: PC → All
Product: Core → MailNews Core
Attached patch Proposed patch (obsolete) — Splinter Review
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).
Assignee: nobody → jh
Status: NEW → ASSIGNED
Attachment #356333 - Flags: superreview?(bienvenu)
Attachment #356333 - Flags: review?(bienvenu)
Attached patch Updated, tested patch (obsolete) — Splinter Review
Seems I was too optimistic. event.which reports Delete as 0. :-(
Attachment #356333 - Attachment is obsolete: true
Attachment #356410 - Flags: superreview?(bienvenu)
Attachment #356410 - Flags: review?(bienvenu)
Attachment #356333 - Flags: superreview?(bienvenu)
Attachment #356333 - Flags: review?(bienvenu)
Comment on attachment 356410 [details] [diff] [review]
Updated, tested patch

Neil might have some thoughts on this...
Attachment #356410 - Flags: review?(bienvenu) → review?(neil)
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.
Attachment #356410 - Flags: review?(neil) → review+
(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.
(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...
Attached patch Cleaner approach, SM part (obsolete) — Splinter Review
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.
Attachment #356410 - Attachment is obsolete: true
Attachment #357588 - Flags: review?(neil)
Attachment #356410 - Flags: superreview?(bienvenu)
Attached patch Cleaner approach, TB part (obsolete) — Splinter Review
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).
Attachment #357589 - Flags: review?(neil)
Attachment #357588 - Flags: review?(neil) → review+
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)
Attached patch Cleaner approach, TB part v2 (obsolete) — Splinter Review
First I forgot to copy the filter variable, then I found this can be done much simpler...
Attachment #357589 - Attachment is obsolete: true
Attachment #357590 - Flags: review?(neil)
Attachment #357589 - Flags: review?(neil)
Comment on attachment 357590 [details] [diff] [review]
Cleaner approach, TB part v2

My review doesn't apply here.
Attachment #357590 - Flags: review?(neil)
Attachment #357590 - Flags: review?(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...
Review comments addressed
Attachment #357590 - Attachment is obsolete: true
Attachment #357590 - Flags: review?(bienvenu)
Comment on attachment 357588 [details] [diff] [review]
Cleaner approach, SM part

r=me with the superfluous braces on the inner for loop removed.
Attachment #357588 - Flags: review+
Review comments addressed, sr=neil over IRC
Attachment #357588 - Attachment is obsolete: true
Attachment #359587 - Flags: superreview+
Attachment #359587 - Flags: review+
Attachment #359560 - Flags: review+
Keywords: checkin-needed
Attachment #359587 - Attachment description: SM part v2, r=mnyromyr, sr=neil → SM part v2, r=mnyromyr, sr=neil [Checkin: Comment 16]
Comment on attachment 359560 [details] [diff] [review]
TB part v3, r=bienvenu
[Checkin: Comment 17]


http://hg.mozilla.org/comm-central/rev/c215a361c0df
Attachment #359560 - Attachment description: TB part v3, r=bienvenu → TB part v3, r=bienvenu [Checkin: Comment 17]
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b3
v.fixed thunderbird
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b3pre) Gecko/20090202 Shredder/3.0b2pre
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.