725375 - Add feature to sourceeditor/scratchpad: duplicate line(s)

Status: RESOLVED WONTFIX

(DevTools :: General, defect)

Description

[Johan C]

Duplicating a line or lines of code can be extremely useful, and it could prove a great addition to the sourceeditor/Scratchpad.

Use case 1:
1) Place caret cursor/marker on a line
2) Use "Duplicate Current Line" or the the keyboard shortcut.
3) A duplicate line is inserted below the current line.

Use case 2:
1) Select some text.
2) Use "Duplicate Current Line"/"Duplicate Currect Selection" or the the keyboard shortcut.
3) The selected text is duplicated and inserted at the end of the current selection.

Example of keyboard shortcut (windows): "ctrl-D" (used in notepad++).

Comment 1

[Mihai Sucan [:msucan]]

How to:

1. Get the latest code from the fx-team repo [1] and build Firefox [2].
2. Open up browser/devtools/sourceeditor/source-editor-orion.jsm
3. Add a keyboard shortcut of your choice (at the moment) for a new action (a new method), say _duplicateLines(), and write the code of the function.

In your code you need to use the Orion API (TextView and TextModel) - see through the JSM source code how that works, and also checkout the Orion jsdocs. [3]

After all the code changes you might need to rebuild Firefox, depending on your system. On my system, Linux, I don't need to do that.

When done, attach the patch [4] to the bug and ask for feedback from me (set the feedback flag to "?" and write :msucan in the input).

Please test your code in Scratchpad, to see if the new keyboard shortcut works as desired.

Once we are done with this step, we will look into writing a test. [5] But in the interest of keeping things simple, let's go through a round of feedback before you write the test.

Thank you very much for your interest to help us out! This is awesome! Good luck and have fun learning!


[1] https://developer.mozilla.org/en/Mozilla_Source_Code_(Mercurial)
[2] https://developer.mozilla.org/En/Simple_Firefox_build
[3] http://orion.eclipse.org/jsdoc/
[4] https://developer.mozilla.org/En/Creating_a_patch
[5] https://developer.mozilla.org/en/Browser_chrome_tests

Comment 2

[Leonard Camacho [:lcamacho]]

Attachment: Duplicate text

Comment 3

[Mihai Sucan [:msucan]]

Comment on attachment 596645 [details] [diff] [review]
Duplicate text

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

This is great Leonard! Thanks a lot for your contribution! The patch applies cleanly and it works really well.

Some general comments:
- make it two actions to copy lines up and lines down.
- make it always copy lines, even if there's a selection that spans multiple lines. Don't just duplicate the selected text.
- make sure the code only inserts text - currently undo/redo shows that that text changed more than needed.

Really good work for a first-patch inside the Source Editor! Please address the comments. I am looking forward for the updated patch! Thanks!

::: browser/devtools/sourceeditor/source-editor-orion.jsm
@@ +112,5 @@
> +    action: "Duplicate text",
> +    code: Ci.nsIDOMKeyEvent.DOM_VK_C,
> +    accel: true,
> +    shift: true,
> +  },

Please add two actions: Copy lines down and Copy lines up. For the first action use the Ctrl+Alt+Down keyboard shortcut, and for the second action use Ctrl+Alt+Up. These are the keyboard shortcuts that are also used in Orion upstream.

@@ +658,5 @@
> +   * Duplicate the selected text and insert after the selection
> +   * or if there's no selection duplicate the actual line and insert
> +   * below.
> +   */
> +  _duplicateText: function SE_duplicateText()

This method works as advertised!

However I have some suggestions for improvements: don't overwrite the whole text from start to end. When you duplicate a line then you press undo the selection will be for all the text you change. So, insert the new text only at the desired offset. Play with undo/redo to see the when it works well.

@@ +678,5 @@
> +      text = this.getSelectedText();
> +    }
> +
> +    caretOffset = start + text.length;
> +    text = text + this.getText(start);

You don't need to do text + getText()

@@ +682,5 @@
> +    text = text + this.getText(start);
> +
> +    if (text != '') 
> +    {
> +      this.setText(text, start);

This will update the whole document from the start offset to the end. Just do this.setText(text, start, start) to put it where you want.

Comment 4

[Leonard Camacho [:lcamacho]]

Thanks for the feedback
 
> - make it always copy lines, even if there's a selection that spans multiple
> lines. Don't just duplicate the selected text.

I don't understand this, use case #2 says duplicate the text selected but what are you saying is no matter if I select some text it should always duplicate the complete line?

What It should happen if my selection starts at the middle of a line to the middle of the next one?
 
> Please add two actions: Copy lines down and Copy lines up. For the first
> action use the Ctrl+Alt+Down keyboard shortcut, and for the second action
> use Ctrl+Alt+Up. These are the keyboard shortcuts that are also used in
> Orion upstream.

These shortcuts will collide with the shortcuts to switch workspaces on Ubuntu.

Comment 5

[Mihai Sucan [:msucan]]

(In reply to Leonard Camacho [:lcamacho] from comment #4)
> Thanks for the feedback
>  
> > - make it always copy lines, even if there's a selection that spans multiple
> > lines. Don't just duplicate the selected text.
> 
> I don't understand this, use case #2 says duplicate the text selected but
> what are you saying is no matter if I select some text it should always
> duplicate the complete line?

That is correct. The bug summary is not clear enough.

> What It should happen if my selection starts at the middle of a line to the
> middle of the next one?

In such cases the two lines are duplicated entirely, above or below the caret location. 

> > Please add two actions: Copy lines down and Copy lines up. For the first
> > action use the Ctrl+Alt+Down keyboard shortcut, and for the second action
> > use Ctrl+Alt+Up. These are the keyboard shortcuts that are also used in
> > Orion upstream.
> 
> These shortcuts will collide with the shortcuts to switch workspaces on
> Ubuntu.

Unfortunately, that's true, but I don't have better suggestions. I picked them because the Orion IDE uses them and they work on Windows and Mac (if I am not mistaken). Can you please make suggestions for better keyboard shortcuts?

Thanks a lot!

Comment 6

[Leonard Camacho [:lcamacho]]

Attachment: Duplicate text V2

Comment 7

[Mihai Sucan [:msucan]]

Comment on attachment 597367 [details] [diff] [review]
Duplicate text V2

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

Awesome! This works much better. Thanks for your update!

See more comments below:

- you should add your name to the contributors list!

::: browser/devtools/sourceeditor/source-editor-orion.jsm
@@ +114,5 @@
> +    accel: true,
> +    shift: true,
> +  },
> +  {
> +    action: "Duplicate lines insert below",

Please rename the actions to "Duplicate lines above" and "Duplicate lines below".

@@ +117,5 @@
> +  {
> +    action: "Duplicate lines insert below",
> +    code: Ci.nsIDOMKeyEvent.DOM_VK_DOWN,
> +    accel: true,
> +    shift: true,

Ctrl-Shift is used for making word by word selections (Left/Right) and whole-line selections when combined with Up/Down.

I would still pick Ctrl-Alt-Up/Down, unless better suggestions are made.

@@ +308,5 @@
>        "Find Next Occurrence": [this.ui.findNext, this.ui],
>        "Find Previous Occurrence": [this.ui.findPrevious, this.ui],
>        "Goto Line...": [this.ui.gotoLine, this.ui],
> +      "Duplicate lines insert over": [this._duplicateLinesOver, this],
> +      "Duplicate lines insert below": [this._duplicateLines, this],

You don't need _duplicateLinesOver(). You can do: this._duplicateLines.bind(this, true).

@@ +673,5 @@
> +  /**
> +   * Duplicate the lines or line that have some text selected, 
> +   * if there's no selection duplicate the actual line.
> +   * The duplicates can be inserted over or below the line 
> +   * where the caret is located.

s/The duplicates/The duplicated lines/

@@ +679,5 @@
> +   * @param boolean insertOver
> +   *        Indicates if the duplicate should be inserted over the line
> +   *        where the caret is located.
> +   */
> +  _duplicateLines: function SE_duplicateLines(insertOver)

Please prefix arguments with "a". (for example aInsertAbove)

@@ +692,5 @@
> +    while (lineIndex <= lineEnd)
> +    {
> +      text += this.getLineDelimiter() + this._model.getLine(lineIndex);
> +      lineIndex++;
> +    }

You can probably simplify the code like this:

let firstLine = this._model.getLineAtOffset(selection.start);
let firstLineStart = this._model.getLineStart(startLine);
let lastLine = this._model.getLineAtOffset(selection.end);
let lastLineEnd = this._model.getLineEnd(lastLine, true);
let text = this.getText(firstLineStart, lastLineEnd);

What do you think?

(the second argument for getLineEnd() tells we want the line end delimiter to be included)

@@ +701,5 @@
> +      {
> +        lineStart -= 1;
> +        start = this._model.getLineEnd(lineStart);
> +      }
> +      this.setText(text, start, start);

After you add the text please select the new text.

Comment 8

[Leonard Camacho [:lcamacho]]

Attachment: Duplicate text V3

Alt+Shift+DOWN works on Ubuntu.

Comment 9

[Leonard Camacho [:lcamacho]]

Attachment: Duplicate text V3.1

Correct name for parameter

Comment 10

[Cedric Vivier [:cedricv]]

Ctrl-D / Ctrl-Shift-D ?

IMHO this feature is not common/useful enough to reserve 'previous' directional key bindings (which can be used for navigational and adjustement features), duplicating a line with a sequence of 2 key bindings is not a deal breaker.

Also, I'm not sure sure to understand what's the difference between "duplicate above" and "duplicate below".
Isn't the end result the same ? (unless it overwrites the line it is pasted to.. but then the feature might be quite confusing/annoying)

Comment 11

[Cedric Vivier [:cedricv]]

s/'previous'/'precious'

Comment 12

[Mihai Sucan [:msucan]]

Comment on attachment 597669 [details] [diff] [review]
Duplicate text V3.1

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

Thanks for your updated patch Leonard!

Alt-Up/Down is what other editors use for moving lines up/down, so Alt-Shift-Up/Down to copy the lines kinda makes sense. Ctrl-D is already taken by Scratchpad, so we can't use this.

We keep Alt-Shift-Up/Down for now, until someone makes a better suggestion.

One bug with this code:

1. Open Scratchpad and go to line 5 and shift-down to select the whole line.
2. Then press alt-shift-down.

You get two lines copied. I think the code believes the selection extends into the 6th line. Please fix this bug.

Thanks for your awesome work! Looking forward for the updated patch!

::: browser/devtools/sourceeditor/source-editor-orion.jsm
@@ +693,5 @@
> +      if (!aLineAbove)
> +      {
> +        firstLineStart = this._model.getLineStart(lastLine + 1);
> +      }
> +      this.setSelection(firstLineStart, firstLineStart + text.length - 1);

This block of code is confusing for me as a new reader. You reuse variables defined above for different purposes.

I suggest defining new variables for insertAtOffset, newSelectionStart and newSelectionEnd.

Also, the new selection should include the line end, so I can press delete to remove the new lines entirely.

Comment 13

[Leonard Camacho [:lcamacho]]

Attachment: Duplicate text V4

Comment 14

[Mihai Sucan [:msucan]]

Comment on attachment 598092 [details] [diff] [review]
Duplicate text V4

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

Much better! Thanks for your update Leonard!

General coding style comments:
- please make sure you patch doesn't have any trailing whitespace. hg qdiff/hg diff show whitespaces in red, if you have colors enabled.

- the coding style of the file shows that curly brackets for if () { } else { } blocks are on the same line:
  if (foo) 
    bar();
  } else {
    baz();
  }

Overall this looks good. Thanks again!

You can go ahead and write a test for this new feature. The comments below are minor, easy to address. Please let me know if you need help with writing tests! You can find the current source editor tests in the same location, in the test/ subfolder. Good luck!

::: browser/devtools/sourceeditor/source-editor-orion.jsm
@@ +692,5 @@
> +      lastLineEnd = this._model.getLineEnd(lastLine);
> +      text = this.getLineDelimiter() + this.getText(firstLineStart, lastLineEnd);
> +      insertAtOffset = lastLineEnd;
> +      newSelectionStart = lastLineEnd;
> +    }

You can have this block of code execute before you set text, insertAtOffset and newSelectionStart, so you don't set them twice.

@@ +700,5 @@
> +      if (aLineAbove)
> +      {
> +        newSelectionStart = this._model.getLineStart(firstLine);
> +        firstLine -= 1;
> +        insertAtOffset = this._model.getLineEnd(firstLine);

insertAtOffset = this._model.getLineEnd(firstLine - 1);

Comment 15

[Mihai Sucan [:msucan]]

Found a bug while playing with the patch. Try Alt-Shift-Up several times in a row and Alt-Shift-Down several times in a row. When you do Alt-Shift-Down several times in a row you get more and more lines duplicated. This happens because the text that is added starts from the previous line (the new text always starts with a new line, instead of ending with a new line).

Comment 16

[Leonard Camacho [:lcamacho]]

Attachment: Duplicate text V5

Comment 17

[Mihai Sucan [:msucan]]

Comment on attachment 598541 [details] [diff] [review]
Duplicate text V5

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

This is really good work Leonard! Now it works very well.

Please go ahead and write the test. Ping me on IRC if you need any help. See:

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

Good luck! Thanks for your work and patience!

::: browser/devtools/sourceeditor/source-editor-orion.jsm
@@ +700,5 @@
> +    }
> +
> +    if (text != this.getLineDelimiter()) {
> +      this.setText(text, insertAtOffset, insertAtOffset);
> +      this.setSelection(newSelectionStart, newSelectionStart + text.length);

I think you can remove newSelectionStart and just reuse insertAtOffset here.

Comment 18

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

How is this an improvement over copying and pasting a line or lines?

Comment 19

[Greg Karz]

(In reply to Rob Campbell [:rc] (robcee) from comment #18)
> How is this an improvement over copying and pasting a line or lines?

It's much more convenient to press ctrl+d than [select line], ctrl+c, [disselect], enter, ctrl+v

Comment 20

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

I'm not sure this is a feature that's used enough to warrant the expenditure of a keyboard shortcut.