968424 - Change keyboard shortcut to move selected lines in source editor to Alt-Up/Down

Status: VERIFIED FIXED

(DevTools :: Source Editor, defect)

Description

[Gabriel Luong [:gl] (ΦωΦ)]

Continuing the discussion from https://bugzilla.mozilla.org/show_bug.cgi?id=924996#c18
> So the shortcut before Code Mirror was Alt + UP/DOWN on all browsers except
> Mac, where it was Cmd + Opt + UP/DOWN.
> 
> Now, its Ctrl + Alt + UP/DOWN on windows too. Which does not work out for
> most Windows with Intel chips as Intel graphics driver (at least on Windows)
> map Ctrl + Alt + ARROW keys to screen rotation.
> 
> Also, right now, the documentation is incorrect too. [1]
> 
> Can we have it back to Alt + UP/DOWN ?

Objective of this bug:
1. Change the command key for moving lines up and down to Alt - Up/Down across all platforms.
2. Update MDN documentations.

[1] https://developer.mozilla.org/en-US/docs/Tools/Using_the_Source_Editor

Comment 1

[Gabriel Luong [:gl] (ΦωΦ)]

I will try to have a patch by Friday evening.

Comment 2

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 968424.patch

I wasn't sure what was the best way to ensure that accel wasn't appended to every command. This patch ensure that alt+up/down is the shortkey for moving lines up/down across all platforms (mac and windows included).

Comment 3

[Anton Kovalyov (:anton)]

Comment on attachment 8372126 [details] [diff] [review]
968424.patch

Review of attachment 8372126 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/sourceeditor/editor.js
@@ +831,3 @@
>   */
> +Editor.keyFor = function (cmd, accel=true) {
> +  return accel? Editor.accel(L10N.GetStringFromName(cmd + ".commandkey")) :

Nit: put a space after accel.

@@ +831,4 @@
>   */
> +Editor.keyFor = function (cmd, accel=true) {
> +  return accel? Editor.accel(L10N.GetStringFromName(cmd + ".commandkey")) :
> +         L10N.GetStringFromName(cmd + ".commandkey");

You should assign the value of L10N.GetStringFromName call to a variable:

  var key = L10N.GetStringFromName(cmd + ".commandkey");
  return accel ? Editor.accel(key) : key;

One thing that bugs me about this method, though, is that accel=true by default. This makes Editor.keyFor("X") and Editor.keyFor("X", false) behave differently which is not very JavaScript-ey. Also it introduces a boolean trap [1]. I think this methods signature should be changed to:

  Editor.keyFor = function (cmd, opts={ noaccel: false }) { ... };

[1] — http://ariya.ofilabs.com/2011/08/hall-of-api-shame-boolean-trap.html

::: browser/locales/en-US/chrome/browser/devtools/sourceeditor.properties
@@ +58,5 @@
>  # conjunction with accel (Command on Mac or Ctrl on other platforms) to either
>  # comment or uncomment selected lines in the editor.
>  toggleComment.commandkey=/
>  
> +# LOCALIZATION NOTE  (toolboxPrevTool.commandkey): This is the key to use in

What are these changes? They don't seem to change anything.

Comment 4

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 968424.patch

Thanks for the review Anton! The hall of api shame was especially insightful.

This patch fixes the following:
-Added noaccel option to keyFor signature and adjusted the method calls for moveLineUp and moveLineDown
-In sourceeditor.properties, I updated the localization note for moveLineUp and moveLineDown to remove the note about accel conjuction with the commandkey.

Comment 5

[Anton Kovalyov (:anton)]

Comment on attachment 8375204 [details] [diff] [review]
968424.patch

Review of attachment 8375204 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks for the patch!

Comment 6

[Gabriel Luong [:gl] (ΦωΦ)]

Thanks Anton! I will update the MDN docs once it lands.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/15b392d37510

Comment 8

[Girish Sharma [:Optimizer]]

Can we have this uplifted too ? at least till aurora, if not beta.

Comment 9

[Gabriel Luong [:gl] (ΦωΦ)]

Updated MDN with the new keys

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/15b392d37510

Comment 11

[Girish Sharma [:Optimizer]]

Please ask for aurora approval. This shortcut will be broken in Windows for 2 releases otherwise.

Comment 12

[Gabriel Luong [:gl] (ΦωΦ)]

Can this get uplift to Aurora? Otherwise, the previous command (cmd+alt+up/down) would be rotating Window user's displays. Thanks!

Comment 13

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8375204 [details] [diff] [review]
968424.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 924996
User impact if declined: The previous command for moving lines up and down in the source editor (cmd+alt+up/down) would rotate Window user's displays
Testing completed (on m-c, etc.): on m-c since 2-13-2014
Risk to taking this patch (and alternatives if risky): Low risk - command key changes to DevTools
String or IDL/UUID changes made by this patch:

Comment 14

[Gabriel Luong [:gl] (ΦωΦ)]

Figured out how to ask for aurora uplift!

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/bc0a26f61e41

Comment 16

[Paul Silaghi, QA [:pauly]]

(In reply to Gabriel L (:gluong) from comment #0)
> Objective of this bug:
> 1. Change the command key for moving lines up and down to Alt - Up/Down
> across all platforms.
> 2. Update MDN documentations.
Verified fixed FF 29.0a2(2014-03-11), 30.0a1(2014-03-11) Win 7, Ubuntu 13.04, Mac OS X 10.8.5

Comment 17

[Will Bamberg [:wbamberg]]

I'm updating devtools docs for Firefox 30, and just wanted to check with you that the documentation for these shortcuts is correct now. I reorganised the keyboard shortcut docs after you updated them, but before I saw this bug, so I just wanted to check I didn't break the docs again after you fixed them:

https://developer.mozilla.org/en-US/docs/tools/Keyboard_shortcuts#Source_editor

Thanks!

Comment 18

[Gabriel Luong [:gl] (ΦωΦ)]

Looks good still!