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 4 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
3 changes: 3 additions & 0 deletions src/command/Menus.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -856,6 +856,9 @@ define(function (require, exports, module) {
menu.addMenuItem(Commands.EDIT_INDENT, [{key: "Indent", displayKey: "Tab"}]);
menu.addMenuItem(Commands.EDIT_UNINDENT, [{key: "Unindent", displayKey: "Shift-Tab"}]);
menu.addMenuItem(Commands.EDIT_DUPLICATE, "Ctrl-D");
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.EDIT_LINE_COMMENT, "Ctrl-/");
menu.addMenuDivider();
menu.addMenuItem(Commands.TOGGLE_USE_TAB_CHARS);
Expand Down
87 changes: 87 additions & 0 deletions src/editor/EditorCommandHandlers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,12 @@ define(function (require, exports, module) {
EditorManager = require("editor/EditorManager");


/**
* List of constants
*/
var DIRECTION_UP = -1;
var DIRECTION_DOWN = +1;

/**
* Add or remove line-comment tokens to all the lines in the selected range, preserving selection
* and cursor position. Applies to currently focused Editor.
Expand Down Expand Up @@ -152,6 +158,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. The cursor/selection
* moves with the line/lines.
* @param {Editor} editor - target editor
* @param {Number} direction - direction of the move (-1,+1) => (Up,Down)
*/
function moveLine(editor, direction) {
editor = editor || EditorManager.getFocusedEditor();
if (!editor) {
return;
}

var doc = editor.document,
sel = editor.getSelection(),
originalSel = 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};
}

// Make the move
switch (direction) {
case DIRECTION_UP:
if (sel.start.line !== 0) {
var prevText = doc.getRange({ line: sel.start.line - 1, ch: 0 }, sel.start);

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

doc.replaceRange("", { line: sel.start.line - 1, ch: 0 }, sel.start);
doc.replaceRange(prevText, { line: sel.end.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.

We should wrap this in a doc.batchOperation() call. In this case, include the setSelection() code within that block too.

Sorry, I should have caught this in the original code review.


// fixes selection problems
Copy link
Member

Choose a reason for hiding this comment

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

It'd be good to be more specific about the problem. Something like "make sure CodeMirror hasn't expanded the selection to include the line we inserted below."

originalSel.start.line--;
originalSel.end.line--;
editor.setSelection(originalSel.start, originalSel.end);
}
break;
case DIRECTION_DOWN:
if (sel.end.line < editor.lineCount()) {
var nextText = doc.getRange(sel.end, { line: sel.end.line + 1, ch: 0 });

var deletionStart = sel.end;
if (sel.end.line === editor.lineCount() - 1) {
nextText += "\n";

var lastSelectedLine = doc.getLine(sel.end.line - 1);
if (lastSelectedLine !== undefined) {
Copy link
Member

Choose a reason for hiding this comment

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

Is there any case where lastSelectedLine would be undefined? It seems like that can only happen if the line where the cursor/selection lies doesn't exist... which presumably is impossible.

deletionStart = { line: sel.end.line - 1, ch: lastSelectedLine.length };
}
}

doc.replaceRange("", deletionStart, { line: sel.end.line + 1, ch: 0 });
doc.replaceRange(nextText, { line: sel.start.line, ch: 0 });
}
break;
}
}

/**
* Moves the selected text, or current line if no selection, one line up. The cursor/selection
* moves with the line/lines.
*/
function moveLineUp(editor) {
moveLine(editor, DIRECTION_UP);
}

/**
* 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.

moveLine(editor, DIRECTION_DOWN);
}

/**
* Indent a line of text if no selection. Otherwise, indent all lines in selection.
Expand Down Expand Up @@ -182,4 +267,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
Loading