Closed Bug 968424 Opened 6 years ago Closed 6 years ago

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

Categories

(DevTools :: Source Editor, defect)

x86
Windows 8
defect
Not set

Tracking

(firefox29 verified, firefox30 verified)

VERIFIED FIXED
Firefox 30
Tracking Status
firefox29 --- verified
firefox30 --- verified

People

(Reporter: gl, Assigned: gl)

Details

Attachments

(1 file, 1 obsolete file)

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
I will try to have a patch by Friday evening.
Assignee: nobody → gabriel.luong
OS: Mac OS X → Windows 8
Attached patch 968424.patch (obsolete) — Splinter Review
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).
Attachment #8372126 - Flags: review?(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.
Attachment #8372126 - Flags: review?(anton) → review-
Attached patch 968424.patchSplinter Review
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.
Attachment #8372126 - Attachment is obsolete: true
Attachment #8375204 - Flags: review?(anton)
Comment on attachment 8375204 [details] [diff] [review]
968424.patch

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

Looks good, thanks for the patch!
Attachment #8375204 - Flags: review?(anton) → review+
Whiteboard: [checkin-needed]
Thanks Anton! I will update the MDN docs once it lands.
https://hg.mozilla.org/integration/fx-team/rev/15b392d37510
Whiteboard: [checkin-needed] → [fixed-in-fx-team]
Can we have this uplifted too ? at least till aurora, if not beta.
Updated MDN with the new keys
https://hg.mozilla.org/mozilla-central/rev/15b392d37510
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 30
Keywords: verifyme
Please ask for aurora approval. This shortcut will be broken in Windows for 2 releases otherwise.
Flags: needinfo?(gabriel.luong)
Can this get uplift to Aurora? Otherwise, the previous command (cmd+alt+up/down) would be rotating Window user's displays. Thanks!
Flags: needinfo?(gabriel.luong) → needinfo?(rcampbell)
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:
Attachment #8375204 - Flags: approval-mozilla-aurora?
Figured out how to ask for aurora uplift!
Flags: needinfo?(rcampbell)
Attachment #8375204 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(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
Status: RESOLVED → VERIFIED
Keywords: verifyme
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!
Flags: needinfo?(gabriel.luong)
Looks good still!
Flags: needinfo?(gabriel.luong)
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.