Closed Bug 452634 Opened 16 years ago Closed 13 years ago

Add keyboard shortcut to add attachment [Ctrl+Shift+A], change keyboard shortcut for "Remove Named Anchors" to [Ctrl+Shift+R]

Categories

(Thunderbird :: Message Compose Window, enhancement)

enhancement
Not set
normal

Tracking

(thunderbird3.0 wontfix)

RESOLVED FIXED
Thunderbird 10.0
Tracking Status
thunderbird3.0 --- wontfix

People

(Reporter: hendrik, Assigned: squib)

References

(Blocks 2 open bugs, )

Details

Attachments

(1 file, 3 obsolete files)

There is keyboard shortcut to add an attachment when creating a new message.  This forces me to use the mouse, which is annoying.  It would be great if there were a keyboard shortcut.

Workaround: Use the accel keys for File → Attachment → File
Seconded by me.
Ping.
Version: 2.0 → Trunk
any shortcut proposals?
How about Ctrl+Shift+A? (Seems to do nothing now.)
That's taken. (Remove named anchors.)
I'm actually working on this bug. How about Cltrl_Shift+F, it's not taken elsewhere
(In reply to comment #8)
> I'm actually working on this bug. How about Cltrl_Shift+F, it's not taken
> elsewhere

Bryan ?
Assignee: nobody → mirna.bouchra
hmm, that looks like it's free to me though it doesn't have as much meaning as something like ctrl+shift+a would.
ctrl+shift+a is taken though
Comment on attachment 418363 [details] [diff] [review]
The attached patch is to create a keyboard shortcut for attach file in Message Compose Window. the shortcut is ctrl+shift+F since ctrl+shift+a is taken. the shortcut can be changed if there is a more 

Thanks so much for the patch!  Sorry I fell behind on this during the holidays but I'm back now.

Everything looks good here other than some indenting on the messengercompose.xul file which should be easy to fix.

I'm minusing because I think should be using ctrl+shift+a as our key combination.  After looking into the function that is already using it I believe we should drop that key code in favor of this one.  Remove named anchors is a by far less used feature than attaching files to an email so I can't see why it would get the better key combination.

From here all that is required is to remove the key code from the other action such that we can use it for attachments.  Then if you add me for the ui-review of that patch I'll make sure we can get a proper code review of it as well.

With any luck we might be able to land this in the 3.0.1 release to get it out quickly.
Attachment #418363 - Flags: ui-review?(clarkbw)
Attachment #418363 - Flags: ui-review-
Attachment #418363 - Flags: superreview?(clarkbw)
Attachment #418363 - Flags: review?(clarkbw)
Setting this as assigned to Mirna and marking for wanted in the 3.0 point releases however since shortcuts are localized our odds are low that we'll be able to sneak this in.
Status: NEW → ASSIGNED
s/low/absolutely zero/. You're lucky that formatRemoveNamedAnchors.key is only in editor/ui/, not in mozilla/ somewhere, so you only have to coordinate stealing ctrl+shift+A with SeaMonkey mail and SeaMonkey composer, not with Firefox and its schedule too, but still: things like this which absolutely positively should not be done or even thought about for a stable branch are exactly why we want 3.1 and later to be quick releases. The first thing that should come into your head is "we'll be able to land this for the 3.1 release to get it out quickly" not "how can we subvert l10n to get this into some people's hands in less than three years?"
I wouldn't say 'subvert' as much as "employ judicious use of smoke and mirrors" but you're completely correct that my world has changed to a long release view and should change back to our new world order.
Even in a long release view world, doing something like this which is just a little extra convenience isn't right for l10n hence wontfix for the 3.0 branch.
Just to note that Cmd-Shift-A is the shortcut used by Mail.App, Outlook doesn't appear to have such a shortcut.
I'm really sorry I fell behind this for a while. I was having my final exams. Anyways, I'll ge this fixed really soon and update you. Thanks a lot for your help :)
(In reply to comment #20)
> Created an attachment (id=422017) [details]
> Patch for removeNamedAnchors shortcut removed and Alt+shift+A shortcut for add
> attachment

Don't forget to target reviewers too :-)
Attachment #422017 - Attachment description: Patch for removeNamedAnchors shortcut removed and Alt+shift+A shortcut for add attachment → Patch for removeNamedAnchors shortcut removed and ctrl+shift+A shortcut for add attachment
(In reply to comment #21)
> (In reply to comment #20)
> > Created an attachment (id=422017) [details] [details]
> > Patch for removeNamedAnchors shortcut removed and Alt+shift+A shortcut for add
> > attachment
> 
> Don't forget to target reviewers too :-)

Mr Ludovic, you turned my attention to some mistake=) the shortcut is ctrl+shift+A not alt+shift+A. I do really apologize for this. 
I requested a ui review from Mr Bryan W. Clark. could you please tell me who else should I ask their review?
Thanks a lot and sorry again for the typo
Comment on attachment 422017 [details] [diff] [review]
Patch for removeNamedAnchors shortcut removed and ctrl+shift+A shortcut for add attachment

Magnus, can you take the review on this?  I'm not sure about the superreview requirements here but I'd like to get Mnyromyr to take a look.
Attachment #422017 - Flags: ui-review?(clarkbw)
Attachment #422017 - Flags: ui-review+
Attachment #422017 - Flags: superreview?
Attachment #422017 - Flags: review?(mkmelin+mozilla)
Attachment #422017 - Flags: review?
Karsten, is this something you'd want for suite as well?
Comment on attachment 422017 [details] [diff] [review]
Patch for removeNamedAnchors shortcut removed and ctrl+shift+A shortcut for add attachment

This patch does not apply:

patching file editor/ui/composer/content/editorOverlay.xul
This patch doesn't apply

patch: **** malformed patch at line 40: diff -r 4b63c4badb42 editor/ui/locales/en-US/chrome/composer/editorOverlay.dtd

Also a few notes:
 - align the attributes properly with those on the line above
 - use spaces only (mozilla style is no tabs)

For the next patch, ask sr from neil as iirc he owns /editor. If this isn't wanted there this change might be significantly harder to code.
Attachment #422017 - Flags: review?(mkmelin+mozilla) → review-
Mirna, thanks a lot for working on this patch; I'm looking forward to this feature. Do you think you can fix the technical problems of your patch that are mentioned in comment 11?

btw, in bugzilla.mozilla.org, we tend to use first names instead of Mr. ... to make everyone's life easier (or full names where needed for clarification, but still without "Mr.").
Mirna, are you still working on this patch? If yes, could you fix the technical problems of your patch and request superreview as per comment 25?
If not, please unassign yourself from this bug (assign to nobody) so that somebody else can pick it up.
Summary: Add shortcut to add attachment → Add keyboard shortcut to add attachment [Ctrl+Shift+A]
(In reply to comment #23)
> Comment on attachment 422017 [details] [diff] [review]
> Patch for removeNamedAnchors shortcut removed and ctrl+shift+A shortcut for add
> attachment
> 
> Magnus, can you take the review on this?  I'm not sure about the superreview
> requirements here but I'd like to get Mnyromyr to take a look.

Isn't Ctrl-Shift-A already taken by Edit -> Select -> Thread? 
(http://support.mozillamessaging.com/en-US/kb/Keyboard+shortcuts#Message_functions)

(Or is it okay that a shortcut has one behavior when a compose window is open and another the rest of the time?)
Blocks: tbattacha11y
Jim, this little bug already has ui-review+ and a draft patch (with some rough edged explained in comment 25). Would be a great complement to round off your keyboard accessibility work in bug 630759, only that this one is limited to composer. Perhaps you could have mercy...

(In reply to comment #28)
> Isn't Ctrl-Shift-A already taken by Edit -> Select -> Thread? 
> (http://support.mozillamessaging.com/en-US/kb/
> Keyboard+shortcuts#Message_functions)
> (Or is it okay that a shortcut has one behavior when a compose window is
> open and another the rest of the time?)

Yes, it's very OK for a shortcut to have different behaviours in different contexts, especially different components (compose vs. mail reader).
No problem with Ctrl+Shift+A in the composer, that's why it has ui-review+.
Assignee: mirna.bouchra → nobody
Status: ASSIGNED → NEW
Whiteboard: [has ui-review+ and draft patch]
Here's an updated patch that should apply. Note that this doesn't work quite right in Linux; that's bug 269228.
Assignee: nobody → squibblyflabbetydoo
Attachment #418363 - Attachment is obsolete: true
Attachment #422017 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #546063 - Flags: ui-review+
Attachment #546063 - Flags: superreview?(neil)
Attachment #546063 - Flags: review?(mkmelin+mozilla)
Comment on attachment 546063 [details] [diff] [review]
Make Ctrl+Shift+A add attachment instead of remove named anchors

>diff --git a/mail/components/compose/content/editorOverlay.xul b/mail/components/compose/content/editorOverlay.xul
Given that you have your own override of this file, you could in theory just drop the removenamedanchorskb key locally. Or you could reduce disruption by renaming instead of removing the named anchors key. We'd want you to use one that isn't used by either SeaMonkey's editor or message compose, of course.
(In reply to comment #31)
> Given that you have your own override of this file, you could in theory just
> drop the removenamedanchorskb key locally. Or you could reduce disruption by
> renaming instead of removing the named anchors key. We'd want you to use one
> that isn't used by either SeaMonkey's editor or message compose, of course.

Do you have a preference either way? I'm not picky.
(In reply to comment #32)
> Do you have a preference either way?
I guess my preference is for you to rename the key.
Neil, thank you for providing the information needed for Jim to finalize this useful patch! *omg, what a poorly disguised attempt to show my eagerness on seeing this in action* :)
Ok, it looks like Ctrl+Shift+R is free for "Remove named anchors". I couldn't find anything else that used it, at least.

Unfortunately, Ctrl+Shift+A doesn't work very well on Linux, since GTK eats that key combo in text fields (e.g. addressing and subject fields, but not the message body). See bug 269228 for that. This is sad news for Linux users, but it's not like things are getting any worse for them as a result of this patch, so maybe it's not a big deal. We could try to find an alternate keybinding (or fix bug 269228), though.
Attachment #546063 - Attachment is obsolete: true
Attachment #560514 - Flags: ui-review+
Attachment #560514 - Flags: superreview?(neil)
Attachment #560514 - Flags: review?(mbanner)
Attachment #546063 - Flags: superreview?(neil)
Attachment #546063 - Flags: review?(mkmelin+mozilla)
Attachment #560514 - Flags: superreview?(neil) → superreview+
Comment on attachment 560514 [details] [diff] [review]
Give "Remove named anchors" a different key

>diff --git a/editor/ui/locales/en-US/chrome/composer/editorOverlay.dtd b/editor/ui/locales/en-US/chrome/composer/editorOverlay.dtd

> <!ENTITY formatRemoveNamedAnchors.label "Remove Named Anchors">
> <!ENTITY formatRemoveNamedAnchors.accesskey "R">
>-<!ENTITY formatRemoveNamedAnchors.key "A">
>+<!ENTITY formatRemoveNamedAnchors2.key "R">

Axel, do I remember something mentioned in the past about we should be keeping all these names in sync for the benefit of the l10n tools? (i.e. add '2' onto the accesskey and label as well)?
For accesskeys, it might. This is a control key, though, which aren't mapped in to the string by tools, so that's fine.
Mark, given comment 39 which addresses your comment 38, would that imply your review+ for this small and straightforward patch?
Attachment #560514 - Flags: review?(mbanner) → review+
Keywords: checkin-needed
Whiteboard: [has ui-review+ and draft patch]
Summary: Add keyboard shortcut to add attachment [Ctrl+Shift+A] → Add keyboard shortcut to add attachment [Ctrl+Shift+A], change keyboard shortcut for "Remove Named Anchors" to [Ctrl+Shift+R]
Checked in: http://hg.mozilla.org/comm-central/rev/05f2ab551575
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 10.0
Comment on attachment 560514 [details] [diff] [review]
Give "Remove named anchors" a different key

>diff --git a/editor/ui/locales/en-US/chrome/composer/editorOverlay.dtd b/editor/ui/locales/en-US/chrome/composer/editorOverlay.dtd
>--- a/editor/ui/locales/en-US/chrome/composer/editorOverlay.dtd
>+++ b/editor/ui/locales/en-US/chrome/composer/editorOverlay.dtd
>@@ -212,7 +212,7 @@
> <!ENTITY formatRemoveLinks.key "K">
> <!ENTITY formatRemoveNamedAnchors.label "Remove Named Anchors">
> <!ENTITY formatRemoveNamedAnchors.accesskey "R">
>-<!ENTITY formatRemoveNamedAnchors.key "A">
>+<!ENTITY formatRemoveNamedAnchors2.key "R">

Ouch. First, why change the string ID at all there? Second, now (L10n) tools cannot connect the key to the label and accesskey any more because the part before the . is different :(
Hmm, OK, Axel already weighed in. I guess for a .key it's tolerable, even if not ideal.
I hope that with L20n we'll get some tooling that will make stuff like that cleaner.
This needs to be mentioned in relnotes (new in tb10), and updated in keyboard shortcuts article.
Keywords: relnote
(In reply to Thomas D. from comment #44)
> This needs to be mentioned in relnotes (new in tb10)

Jen has documented this in http://support.mozillamessaging.com/kb/new-thunderbird-10. Thank you!

Should this be mentioned in the actual relase notes, too? I think we should be more generous with mentioning new features, and adding attachments is quite a frequent action...

https://www.mozilla.org/en-US/thunderbird/10.0/releasenotes/ (not yet published, so final URL might change)
-> What's New in Thunderbird

Well, and there's still a third place:

Help > What's New

...with an TB-Internal documentation (opens in a TB-tab) of some highlights of the new version.
IMO, new keyboard shortcuts definitely belong there, and they should have their own section.

> and updated in keyboard shortcuts article (1)

I've updated the keyboard shortcuts article with this new shortcut already, in major revision 5658 (2), as documented in bug 721666.

|Remove Named Anchors||{for =tb31,=tb5,=tb6,=tb7,=tb8,=tb9}{for win,linux}{key Ctrl+Shift+A}{/for}{for mac}{key Command+Shift+A}{/for}{/for} {for tb10}{for win,linux}{key Ctrl+Shift+R}{/for}{for mac}{key Command+Shift+R}{/for}{/for}

|Attach File||{for tb10}{for win,linux}{key Ctrl+Shift+A}{/for}{for mac}{key Command+Shift+A}{/for}{/for}

(1) https://support.mozillamessaging.com/en-US/kb/keyboard-shortcuts/revision
(2) https://support.mozillamessaging.com/en-US/kb/keyboard-shortcuts/revision/5658
Blocks: 721666
Keywords: relnote
You need to log in before you can comment on or make changes to this bug.