Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Product Backlog - Move Line Up/Down on Key #1282

Merged
merged 6 commits into from
Jul 31, 2012
Merged

Product Backlog - Move Line Up/Down on Key #1282

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b86ec3f
Public Product Backlog - Move Line Up/Down on Key
bfil Jul 19, 2012
bed03b5
Move Line Up/Down on Key - Bug Fix and Tests
bfil Jul 19, 2012
53ed4f0
Merge branch 'master' of github.com:adobe/brackets
bfil Jul 26, 2012
4833b4c
Move Line Up/Down on Key - Updating code and tests after review
bfil Jul 26, 2012
df6a3b7
Merge branch 'master' of https://github.com/adobe/brackets
bfil Jul 28, 2012
45034aa
Move Line Up/Down on Key - Updating menu shortcuts, using batchOperat…
bfil Jul 28, 2012
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/command/Commands.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,8 @@ define(function (require, exports, module) {
exports.EDIT_UNINDENT = "edit.unindent";
exports.EDIT_DUPLICATE = "edit.duplicate";
exports.EDIT_LINE_COMMENT = "edit.lineComment";
exports.EDIT_LINE_UP = "edit.lineUp";
exports.EDIT_LINE_DOWN = "edit.lineDown";
exports.TOGGLE_USE_TAB_CHARS = "debug.useTabChars";

// VIEW
Expand Down
2 changes: 2 additions & 0 deletions src/command/Menus.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -851,6 +851,8 @@ define(function (require, exports, module) {
menu.addMenuItem(Commands.EDIT_UNINDENT, [{key: "Unindent", displayKey: "Shift-Tab"}]);
menu.addMenuItem(Commands.EDIT_DUPLICATE, "Ctrl-D");
menu.addMenuItem(Commands.EDIT_LINE_COMMENT, "Ctrl-/");
menu.addMenuItem(Commands.EDIT_LINE_UP, {key: "Ctrl-Up", displayKey: "Ctrl-\u2191"});
menu.addMenuItem(Commands.EDIT_LINE_DOWN, {key: "Ctrl-Down", displayKey: "Ctrl-\u2193"});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The standard on Windows seems to be Ctrl+Shift+up/down (see Sublime, Notepad++, and IntelliJ/WebStorm). The only odd one out I could find is Eclipse, which uses Alt+up/down. On all of those editors, Ctrl+up/down instead scrolls the view by one line (without making any edits).

On Mac, most editors seem to lack this feature. Eclipse uses Alt+up/down, Sublime uses Cmd+Ctrl+up/down (edit: TextMate and Espresso also use this shortcut), and IntelliJ/WebStorm uses Cmd+Shift+up/down. I'd be inclined to go with Cmd+Shift since it's more consistent with the Windows shortcut (in fact Brackets will automatically convert Ctrl<->Cmd, so you wouldn't even have to declare two shortcuts in that case).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re the menus: move line up/down feels more related to Duplicate. In fact, now that this menu section is getting longer it feels weird to have Comment/Uncomment grouped in with more generic editing commands. How about reordering it so the menu looks like this?

...
Duplicate
Move Line(s) Up
Move Line(s) Down
---divider---
Comment/Uncomment Lines
---divider---
Use Tab Characters

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. In the details of the Products Backlog it was specified to use Ctrl+Up/Down as key bindings. So, I should do the following?
  • Windows: Ctrl+Shift+Up/Down
  • Mac: Alt+Up/Down
  1. I'll reorganize the menu as you've specified.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think the note in the backlog got this shortcut confused with the "scroll 1 line" feature that most editors have. Let's use Ctrl+Shift+up/down on Windows.

On Mac, I found that more of them support this than I thought. Sublime, TextMate and Espresso all use Cmd+Ctrl+up/down, so that seems like the best shortcut to use.

menu.addMenuDivider();
menu.addMenuItem(Commands.TOGGLE_USE_TAB_CHARS);

Expand Down
81 changes: 81 additions & 0 deletions src/editor/EditorCommandHandlers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,85 @@ define(function (require, exports, module) {
var selectedText = doc.getRange(sel.start, sel.end) + delimiter;
doc.replaceRange(selectedText, sel.start);
}

/**
* Moves the selected text, or current line if no selection, one line up. The cursor/selection
* moves with the line/lines.
*/
function moveLineUp(editor) {
editor = editor || EditorManager.getFocusedEditor();
if (!editor) {
return;
}

var sel = editor.getSelection(),
hasSelection = (sel.start.line !== sel.end.line) || (sel.start.ch !== sel.end.ch);

sel.start.ch = 0;
// The end of the selection becomes the start of the next line, if it isn't already
if (!hasSelection || sel.end.ch !== 0) {
sel.end = {line: sel.end.line + 1, ch: 0};
}

if (sel.start.line !== 0) {
// Make the edit
var doc = editor.document;

var currentText = doc.getRange(sel.start, sel.end);
var prevText = doc.getRange({ line: sel.start.line - 1, ch: 0 }, sel.start);

if (sel.end.line === editor.lineCount()) {
currentText += "\n";
prevText = prevText.substring(0, prevText.length - 1);
}

doc.replaceRange(currentText, { line: sel.start.line - 1, ch: 0 },
{ line: sel.end.line - 1, ch: 0 });
doc.replaceRange(prevText, { line: sel.end.line - 1, ch: 0 }, sel.end);
editor.setSelection({ line: sel.start.line - 1, ch: 0 },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two questions:

  1. Is it valuable to the user to auto-expand the selection to the full line(s)? Eclipse does this, but the other editors I tested don't (they leave the user's original selection as-is).

  2. The pair of replaceRange() calls here seems more complex than it needs to be: it's basically replacing the line above with the line below, then replacing the line below with the line above (and it gets especially tricky with multi-line selections, where the ranges start to overlap). What if we just remove the line above and then re-insert it below? That would also make it easier to preserve the selection if we wanted to do that.

You'd still need a special case for the bottommost line, but I think you could handle it just by adjusting the deletion range.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Ok, sounds fair, I will keep the original selection. In Aptana I'm used to see the full lines selected, that's why I did it in this way ;)

  2. I agree, that should be way better, I still need to get familiar with the CodeMirror APIs.

{ line: sel.end.line - 1, ch: 0 });
}
}

/**
* Moves the selected text, or current line if no selection, one line down. The cursor/selection
* moves with the line/lines.
*/
function moveLineDown(editor) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code here is very close to moveLineUp(). Is there a clean way to consolidate these? For the parts that just vary +1 vs. -1, you could pass in a "direction" argument. Then the only real diffs would be the corner case checks.

editor = editor || EditorManager.getFocusedEditor();
if (!editor) {
return;
}

var sel = editor.getSelection(),
hasSelection = (sel.start.line !== sel.end.line) || (sel.start.ch !== sel.end.ch);

sel.start.ch = 0;
// The end of the selection becomes the start of the next line, if it isn't already
if (!hasSelection || sel.end.ch !== 0) {
sel.end = {line: sel.end.line + 1, ch: 0};
}

if (sel.end.line < editor.lineCount()) {

// Make the edit
var doc = editor.document;

var currentText = doc.getRange(sel.start, sel.end);
var nextText = doc.getRange(sel.end, { line: sel.end.line + 1, ch: 0 });

if (sel.end.line === editor.lineCount() - 1) {
currentText = currentText.substring(0, currentText.length - 1);
nextText += "\n";
}

doc.replaceRange(currentText, { line: sel.start.line + 1, ch: 0 },
{ line: sel.end.line + 1, ch: 0 });
doc.replaceRange(nextText, sel.start, { line: sel.start.line + 1, ch: 0 });
editor.setSelection({ line: sel.start.line + 1, ch: 0 },
{ line: sel.end.line + 1, ch: 0 });
}
}

/**
* Indent a line of text if no selection. Otherwise, indent all lines in selection.
Expand Down Expand Up @@ -182,4 +261,6 @@ define(function (require, exports, module) {
CommandManager.register(Strings.CMD_UNINDENT, Commands.EDIT_UNINDENT, unidentText);
CommandManager.register(Strings.CMD_COMMENT, Commands.EDIT_LINE_COMMENT, lineComment);
CommandManager.register(Strings.CMD_DUPLICATE, Commands.EDIT_DUPLICATE, duplicateText);
CommandManager.register(Strings.CMD_LINE_UP, Commands.EDIT_LINE_UP, moveLineUp);
CommandManager.register(Strings.CMD_LINE_DOWN, Commands.EDIT_LINE_DOWN, moveLineDown);
});
2 changes: 2 additions & 0 deletions src/strings.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,8 @@ define(function (require, exports, module) {
exports.CMD_UNINDENT = "Unindent";
exports.CMD_DUPLICATE = "Duplicate";
exports.CMD_COMMENT = "Comment/Uncomment Lines";
exports.CMD_LINE_UP = "Move Line(s) Up";
exports.CMD_LINE_DOWN = "Move Line(s) Down";

// View menu commands
exports.VIEW_MENU = "View";
Expand Down
242 changes: 242 additions & 0 deletions test/spec/EditorCommandHandlers-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -500,6 +500,248 @@ define(function (require, exports, module) {
});
});

describe("Move Lines Up/Down", function () {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are great! Very thorough.

One more area to cover, though: please add tests for the first/last-line edge cases (moving the bottommost line up or the topmost line down). If I comment out the inner if in moveLineUp(), none of these tests will fail... but moving up the last line in the file will be totally broken.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's weird, moving up the bottom line works well for me. Anyway, I will add the tests for those edge cases.


it("should move whole line up if no selection", function () {
// place cursor in middle of line 1
myEditor.setCursorPos(1, 10);

CommandManager.execute(Commands.EDIT_LINE_UP, myEditor);

var lines = defaultContent.split("\n");
var temp = lines[0];
lines[0] = lines[1];
lines[1] = temp;
var expectedText = lines.join("\n");

expect(myDocument.getText()).toEqual(expectedText);
expectSelection({start: {line: 0, ch: 0}, end: {line: 1, ch: 0}});
});

it("should move whole line down if no selection", function () {
// place cursor in middle of line 1
myEditor.setCursorPos(1, 10);

CommandManager.execute(Commands.EDIT_LINE_DOWN, myEditor);

var lines = defaultContent.split("\n");
var temp = lines[2];
lines[2] = lines[1];
lines[1] = temp;
var expectedText = lines.join("\n");

expect(myDocument.getText()).toEqual(expectedText);
expectSelection({start: {line: 2, ch: 0}, end: {line: 3, ch: 0}});
});

it("shouldn't move up first line", function () {
// place cursor at start of line 0
myEditor.setCursorPos(0, 0);

CommandManager.execute(Commands.EDIT_LINE_UP, myEditor);

expect(myDocument.getText()).toEqual(defaultContent);
expectCursorAt({line: 0, ch: 0});
});

it("shouldn't move line down if selected line is at end of file", function () {
var lines = defaultContent.split("\n"),
len = lines.length;

// place cursor at the beginning of the last line
myEditor.setCursorPos(len-1, 0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JSLint nit: need spaces around the "-". Same thing with the last line of this function.


CommandManager.execute(Commands.EDIT_LINE_DOWN, myEditor);

var expectedText = lines.join("\n");

expect(myDocument.getText()).toEqual(expectedText);
expectCursorAt({line: len-1, ch: 0});
});

it("should move up empty line", function () {
// place cursor on line 6
myEditor.setCursorPos(6, 0);

CommandManager.execute(Commands.EDIT_LINE_UP, myEditor);

var lines = defaultContent.split("\n");
var temp = lines[5];
lines[5] = lines[6];
lines[6] = temp;
var expectedText = lines.join("\n");

expect(myDocument.getText()).toEqual(expectedText);
expectSelection({start: {line: 5, ch: 0}, end: {line: 6, ch: 0}});
});

it("should move down empty line", function () {
// place cursor on line 6
myEditor.setCursorPos(6, 0);

CommandManager.execute(Commands.EDIT_LINE_DOWN, myEditor);

var lines = defaultContent.split("\n");
var temp = lines[7];
lines[7] = lines[6];
lines[6] = temp;
var expectedText = lines.join("\n");

expect(myDocument.getText()).toEqual(expectedText);
expectCursorAt({line: 7, ch: 0});
});

it("should move up when entire line selected, excluding newline", function () {
// select all of line 1, EXcluding trailing \n
myEditor.setSelection({line: 1, ch: 0}, {line: 1, ch: 20});

CommandManager.execute(Commands.EDIT_LINE_UP, myEditor);

var lines = defaultContent.split("\n");
var temp = lines[0];
lines[0] = lines[1];
lines[1] = temp;
var expectedText = lines.join("\n");

expect(myDocument.getText()).toEqual(expectedText);
expectSelection({start: {line: 0, ch: 0}, end: {line: 1, ch: 0}});
});

it("should move down when entire line selected, excluding newline", function () {
// select all of line 1, EXcluding trailing \n
myEditor.setSelection({line: 1, ch: 0}, {line: 1, ch: 20});

CommandManager.execute(Commands.EDIT_LINE_DOWN, myEditor);

var lines = defaultContent.split("\n");
var temp = lines[2];
lines[2] = lines[1];
lines[1] = temp;
var expectedText = lines.join("\n");

expect(myDocument.getText()).toEqual(expectedText);
expectSelection({start: {line: 2, ch: 0}, end: {line: 3, ch: 0}});
});

it("should move up when entire line selected, including newline", function () {
// select all of line 1, INcluding trailing \n
myEditor.setSelection({line: 1, ch: 0}, {line: 2, ch: 0});

CommandManager.execute(Commands.EDIT_LINE_UP, myEditor);

var lines = defaultContent.split("\n");
var temp = lines[0];
lines[0] = lines[1];
lines[1] = temp;
var expectedText = lines.join("\n");

expect(myDocument.getText()).toEqual(expectedText);
expectSelection({start: {line: 0, ch: 0}, end: {line: 1, ch: 0}});
});

it("should move down when entire line selected, including newline", function () {
// select all of line 1, INcluding trailing \n
myEditor.setSelection({line: 1, ch: 0}, {line: 2, ch: 0});

CommandManager.execute(Commands.EDIT_LINE_DOWN, myEditor);

var lines = defaultContent.split("\n");
var temp = lines[2];
lines[2] = lines[1];
lines[1] = temp;
var expectedText = lines.join("\n");

expect(myDocument.getText()).toEqual(expectedText);
expectSelection({start: {line: 2, ch: 0}, end: {line: 3, ch: 0}});
});

it("should move up when multiple lines selected", function () {
// select lines 2-3
myEditor.setSelection({line: 2, ch: 0}, {line: 4, ch: 0});

CommandManager.execute(Commands.EDIT_LINE_UP, myEditor);

var lines = defaultContent.split("\n");
var temp = lines[1];
lines[1] = lines[2];
lines[2] = lines[3];
lines[3] = temp;
var expectedText = lines.join("\n");

expect(myDocument.getText()).toEqual(expectedText);
expectSelection({start: {line: 1, ch: 0}, end: {line: 3, ch: 0}});
});

it("should move down when multiple lines selected", function () {
// select lines 2-3
myEditor.setSelection({line: 2, ch: 0}, {line: 4, ch: 0});

CommandManager.execute(Commands.EDIT_LINE_DOWN, myEditor);

var lines = defaultContent.split("\n");
var temp = lines[4];
lines[4] = lines[3];
lines[3] = lines[2];
lines[2] = temp;
var expectedText = lines.join("\n");

expect(myDocument.getText()).toEqual(expectedText);
expectSelection({start: {line: 3, ch: 0}, end: {line: 5, ch: 0}});
});

it("should move up selection crossing line boundary", function () {
// select from middle of line 2 to middle of line 3
myEditor.setSelection({line: 2, ch: 13}, {line: 3, ch: 11});

CommandManager.execute(Commands.EDIT_LINE_UP, myEditor);

var lines = defaultContent.split("\n");
var temp = lines[1];
lines[1] = lines[2];
lines[2] = lines[3];
lines[3] = temp;
var expectedText = lines.join("\n");

expect(myDocument.getText()).toEqual(expectedText);
expectSelection({start: {line: 1, ch: 0}, end: {line: 3, ch: 0}});
});

it("should move down selection crossing line boundary", function () {
// select from middle of line 2 to middle of line 3
myEditor.setSelection({line: 2, ch: 13}, {line: 3, ch: 11});

CommandManager.execute(Commands.EDIT_LINE_DOWN, myEditor);

var lines = defaultContent.split("\n");
var temp = lines[4];
lines[4] = lines[3];
lines[3] = lines[2];
lines[2] = temp;
var expectedText = lines.join("\n");

expect(myDocument.getText()).toEqual(expectedText);
expectSelection({start: {line: 3, ch: 0}, end: {line: 5, ch: 0}});
});

it("shouldn't move up after select all", function () {
myEditor.setSelection({line: 0, ch: 0}, {line: 7, ch: 1});

CommandManager.execute(Commands.EDIT_LINE_UP, myEditor);

expect(myDocument.getText()).toEqual(defaultContent);
expectSelection({start: {line: 0, ch: 0}, end: {line: 7, ch: 1}});
});

it("shouldn't move down after select all", function () {
myEditor.setSelection({line: 0, ch: 0}, {line: 7, ch: 1});

CommandManager.execute(Commands.EDIT_LINE_DOWN, myEditor);

expect(myDocument.getText()).toEqual(defaultContent);
expectSelection({start: {line: 0, ch: 0}, end: {line: 7, ch: 1}});
});
});


});
});