[CLOSED] Add feature. Open line above/below the current line.

[core-ai-bot]

Issue by zeis
Wednesday Jan 30, 2013 at 23:52 GMT
Originally opened as adobe/brackets#2729

---

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

---

zeis included the following code: https://github.com/adobe/brackets/pull/2729/commits

[core-ai-bot]

Comment by peterflynn
Wednesday Jan 30, 2013 at 23:59 GMT

---

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?

[core-ai-bot]

Comment by dangoor
Thursday Jan 31, 2013 at 01:49 GMT

---

+1 this is actually a feature I miss regularly.

[core-ai-bot]

Comment by zeis
Thursday Jan 31, 2013 at 10:54 GMT

---

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});
        });
    });

[core-ai-bot]

Comment by dangoor
Thursday Jan 31, 2013 at 13:05 GMT

---

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.

[core-ai-bot]

Comment by zeis
Thursday Jan 31, 2013 at 15:56 GMT

---

Thanks dangoor.

I've added unit tests.

[core-ai-bot]

Comment by peterflynn
Thursday Jan 31, 2013 at 19:15 GMT

---

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

[core-ai-bot]

Comment by dangoor
Thursday Jan 31, 2013 at 20:03 GMT

---

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.

[core-ai-bot]

Comment by zeis
Friday Feb 01, 2013 at 09:25 GMT

---

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

[core-ai-bot]

Comment by dangoor
Monday Feb 11, 2013 at 16:46 GMT

---

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

[core-ai-bot]

Comment by dangoor
Monday Feb 11, 2013 at 17:09 GMT

---

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!

[core-ai-bot]

Comment by zeis
Tuesday Feb 12, 2013 at 00:42 GMT

---

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");

[core-ai-bot]

Comment by dangoor
Tuesday Feb 12, 2013 at 21:00 GMT

---

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.

[core-ai-bot]

Comment by zeis
Tuesday Feb 12, 2013 at 22:59 GMT

---

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;
}

[core-ai-bot]

Comment by TomMalbran
Tuesday Feb 12, 2013 at 23:08 GMT

---

There is already an issue for the Move Line Up/Down in the inline editors adobe/brackets#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.

[core-ai-bot]

Comment by zeis
Wednesday Feb 13, 2013 at 00:44 GMT

---

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?