Add feature. Open line above/below the current line.

[zeis]

Keyboard shortcus are Ctrl-Enter and Ctrl-Shift-Enter

[peterflynn]

I wonder if this feature is common enough that we'd want it in core, or if it'd be better to have it as an optional extension... It seems easy to move out into an extension if desired. It does seem to be in Sublime, though, so that's one data point in favor of core.

If we're going to take it in core, can you add unit tests?

[dangoor]

+1 this is actually a feature I miss regularly.

[zeis]

This feature is common. It is also in Visual Studio (with the same key bindings), Vim and Emacs.

I've started writing unit tests.

But, I've a question: How do I set (or get) the number of spaces for indentation before I run my tests?
For example in the following code I use lines.splice(1, 0, "    "); to reproduce 4 spaces, but the test fails because strangely it seems like indentation is set to 1 space when running tests.

    describe("Open Line Above and Below", function () {
        beforeEach(setupFullEditor);

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

            CommandManager.execute(Commands.EDIT_OPEN_LINE_ABOVE, myEditor);

            var lines = defaultContent.split("\n");
            lines.splice(1, 0, "    ");
            var expectedText = lines.join("\n");

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

[dangoor]

There was a test in the test suite that was setting indentation to 1 space. I just removed that and it was merged yesterday morning. You might try merging from master and see if the problem clears up.

If I'm reading things right, you should be able to find out what the indentation is set to by calling myEditor.getIndentUnit.

[zeis]

Thanks dangoor.

I've added unit tests.

[peterflynn]

Two quick thoughts:

• Editor commands often run into problems at the edge cases: first/last line in the file, and first/last visible line in an inline editor's range. Please test those cases carefully with both commands to make sure they do the right thing... and ideally, add unit tests for them too.
• In the unit tests, the code that generates spaces is duplicated all over the place. I think you could simply compute that once and store it in the describe()'s closure for use in all the tests (or at the least, move the code to a shared utility function that every test calls).

[dangoor]

It's great to see all of those unit tests. Thanks!

One other note: someone on the Brackets core team noted that the Edit menu is getting rather long (and we don't have submenus yet to help with that). Because of that, we should probably land this with just key bindings and not menu entries.

[zeis]

@peterflynn
I didn't actually know of editrors with range, and now I fixed an unwanted behaviour with the inline editor.

• Additional first/last line and inline editor's unit tests have been added.
• Indentation is now generated once.
• Feature's code refactored.

@dangoor

• Removed menu entries and left strings in case they'll be used in future.

[dangoor]

These keyboard shortcuts are actually the reverse of what I'd expect. ctrl-enter for open line below and ctrl-shift-enter for open line above. I just fired up Sublime 3 and it agrees with me. From my own usage, I've probably opened lines below 10 times for every time I've opened above...

[zeis]

I agree with you on this. I also open lines below more frequently than above. I just kept the same shortcuts as in Visual Studio. I never used Sublime Text.
In the last commit they have been inverted.

[dangoor]

I noticed that opening a line below at the bottom of an inline (CSS) editor doesn't work. This is not a big deal, because you can't really add rules using the existing inline CSS editor, but I thought I'd mention it in case this is not expected behavior. (edit: also doesn't work in JS inline editor...)

[dangoor]

should include the jsdoc for the editor argument

[dangoor]

Nice implementation and I appreciate all of the tests. (I'm looking forward to having this myself!)

Looking at all of that test code, I kept thinking "there's got to be a way to boil this down a bit", but looking at the code that varies between tests there's really only a couple of lines that are truly duplicated from test to test, so it may be best as it is, which is nice and straightforward.

Thanks for pulling this together!

[zeis]

What do you mean when you say "opening a line below at the bottom of an inline (CSS) editor doesn't work"? How can I reproduce the problem? That problem should have already been fixed in the second to last commit (9d4a186).
Now, before the last commit, I've rebased against upstream/master and tested again. To me it seems to work as expected. Maybe, there's an unexpected behavior with something that appears normal to me... can you explain better?

In order to make unit tests are a bit lighter, the following 3 lines could be put within a function which would take the slice() input parameters. But I'm not really sure if it is a nicer solution, so I'm not going to changed that before hearing your opinion.

var lines = defaultContent.split("\n");
lines.splice(1, 0, indentation);
var expectedText = lines.join("\n");

[dangoor]

Thanks for the update.

Regarding the tests, I came to the same conclusion you did: it's not worth trying to extract those 3 lines into a function.

Here is how you can reproduce the (minor) issue that I see:

1. open test/smokes/citrus completed/index.html
2. go to the tag (put the cursor inside the tag)
3. press ctrl/cmd-E (to open the inline editor)
4. go to the last line
5. press ctrl/cmd-enter to open a line below
6. a line is added at the correct place in the file, but the inline editor does not change height so you can't see the new line unless you go to a full editor for that CSS file

Like I said, though, this is a small thing and the feature generally works well for me.

I'm not going to merge it in right now, because we're gearing up for the next release and I don't want to introduce risk. There's a big change coming into the repo (codemirror v3) which may require a bit of merging for this.

One tip: once you've pushed to a public repo, it's better to not rebase. It's fine rebasing until your code is public, merge as necessary after that.

[zeis]

This is definitely an issue.
I didn't notice it before because I've been using a html page with a single css file to test it. And I figured out that maybe it doesn't happen if you have only one css file (see code below).

But that's really strange, because while trying to figure out what could the real problem be I also tested the MoveLine command and noticed a strange behavior as well:

1. Open test/smokes/citrus completed/index.html
2. Open the inline editor
3. Move one line down until it reaches the bottom of the inline editor. Now the inline editor automatically closes.

But, if you repeat the steps with the following code it doesn't happen and the inline editor doesn't close.
Could you confirm that it's a strange behavior?
And if not, does this issue with the last line of an inline editor affect only the OpenLine code?

HTML:

<!doctype html>
<html>
<head>
    <title>Testing</title>
    <link rel="stylesheet" href="style.css">
</head>
<body>
    <div class="myclass"></div>
</body>
</html>

CSS:

.myclass {
    height: 30 px;
    width: 50 px;
}

[TomMalbran]

There is already an issue for the Move Line Up/Down in the inline editors #1933. I tried a fix on that one by not allowing to move past the last line. There seems to be an issue in the inline editors when replacing beyond the last/first line.

[zeis]

Thanks @TomMalbran. So I'll wait for @dangoor's confirmation that the problem is not related to my code.

By the way, thanks @dangoor for the rebase tip. Actually, I had some troubles pushing the last commit. But, apart from your notes I don't recall of any actual new changes to the repo until that moment... what went wrong?

[dangoor]

There's a change coming to CodeMirror that will likely alter some of how inline editors work. I don't know if that will automatically correct problems at the boundaries of inline editors, but it may.

@zeis I can't say what may have gone wrong beyond the rebase. I was able to get your code running on my machine so I wasn't too worried about it. Now that sprint 20 has ended (and codemirror v3 has landed), I'm going to merge this change.

[dangoor]

@zeis actually, before I can merge your change, we need to have a contributor license agreement from you. Can you fill this out for us? http://dev.brackets.io/brackets-contributor-license-agreement.html

Thanks!

[zeis]

@dangoor done.

[dangoor]

I was just about to merge this and then ran into a problem... This could be related to the CodeMirror v3 integration, which happened between the time you created this feature and now.

Here are my steps to reproduce:

1. open test/smokes/citrus completed/css/desktop.css (I usually quickopen "citdes")
2. go to the middle of any line
3. press cmd/ctrl-enter to open a line
4. press cmd/ctrl-z to undo
5. the cursor goes to the old position
6. press cmd/ctrl-z again
7. the new line is removed

There were some changes in CodeMirror 3 with respect to undo and multiple operations. Do you think you can look into this?

[TomMalbran]

@zeis Hi, I was checking this request again and the problem with the selection in inline editors is that this line should be moved outside of the batch operations. See the comments on all the other functions where this was changed to fix the selection issue on the inline editors.

[TomMalbran]

With those 2 changes, I think this should work well, since the selection was one of the last broken things, and from some of the testing I did and the tests from the request, everything seems to work well.

[zeis]

I already tried it before, but something has been changed and now it works! Thanks @TomMalbran

[njx]

Marking [OPEN] for committers to review. @TomMalbran - since you've already been involved in this, would you like to do the final review and merge?

[TomMalbran]

Sure, i'll give it a final review. I think the code works good now.

[TomMalbran]

You could actually just do var indentation = (new Array(indentUnit + 1)).join(" ") to get the indentUnit spaces.

[zeis]

It raises the JSLint literal notation error.

[TomMalbran]

Woops, I just noticed that @dangoor made the same suggestion. Lets leave it like that then.

[TomMalbran]

Since you can actually write additional selectors inside the CSS inline editor or new functions inside the JS inline editor by pressing enter on the last line and start writing, I think it would be nice if open line would work in the last line of inline editors (add the line and show it in the editor).

The problem here is similar to the one I fixed in the move line, you need to add the new content before the last line break, so I tested with this code and it works for the last line:

doc.replaceRange("\n", {line: line - 1, ch: doc.getLine(line - 1).length});
cm.indentLine(line);

So, we could fix this with a special case that would use {line: line - 1, ch: doc.getLine(line - 1).length} for this case and {line: line; ch 0} for every other case.

[zeis]

It makes sense. I added a special case for the last line in inline editor.

[TomMalbran]

Great. I am seeing one last problem. The line doesn't gets indented in the inline editors. I found out that the change from the line indentation isn't getting added to the changes list for some reason so the inline editor doesn't know about it. I don't know why this doesn't work like this since the CodeMirror add new line and indent command does something similar and works. But I found a solution replacing the current doc.batchOperation block with:

if (line > lastLine && isInlineWidget) {
    doc.replaceRange("\n", {line: line - 1, ch: doc.getLine(line - 1).length}, null, "+input");
} else {
    doc.replaceRange("\n", {line: line, ch: 0}, null, "+input");
}
cm.indentLine(line, "smart", false);

Using "+input" makes it so that it joins the changes in the undo history but can be shown as separate in the changes list.

Since makeEditorWithRange doesn't actually mocks an inline editor well enough, since it doesn't deal with the synchronization which is what brings most of the problems, I was thinking if it would be better to handle the test in editors with range in an actual inline editor. If you think we should, check on the Move Line Up/Down in inline editors tests, maybe we could move the beforeEach and afterEach code to a new function that could be used by both tests. In this new tests a test inside the range would be needed to check the smart indentation.

[zeis]

I tested your changes, and everything seems to work well.
I also followed your suggestion to move the beforeEach and afterEach code into two helper functions.
I added two more tests to check the indentation inside the inline editor, and decided to rewrite all the inline editor tests to use the actual inline editor mock in order to test the recent changes properly.
Let me know.

[TomMalbran]

Can you align this strings, and move the testPath to the first describe: describe("EditorCommandHandlers" and remove it from describe("Move Lines Up/Down - inline editor" since it doesn't need to be called twice.

[TomMalbran]

Nice work. This is working great now. It is now joining several commands executions as one in the undo history, when pressing Shift+Enter fast enough. So after adding several new lines really fast undoing will undo all of them, but using Enter to create new lines works in the same way, so I think that is ok.

I just have one final comment and I think we can then merge it, but since there are new strings I am not sure if we can merge it in this Sprint or it might need to wait for the start of the next one. So, thanks for sticking with this and fixing everything so fast :)

@jasonsanjose I saw you posted some messages about not merging some pull request with new strings in them, so I was wondering it that would apply to this one too? I haven't seen any UI freeze comment yet and so I am not sure.

[zeis]

Those strings are actually not used, since menu entries have been removed. I thought they could be useful, but for now there is no reason to keep them there, so I removed Italian strings. English ones seem to be necessary to make the commands work.

[TomMalbran]

Great, thanks. I know the string aren't used, but they still get translated. Maybe in the future those string might be used for something.

I'll wait a bit to hear from @jasonsanjose or @njx to know if we are still in time to merge this pull request in this sprint.

[jasonsanjose]

Thanks @TomMalbran for checking with us. We discussed at our daily meeting and we agreed to land this for sprint 24. Please go ahead and merge.

[TomMalbran]

Great. Thanks. Merging :)