725618 - Source Editor: keyboard shortcut for moving lines up/down

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Mihai Sucan [:msucan]]

Code editors usually allow one to move the current line or the selected lines up/down within the editor. We should add such convenience shortcuts, maybe Ctrl-Up/Down?


How to fix: similar steps to bug 725375 comment 1.

Comment 1

[Mihai Sucan [:msucan]]

Via email Allen has volunteered to take this bug. Thank you!

Comment 2

[Mihai Sucan [:msucan]]

Allen: for this feature we will use the Alt-Up/Down keyboard shortcuts.

Comment 3

[Allen Eubank]

Attachment: Move Lines Up/Down

Comment 4

[Mihai Sucan [:msucan]]

Comment on attachment 598158 [details] [diff] [review]
Move Lines Up/Down

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

This is great work Allen! Thanks for your patch! It did apply cleanly and it works really well.

General comments:
- please remove all trailing whitespaces in the patch.
- please put curly braces like in the rest of the file:
  if (foo) {
    bar();
  } else {
    baz();
  }

I am looking forward for the updated patch. Thank you Allen!

::: browser/devtools/sourceeditor/source-editor-orion.jsm
@@ +553,5 @@
>      }
>    },
>  
>    /**
> +   * Move line/lines up/down.

Move lines upwards or downwards, relative to the current caret location.

@@ +555,5 @@
>  
>    /**
> +   * Move line/lines up/down.
> +   */
> +  _moveLines: function SE_moveLines(aLineAbove)

Function name should be SE__moveLines.

Also, please describe the argument and mark the method as @private.

@@ +564,5 @@
> +    let firstLineStart = this._model.getLineStart(firstLine);
> +    let lastLineEnd = this._model.getLineEnd(lastLine);
> +    let text = this.getText(firstLineStart, lastLineEnd);
> +
> +    if(aLineAbove && ((firstLine - 1)!= -1))

You can bail out of the function quicker: in the code above do if (firstLine == 0) { return true; }. Similarly for lastLine and getLineCount().

@@ +571,5 @@
> +      let aboveLineStart = this._model.getLineStart(aboveLine);
> +      let aboveLineEnd = this._model.getLineEnd(aboveLine);
> +      let aboveText = this.getText(aboveLineStart, aboveLineEnd);
> +      this.setText(text + this.getLineDelimiter() + aboveText,
> +                    aboveLineStart, lastLineEnd);

This makes more changes than needed. Please do a setText("") for the old text range to delete the old text, then add the new text at the new location. To keep undo/redo atomic please do this.startCompoundChange(), make your setText() calls, then this.endCompoundChange().

@@ +574,5 @@
> +      this.setText(text + this.getLineDelimiter() + aboveText,
> +                    aboveLineStart, lastLineEnd);
> +      this.setSelection(aboveLineStart, aboveLineStart + text.length);
> +    }
> +    else if ((lastLine + 1) != this._model.getLineCount())

Please use this.getLineCount() to make sure that any future changes that might happen in Orion are easily solved with a single method change (in this case, our getLineCount).

@@ +584,5 @@
> +      this.setText(belowText + this.getLineDelimiter() + text,
> +                    firstLineStart, belowLineEnd);
> +      this.setSelection(belowLineEnd - text.length, belowLineEnd);
> +    }
> +  },

Please make sure the _moveLines() function returns true.

Comment 5

[Allen Eubank]

Attachment: Move Lines Up/Down Version 2

Thanks for giving my last patch feedback Mihai! And thanks for helping me with this. I am pretty sure I addressed most of your comments. I do have a couple of questions. One of them is about this comment,

> This makes more changes than needed. Please do a setText("") for the old
> text range to delete the old text, then add the new text at the new
> location. To keep undo/redo atomic please do this.startCompoundChange(),
> make your setText() calls, then this.endCompoundChange().

As you can see I had attempted a solution to this but doing a this.setText("") on the old range before putting the new text caused weird behavior. It is commented out but this is how I attempted it. With those lines commented out the method behaves normally and works much better with redo/undo. What do you think?

Also, I noticed on the http://bugzilla.mozilla.org/show_bug.cgi?id=725375, you said you don't have to do a build every time you make a change on Linux? Can you describe how you achieve this? I am Ubuntu.

Comment 6

[Mihai Sucan [:msucan]]

Comment on attachment 598505 [details] [diff] [review]
Move Lines Up/Down Version 2

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

This is quite better Allen! Thanks for your updated patch!

::: browser/devtools/sourceeditor/source-editor-orion.jsm
@@ +587,5 @@
> +      //this.setText("", firstLineStart, lastLineEnd);
> +      this.setText(text + this.getLineDelimiter() + aboveText,
> +                    aboveLineStart, lastLineEnd);
> +      this.endCompoundChange();
> +      this.setSelection(aboveLineStart, aboveLineStart + text.length);

Uncomment the setText("") line, then do:

this.setText(text + this.getLineDelimiter(), aboveLineStart, aboveLineStart);

  so that you insert the text at the start of the line above (firstLine - 1).

(also, you no longer need aboveLineEnd, nor aboveText)

@@ +599,5 @@
> +      //this.setText("", firstLineStart, lastLineEnd);
> +      this.setText(belowText + this.getLineDelimiter() + text,
> +                    firstLineStart, belowLineEnd);
> +      this.endCompoundChange();
> +      this.setSelection(belowLineEnd - text.length, belowLineEnd);

Uncomment the setText("") line, then do:

  let insertOffset = belowLineEnd - lastLineEnd + firstLineStart;
  this.setText(this.getLineDelimiter() + text, insertOffset, insertOffset);

so that you insert the text at the end of the line below (lastLine + 1). You need to calculate the new offset, taking into consideration that all the chars between firstLineStart and lastLineEnd have been removed).

Selection range also needs to be updated:
  this.setSelection(insertOffset, insertOffset + text.length);

Comment 7

[Mihai Sucan [:msucan]]

(In reply to Allen Eubank from comment #5)
> Created attachment 598505 [details] [diff] [review]
> Move Lines Up/Down Version 2
> 
> Thanks for giving my last patch feedback Mihai! And thanks for helping me
> with this. I am pretty sure I addressed most of your comments. I do have a
> couple of questions. One of them is about this comment,

No worries! It's my pleasure!

> > This makes more changes than needed. Please do a setText("") for the old
> > text range to delete the old text, then add the new text at the new
> > location. To keep undo/redo atomic please do this.startCompoundChange(),
> > make your setText() calls, then this.endCompoundChange().
> 
> As you can see I had attempted a solution to this but doing a
> this.setText("") on the old range before putting the new text caused weird
> behavior. It is commented out but this is how I attempted it. With those
> lines commented out the method behaves normally and works much better with
> redo/undo. What do you think?

Good question. I just answered this in the previous comment 6. I hope that helps and I hope I haven't broken the offsets - please double check the code.


> Also, I noticed on the http://bugzilla.mozilla.org/show_bug.cgi?id=725375,
> you said you don't have to do a build every time you make a change on Linux?
> Can you describe how you achieve this? I am Ubuntu.

For some changes you always need to rebuild, but for source editor related changes it I do not need (I only need to restart Firefox). I think it depends on your mozconfig options and on other system configs - build experts know this better than I do. Anyhow, try a mozconfig that is similar to the following:

https://github.com/mihaisucan/mozilla-work/blob/master/mozconfig/opt

This is my mozconfig for opt builds. I did enable symlinks so that also helps with less rebuilding of code. Your mileage may vary.

Please let me know if you have more questions!

Comment 8

[Rob Campbell [:rc] (:robcee)]

I am late to this party and sorry for chiming in after work has already been done.

Is this really a feature we want or need? In the context of the scratchpad it feels a little funny. None of the editors I use regularly support this type of convenience. On OS X, alt+up/down usually results in cursor to previous/next brace or paragraph.

I think I'd be OK with it if the keyboard shortcut were different. Maybe Cmd/Ctrl+Alt+Up/Down?

Comment 9

[Allen Eubank]

Mihai,

I put in the new revisions, and it didn't work quite as expected. I found problems with an extra delimiter getting thrown in, both with the move lines up and move lines down. I played around a bit with the this.setText("") changing parameters but still could not get it to work properly. I am having trouble understanding why the delete of the old range. The way I saw this feature working was just swapping a selection and a line. I'll continue to try and work with deleting the old range but can you explain the reasoning and the differences behind this way as opposed to doing a little more changes but effectively swapping a selection and a line?

Hi Rob,

That shortcut could work but I know Ubuntu uses that command to switch between workspaces. So a user on Ubuntu using Scratchpad trying to move lines could potentially be switching between two workspaces(one above and one below).

What about a Cmd/Ctrl+Up/Down? I am not sure what programs use that but none come to mind at the moment.

Comment 10

[Mihai Sucan [:msucan]]

Allen: I discussed with Rob about the keyboard shortcut. If I am not mistaken, we can keep Alt-Up/Down for Windows and Linux. For Macs we either remove the shortcut entirely or we decide on a different shortcut.

Rob, please let us know what keyboard shortcuts to use on Macs. 


(Allen: wrt. the setText("") problem you have, I'll post a separate reply, later.)

Comment 11

[Rob Campbell [:rc] (:robcee)]

Hi Allen, Mihai,

I think for Mac we should using Alt+Shift or Alt+Ctrl Up/Down. Alt+up/down have fairly well understood behavior on Mac that this will break. Alt+Up/down should move to previous/next brace on Mac (which Mihai said he would file).

Thanks!

Comment 12

[Mihai Sucan [:msucan]]

Attachment: some fixes

Allen: here's an updated version of your patch with all functionality working.

Basically, if you applied the suggestions from comment 6 you were very closes to get it to work well. The problem was that the new lines were not properly handled. You need to keep in mind what you copy with getText() - if it includes any new line at the end of the text, or if it includes a new line at the start of the text. Similarly, you pay attention to where you insert the text: before new lines or after, and so on.

The patch I submit here does that: please pay attention to the changes:

1. when you select a whole line, the line end is included in the selection, and getLineAtOffset(selection.end) gives you the next line, which is not selected. So, I added a check for lastLineStart to see if selection.end == lastLineStart, if yes then do lastLine-1.

2. for lastLineEnd we do getLineEnd(lastLine, true), to include always the line end (the line delimiter).

3. for insertAbove: if lastLine == getLineCount() - 1, then we need to add the line delimiter to the text, because the last line does not include it. We adjust offsets accordingly. We do setText("") to remove the old selection as needed. Then we do setText(text, aboveLineStart, aboveLineStart + text.length).

4. for insert below: belowLineEnd also needs to include the line delimiter (getLineEnd(belowLine, true)). Then when we are at the end of the document, the last line does not have a new line character at the end, so we need to pay specific attention to that.

That's pretty much all.

Please let me know if this helps and if you have any further questions! Thanks! Also, please continue your work based on this patch. More specifically, please remove any trailing whitespace in the code, remove unused variables and so on.

Comment 13

[Allen Eubank]

Attachment: Move Lines Up/Down Version 2.1

This patch worked great with the fixes. You explained it very clearly and I see the improvement. A very clean solution :) Thanks Mihai! 

I have updated the patch to the new version and I am pretty sure no trailing white spaces or unused variables are included.

Comment 14

[Allen Eubank]

Rob, Mihai,

For adding the different Mac user shortcut, how should this be implemented? I noticed you can get the user's OS from this line of code "Services.appinfo.OS" but do we need to add new elements to the DEFAULT_KEYBINDING array near the beginning of the file?

Something like this for Mac, 
  {
    action: "Move Lines Down - Mac",
    code: Ci.nsIDOMKeyEvent.DOM_VK_DOWN,
    accel: true,
    alt: true,
  },

Comment 15

[Allen Eubank]

Attachment: Move Lines Up/Down Version 2.1

Sorry, last patch had some an typos, I guess I accidentally deleted some code and didn't see it in the diff. This patch is correct.

Comment 16

[Mihai Sucan [:msucan]]

Comment on attachment 600308 [details] [diff] [review]
Move Lines Up/Down Version 2.1

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

This is looking good. Thank you Allen!

Please go ahead and write a test. To learn how to write and run tests please see:

https://developer.mozilla.org/en/Browser_chrome_tests

The Source Editor tests are in browser/devtools/sourceeditor/test. Please let me know if you have any questions! Thanks!

::: browser/devtools/sourceeditor/source-editor-orion.jsm
@@ -79,5 @@
> -  Focus: "Focus",
> -  Blur: "Blur",
> -  MouseOver: "MouseOver",
> -  MouseOut: "MouseOut",
> -  MouseMove: "MouseMove",

Please do not remove these events. Please rebase your patch to the latest mozilla-central repository.

We have landed some changes to the source editor which do not affect your patch, but they make your patch to fail to apply cleanly.

Comment 17

[Mihai Sucan [:msucan]]

(In reply to Allen Eubank from comment #14)
> Rob, Mihai,
> 
> For adding the different Mac user shortcut, how should this be implemented?
> I noticed you can get the user's OS from this line of code
> "Services.appinfo.OS" but do we need to add new elements to the
> DEFAULT_KEYBINDING array near the beginning of the file?
> 
> Something like this for Mac, 
>   {
>     action: "Move Lines Down - Mac",
>     code: Ci.nsIDOMKeyEvent.DOM_VK_DOWN,
>     accel: true,
>     alt: true,
>   },

You can do at the top of the file:

if (Services.appinfo.OS == "Darwin") {
  DEFAULT_KEYBINDINGS.push({...});
} else {
  ...
}

(please change from const DEFAULT_KEYBINDINGS = {...} to let DEFAULT_KEYBINDINGS = {...})

Comment 18

[Allen Eubank]

Attachment: Move Lines Up/Down Version 3

Added different shortcut for Mac.

Comment 19

[Allen Eubank]

Attachment: Initial test case

Initial test case. Want to make sure I am on the right track. Still need to add testing for Mac shortcut. Want to make sure before I write that part of the test that my V3 patch has a correct solution to implementing Mac shortcut.

Comment 20

[Mihai Sucan [:msucan]]

Comment on attachment 601174 [details]
Initial test case

Thanks for your test Allen! This is great!

(In reply to Allen Eubank from comment #19)
> Created attachment 601174 [details]
> Initial test case
> 
> Initial test case. Want to make sure I am on the right track. Still need to
> add testing for Mac shortcut. Want to make sure before I write that part of
> the test that my V3 patch has a correct solution to implementing Mac
> shortcut.

Before I can review your code, please include it in the main patch, and please add it in the list of test files, see browser/devtools/sourceeditor/test/Makefile.in.

Also, please run your test and only submit it once it passes on your system.

I agree that you should ask for feedback before making the changes specific to Macs, but I'd like to make sure you have properly followed the steps to write a test: (1) include in the main patch; (2) include in Makefile.in; (3) run the test on your system; (4) submit once you get the test passing on your system.

For more details, please see:
https://developer.mozilla.org/en/Browser_chrome_tests


Thanks again!

Comment 21

[Mihai Sucan [:msucan]]

Comment on attachment 601174 [details]
Initial test case

I looked into the JS code and by the looks of it this is a well written test! Great work Allen!

I was not able to run the test because it's not in the main patch, but the direction, the approach you used to write the code is really good. I think you can go forward and include the Mac-specific tweaks in the patch you'll submit.

Comment 22

[Mihai Sucan [:msucan]]

Comment on attachment 601172 [details] [diff] [review]
Move Lines Up/Down Version 3

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

This looks good. Thanks Allen!

::: browser/devtools/sourceeditor/source-editor-orion.jsm
@@ +152,5 @@
> +    action: "Move Lines Down",
> +    code: Ci.nsIDOMKeyEvent.DOM_VK_DOWN,
> +    alt: true,
> +  });
> +}

Since it's only the accel that's different for Macs, you can simplify the code: just modify the DEFAULT_KEYBINDINGS constant array to include the two new shortcuts, and do accel: Serivces.appInfo.OS == "Darwin".

Comment 23

[Mihai Sucan [:msucan]]

Allen: please rebase your patch on top of the latest mozilla-central (or fx-team) repo.

Comment 24

[Allen Eubank]

Attachment: Full patch w/ test case (includes update for Mac shortcut)

Comment 25

[Allen Eubank]

Attachment: Full patch w/ test case (includes update for Mac shortcut)

The first patch DEFAULT_KEYBINDINGS doesn't have to be changed from "const" declaration to "let". Had to re-upload to keep the original "const" declaration. Sorry!

Comment 26

[Mihai Sucan [:msucan]]

Comment on attachment 601400 [details] [diff] [review]
Full patch w/ test case (includes update for Mac shortcut)

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

Thanks for your patch. Great work! All the tests pass.

This is really close to being ready to land. See the comments below.

::: browser/devtools/sourceeditor/source-editor-orion.jsm
@@ +133,5 @@
> +  },
> +  {
> +    action: "Move Lines Down",
> +    code: Ci.nsIDOMKeyEvent.DOM_VK_DOWN,
> +    accel: Services.appinfo.OS == "Darwin",

Just noticed that we don't have ctrl modifier support here. Can you please add it?

accel means 'ctrl' on Windows and Linux, but it means 'cmd' on Macs. So this is broken.

Please use ctrl: Services.appinfo.OS == "Darwin". Then in _onIframeLoad() there's a forEach loop going through the keys array. Change the line that creates the KeyBinding object to:

  // In Orion mod1 refers to Cmd on Macs and Ctrl on Windows and Linux. So, if ctrl is in aKey we use it on Windows and Linux, otherwise we use aKey.accel for mod1.
  let mod1 = OS != "Darwin" && "ctrl" in aKey ? aKey.ctrl : aKey.accel;
  let binding = new KeyBinding(aKey.code, mod1, aKey.shift, aKey.alt, aKey.ctrl);

That will add support for the ctrl modifier.

Lastly, please update source-editor.jsm, search for the SourceEditor.DEFAULTS, the keys property. Add the 'ctrl' modifier to the list of allowed modifiers in the comment.

Thanks!

@@ +940,5 @@
> +   * Move lines upwards or downwards, relative to the current caret location.
> +   *
> +   * @private
> +   * @param boolean aLineAbove
> +   *        True if moving lines up.

True if you want to move the lines up, false to move the lines down.

@@ +942,5 @@
> +   * @private
> +   * @param boolean aLineAbove
> +   *        True if moving lines up.
> +   */
> +  _moveLines: function SE__moveLines(aLineAbove)

nit: I just noticed this is a private method between two public methods. It would make sense to put it where private methods are defined, say after _doEnter().

Also, please make sure this code doesn't execute when the editor is in readonly mode. First do a check like if (this.readOnly) { return false }.

::: browser/devtools/sourceeditor/test/browser_bug725618_moveLines_shortcut.js
@@ +43,5 @@
> +
> +function editorLoaded()
> +{
> +  let OS = Cc["@mozilla.org/xre/app-info;1"].getService(Ci.nsIXULRuntime).OS;
> +  info("Testing under this OS="+OS);

Is this info() call needed?

Generally, please add spaces between operators.

@@ +51,5 @@
> +
> +  EventUtils.synthesizeKey("VK_DOWN",
> +                            {altKey: true,
> +                            ctrlKey: OS == "Darwin"},
> +                            testWin);

It would be simpler to have a let modifiers = {...} object and reuse it for each synthesizeKey() call.

@@ +113,5 @@
> +                            testWin);
> +  is(editor.getText(), "target\nfoo\nbar",
> +      "Check for top of editor works with multiple line selection");
> +
> +  finish();

Please include tests that make sure the keyboard shortcuts have no effect after you set editor.readOnly = true.

Comment 27

[Allen Eubank]

Attachment: Full patch w/ test case V2

Addressed comments from version 1. Test passed on my machine. 

Is there a way to test the Mac shortcut on my machine? That is one thing that I am not completely sure if it works. Any suggestions Mihai?

Comment 28

[Mihai Sucan [:msucan]]

Thanks for the updated patch!


(In reply to Allen Eubank from comment #27)
> Created attachment 602174 [details] [diff] [review]
> Full patch w/ test case V2
> 
> Addressed comments from version 1. Test passed on my machine. 
> 
> Is there a way to test the Mac shortcut on my machine? That is one thing
> that I am not completely sure if it works. Any suggestions Mihai?

You usually can't do that. When I really want to test a Mac-specific behavior, I change the Orion code to always return isMac true and isLinux/isWin false, and so on. Basically, I temporarily "break" the code to execute the stuff I need. Then I also make similar changes in the tests I want to run.

We will push this patch to the Mozilla try servers which will run it on Macs, Windows and Linux machines, and we'll see if it runs well. I expect it does. ;)

Comment 29

[Mihai Sucan [:msucan]]

Comment on attachment 602174 [details] [diff] [review]
Full patch w/ test case V2

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

This is great! Thanks for your patch Allen!

This is good to land. I will land it this week!

Comment 30

[Mihai Sucan [:msucan]]

Attachment: [in-fx-team] updated patch

Updated the patch with some minor polish. It had some failures on Windows machines, again due to line ends (\r\n versus \n). I fixed the issue.

Green try results:
https://tbpl.mozilla.org/?tree=Try&rev=5a3acac08418

Landed in fx-team:
https://hg.mozilla.org/integration/fx-team/rev/5d523da125b0

Thanks a lot for your contribution Allen!

Comment 31

[Rob Campbell [:rc] (:robcee)]

https://hg.mozilla.org/mozilla-central/rev/5d523da125b0