Closed Bug 924996 Opened 7 years ago Closed 6 years ago

Implement keyboard shortcut to move selected lines in source editor

Categories

(DevTools :: Source Editor, defect, P3)

27 Branch
defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 28

People

(Reporter: daniel.nr01, Assigned: gl)

Details

(Whiteboard: [good first verify] )

Attachments

(1 file, 1 obsolete file)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0 (Beta/Release)
Build ID: 20131008030202

Steps to reproduce:

Since the move to CodeMirror for Scratchpad the keyboard shortcut to move a line up/down is gone. It used to be (on Windows) alt+up/down. It was a very handy shortcut and I think we should reimplement it.
Component: Untriaged → Developer Tools: Scratchpad
Status: UNCONFIRMED → NEW
Ever confirmed: true
This should be relatively easy to implement using CodeMirror's API and I also think it's a useful feature.
OS: Windows 7 → All
Priority: -- → P3
Hardware: x86_64 → All
Hi Benvie, I would like to take on this bug.
Arguably, this should be an Editor bug.
Agree, ask and ye shall receive.
Component: Developer Tools: Scratchpad → Developer Tools: Source Editor
(In reply to Gabriel L (:gluong) from comment #2)
> Hi Benvie, I would like to take on this bug.

Are you still interested in this, Gabriel?
He's been working on this (at least as of yesterday). I'll go ahead an assign it.
Assignee: nobody → gabriel.luong
Hi Rob, I already have the 2 main functions done. Just working on the key bindings and unit test at this point. I will post a WIP tonight.
Summary: The keyboard shortcut to move a line in Scratchpad is gone → Implement keyboard shortcut to move selected lines in source editor
Attached patch 924996.patch (obsolete) — Splinter Review
The current patch implements the keyboard shortcut cmd+alt+up/down to move the selected lines up/down in the source editor. 

You can highlight multiple lines and move them (not necessary to highlight the entire range for those lines). Otherwise, the line that your cursor is currently on will be moved.

After moving the line(s), the cursor or selection needs to be adjusted to where it was in the original text.

I will work on the unit test next, but in the meantime, I could use any comments or feedback regarding the implementation and key mapping. Thanks!
Attachment #8335130 - Flags: feedback?(bbenvie)
Attachment #8335130 - Flags: feedback?(anton)
Comment on attachment 8335130 [details] [diff] [review]
924996.patch

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

Overall, looks good! I'll give more detailed feedback once the patch is ready for review.
Attachment #8335130 - Flags: feedback?(anton) → feedback+
Comment on attachment 8335130 [details] [diff] [review]
924996.patch

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

Indeed, this approach looks fine.
Attachment #8335130 - Flags: feedback?(bbenvie) → feedback+
Attached patch 924996.patchSplinter Review
Added test cases. One thing I did consider was potentially refactoring the current code into its own cm add-on.
Attachment #8335130 - Attachment is obsolete: true
Attachment #8337547 - Flags: review?(anton)
Comment on attachment 8337547 [details] [diff] [review]
924996.patch

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

Looks good. Tested manually. OK to check-in once this Try push is green (i.e. no non-intermittent failures): https://tbpl.mozilla.org/?tree=Try&rev=b18703a23873

Thanks much!
Attachment #8337547 - Flags: review?(anton) → review+
https://hg.mozilla.org/mozilla-central/rev/ddcfbf433ae4
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 28
Whiteboard: [good first verify]
Anton, does Alt Up/ Alt Down work for you in scratchpad ?
It just moves the cursor to top/bottom of the file for me.
Flags: needinfo?(anton)
Gonna take a quick look into this.
This seems to be working correctly on Mac. Could this be a Windows issue?
On Macs, the command is cmd+alt-up/down to move the lines. [1]


[1] https://developer.mozilla.org/en-US/docs/Tools/Using_the_Source_Editor
(In reply to Gabriel L (:gluong) from comment #17)
> This seems to be working correctly on Mac. Could this be a Windows issue?
> On Macs, the command is cmd+alt-up/down to move the lines. [1]
> 
> 
> [1] https://developer.mozilla.org/en-US/docs/Tools/Using_the_Source_Editor

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.

Can we have it back to Alt + UP/DOWN ?
Opened a new bug to keep track of the task to change the key binding to Alt + UP/DOWN for all platforms.
https://bugzilla.mozilla.org/show_bug.cgi?id=968424
Option-Command-Down/Up works for me on a Mac.
Flags: needinfo?(anton)
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.