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

Conversation

bfil
Copy link
Contributor

@bfil bfil commented Jul 19, 2012

@ghost ghost assigned peterflynn Jul 19, 2012
@peterflynn
Copy link
Member

@shadowcloud: this looks great, very thorough! I'll take a stab at reviewing this by early next week.

@@ -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.

@peterflynn
Copy link
Member

@shadowcloud: done reviewing. Please let me know what you think.

And thanks again for this! It's always exciting to get a contribution including its own unit tests :-)

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.

@peterflynn
Copy link
Member

@shadowcloud: Thanks for your patience! Just a few more points to address before we merge this.

@bfil
Copy link
Contributor Author

bfil commented Jul 28, 2012

@peterflynn: Everything should be in place as expected now, great code review.

@peterflynn
Copy link
Member

Great, looks good to go! Thanks again for the contribution -- hopefully the first of many ;-)

peterflynn added a commit that referenced this pull request Jul 31, 2012
Command to shift line(s) up/down
@peterflynn peterflynn merged commit f8349ae into adobe:master Jul 31, 2012
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants