Fix #3130: Move line up/down has incorrect behavior in edge cases in inline editor #3233
Conversation
…f an inline editor
|
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. |
|
Sure, I will add some tests here. |
|
Thanks! |
|
I started working on the tests, but 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? |
|
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. |
ghost
assigned 
|
Assigned to me. Tagging bug #3130. |
| @@ -2065,6 +2070,104 @@ define(function (require, exports, module) { | |||
| }); | |||
|
|
|||
|
|
|||
| describe("Move Lines Up/Down - inline editor", function () { | |||
| var testWindow, promise, editor; | |||
There was a problem hiding this comment.
Add this.category = "integration"; to this suite so that it runs in the integration suite instead of the unit test suite. It's our hack to keep the unit test suite running fast. All of our tests that open a Brackets window are marked as integration.
There was a problem hiding this comment.
Will do. I knew about this, but didn't knew it could be added to internal describes or if it would be needed, since the tests are still fast, and the Extension Dialog tests uses windows and are unit tests.
|
Initial review complete |
| CommandManager.execute(Commands.EDIT_LINE_DOWN, editor); | ||
|
|
||
| expect(editor.document.getText()).toEqual(moveContent); | ||
| expect(editor._codeMirror.doc.historySize().undo).toBe(1); |
There was a problem hiding this comment.
I added the _codeMirror.doc.historySize() here too since is a no-op edit too, but since there was a editor.document.setText after the last test, it has to be 1 instead of 0.
There was a problem hiding this comment.
I see. To make this consistent and expect zero, I'd rather change the afterEach() to close all documents in the working set. You could then change beforeEach to keep the same window open, but always re-create the working set and open the inline widget. Sound good?
There was a problem hiding this comment.
That is ok too, I did that for the Editor options handlers tests so I can grab some code from there.
|
@jasonsanjose Fixes after initial review pushed. |
|
@TomMalbran one more suggestion on the unit tests before merging. |
|
@jasonsanjose Fix pushed. I used the same solution as in the CommandOptionHandlers tests to close the documents. |
|
Looks good. Merging. |
Fix #3130: Move line up/down has incorrect behavior in edge cases in inline editor
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.