[CLOSED] Fix #3130: Move line up/down has incorrect behavior in edge cases in inline editor

[core-ai-bot]

Issue by TomMalbran
Sunday Mar 24, 2013 at 07:46 GMT
Originally opened as adobe/brackets#3233

---

Added 2 special cases making it possible to move the second to last line to the last position and to move the last line up without shrinking the editor.

Currently it would be possible to move the last line to end of file or the line before the end of file, but isn't possible to move before the first line since the TexRange doesn't update to reduce the first line of the inline editor. Anyway, this makes it work better than before and maybe with a new implementation of inline editors, we could remove all the restriction and special cases on inline editors.

---

TomMalbran included the following code: https://github.com/adobe/brackets/pull/3233/commits

[core-ai-bot]

Comment by peterflynn
Monday Mar 25, 2013 at 19:35 GMT

---

We should definitely add unit tests for these cases too. See usage of makeEditorWithRange() in EditorCommandHandlers-test for examples of testing edit commands within inline editors.

[core-ai-bot]

Comment by TomMalbran
Monday Mar 25, 2013 at 23:59 GMT

---

Sure, I will add some tests here.

[core-ai-bot]

Comment by peterflynn
Tuesday Mar 26, 2013 at 04:05 GMT

---

Thanks!

[core-ai-bot]

Comment by TomMalbran
Tuesday Mar 26, 2013 at 08:23 GMT

---

I started working on the tests, but makeEditorWithRange doesn't work for all the cases, since the checks if it is working in an inline editor or not by checking if EditorManager.getFocusedInlineWidget() is not null, and with makeEditorWithRange it will always return null.

I also tried to see if it worked without any special cases, and it does, so even fixing that, these wouldn't make good test cases. Is there another way of really testing an inline editor without creating a window?

[core-ai-bot]

Comment by TomMalbran
Wednesday Mar 27, 2013 at 00:58 GMT

---

I ended creating a window with an inline editor to do the tests, and used the same window for all the tests, so it doesn't make the tests that much slower. If there is a better alternative, I'll change the tests.

[core-ai-bot]

Comment by jasonsanjose
Tuesday Apr 09, 2013 at 04:40 GMT

---

Assigned to me. Tagging bug #3130.

[core-ai-bot]

Comment by jasonsanjose
Tuesday Apr 09, 2013 at 05:07 GMT

---

Initial review complete

[core-ai-bot]

Comment by TomMalbran
Tuesday Apr 09, 2013 at 06:54 GMT

---

@jasonsanjose Fixes after initial review pushed.

[core-ai-bot]

Comment by jasonsanjose
Wednesday Apr 17, 2013 at 00:20 GMT

---

@TomMalbran one more suggestion on the unit tests before merging.

[core-ai-bot]

Comment by TomMalbran
Wednesday Apr 17, 2013 at 00:47 GMT

---

@jasonsanjose Fix pushed. I used the same solution as in the CommandOptionHandlers tests to close the documents.

[core-ai-bot]

Comment by jasonsanjose
Wednesday Apr 17, 2013 at 00:58 GMT

---

Looks good. Merging.