Skip to content
This repository was archived by the owner on Sep 6, 2021. It is now read-only.

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

Merged
merged 6 commits into from
Apr 17, 2013
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f96be45
Adding special cases when moving lines up or down at the last lines o…
TomMalbran Mar 24, 2013
bb3919b
Merge remote-tracking branch 'upstream/master' into tom/fix-issue-3130
TomMalbran Mar 26, 2013
e2ba221
Tests for the new special cases in inline editors
TomMalbran Mar 26, 2013
ac079bc
Tests fixes after first review
TomMalbran Apr 9, 2013
d91ee51
Merge remote-tracking branch 'upstream/master' into tom/fix-issue-3130
TomMalbran Apr 17, 2013
e59dccc
Close all documents after each test and open them before each test.
TomMalbran Apr 17, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 31 additions & 12 deletions src/editor/EditorCommandHandlers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -625,11 +625,13 @@ define(function (require, exports, module) {

var doc = editor.document,
sel = editor.getSelection(),
originalSel = editor.getSelection(),
hasSelection = (sel.start.line !== sel.end.line) || (sel.start.ch !== sel.end.ch),
inlineWidget = EditorManager.getFocusedInlineWidget(),
firstLine = editor.getFirstVisibleLine(),
lastLine = editor.getLastVisibleLine();
originalSel = editor.getSelection(),
hasSelection = (sel.start.line !== sel.end.line) || (sel.start.ch !== sel.end.ch),
isInlineWidget = !!EditorManager.getFocusedInlineWidget(),
firstLine = editor.getFirstVisibleLine(),
lastLine = editor.getLastVisibleLine(),
totalLines = editor.lineCount(),
lineLength = 0;

sel.start.ch = 0;
// The end of the selection becomes the start of the next line, if it isn't already
Expand All @@ -645,7 +647,13 @@ define(function (require, exports, module) {
var prevText = doc.getRange({ line: sel.start.line - 1, ch: 0 }, sel.start);

if (sel.end.line === lastLine + 1) {
prevText = "\n" + prevText.substring(0, prevText.length - 1);
if (isInlineWidget) {
prevText = prevText.substring(0, prevText.length - 1);
lineLength = doc.getLine(sel.end.line - 1).length;
doc.replaceRange("\n", { line: sel.end.line - 1, ch: lineLength });
} else {
prevText = "\n" + prevText.substring(0, prevText.length - 1);
}
}

doc.replaceRange("", { line: sel.start.line - 1, ch: 0 }, sel.start);
Expand All @@ -663,17 +671,28 @@ define(function (require, exports, module) {
}
break;
case DIRECTION_DOWN:
if (sel.end.line <= lastLine + (inlineWidget ? -1 : 1)) {
if (sel.end.line <= lastLine) {
doc.batchOperation(function () {
var nextText = doc.getRange(sel.end, { line: sel.end.line + 1, ch: 0 });
var nextText = doc.getRange(sel.end, { line: sel.end.line + 1, ch: 0 }),
deletionStart = sel.end;

var deletionStart = sel.end;
if (!inlineWidget && sel.end.line === lastLine) {
nextText += "\n";
deletionStart = { line: sel.end.line - 1, ch: doc.getLine(sel.end.line - 1).length };
if (sel.end.line === lastLine) {
if (isInlineWidget) {
if (sel.end.line === totalLines - 1) {
nextText += "\n";
}
lineLength = doc.getLine(sel.end.line - 1).length;
doc.replaceRange("\n", { line: sel.end.line, ch: doc.getLine(sel.end.line).length });
} else {
nextText += "\n";
deletionStart = { line: sel.end.line - 1, ch: doc.getLine(sel.end.line - 1).length };
}
}

doc.replaceRange("", deletionStart, { line: sel.end.line + 1, ch: 0 });
if (lineLength) {
doc.replaceRange("", { line: sel.end.line - 1, ch: lineLength }, { line: sel.end.line, ch: 0 });
}
doc.replaceRange(nextText, { line: sel.start.line, ch: 0 });
});
}
Expand Down
3 changes: 3 additions & 0 deletions test/spec/EditorCommandHandlers-test-files/test.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
.testClass {
color: red;
}
11 changes: 11 additions & 0 deletions test/spec/EditorCommandHandlers-test-files/test.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
<!doctype html>
<html>
<head>
<title>Simple Test</title>
<link rel="stylesheet" href="test.css">
</head>

<body>
<p class="testClass">Brackets is awesome!</p>
</body>
</html>
130 changes: 124 additions & 6 deletions test/spec/EditorCommandHandlers-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,13 @@
*/

/*jslint vars: true, plusplus: true, devel: true, nomen: true, indent: 4, maxerr: 50 */
/*global define, describe, it, expect, beforeEach, afterEach, waitsFor, waits, runs, $ */
/*global define, describe, it, expect, beforeEach, afterEach, waitsFor, waits, runs, $, waitsForDone */

define(function (require, exports, module) {
'use strict';

var Editor = require("editor/Editor").Editor,
EditorManager = require("editor/EditorManager"),
EditorCommandHandlers = require("editor/EditorCommandHandlers"),
Commands = require("command/Commands"),
CommandManager = require("command/CommandManager"),
Expand Down Expand Up @@ -59,19 +60,23 @@ define(function (require, exports, module) {
myEditor.focus();
}

function makeEditorWithRange(range) {
function makeEditorWithRange(range, content) {
content = content || defaultContent;

// create editor with a visible range
var mocks = SpecRunnerUtils.createMockEditor(defaultContent, "javascript", range);
var mocks = SpecRunnerUtils.createMockEditor(content, "javascript", range);
myDocument = mocks.doc;
myEditor = mocks.editor;

myEditor.focus();
}

afterEach(function () {
SpecRunnerUtils.destroyMockEditor(myDocument);
myEditor = null;
myDocument = null;
if (myDocument) {
SpecRunnerUtils.destroyMockEditor(myDocument);
myEditor = null;
myDocument = null;
}
});


Expand Down Expand Up @@ -2065,6 +2070,119 @@ define(function (require, exports, module) {
});


describe("Move Lines Up/Down - inline editor", function () {
this.category = "integration";

var testWindow, promise, editor;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

var testPath = SpecRunnerUtils.getTestPath("/spec/EditorCommandHandlers-test-files");

var moveContent = ".testClass {\n" +
" color: red;\n" +
"}";

beforeEach(function () {
if (!testWindow) {
SpecRunnerUtils.createTestWindowAndRun(this, function (w) {
testWindow = w;

// Load module instances from brackets.test
CommandManager = testWindow.brackets.test.CommandManager;
Commands = testWindow.brackets.test.Commands;
EditorManager = testWindow.brackets.test.EditorManager;

SpecRunnerUtils.loadProjectInTestWindow(testPath);
});
}

runs(function () {
promise = CommandManager.execute(Commands.FILE_ADD_TO_WORKING_SET, {fullPath: testPath + "/test.html"});
waitsForDone(promise, "Open into working set");
});

runs(function () {
// Open inline editor onto test.css's ".testClass" rule
promise = SpecRunnerUtils.toggleQuickEditAtOffset(EditorManager.getCurrentFullEditor(), {line: 8, ch: 11});
waitsForDone(promise, "Open inline editor");
});

runs(function () {
editor = EditorManager.getCurrentFullEditor().getInlineWidgets()[0].editors[0];
});
});

afterEach(function () {
runs(function () {
var promise = CommandManager.execute(Commands.FILE_CLOSE_ALL);
waitsForDone(promise, "Close all open files in working set");

// Close the save dialog without saving the changes
var $dlg = testWindow.$(".modal.instance");
if ($dlg.length) {
SpecRunnerUtils.clickDialogButton("dontsave");
}
});
});


it("should not move the first line of the inline editor up", function () {
editor.setCursorPos({line: 0, ch: 5});
CommandManager.execute(Commands.EDIT_LINE_UP, editor);

expect(editor.document.getText()).toEqual(moveContent);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also check _codeMirror.doc.isClean() or _codeMirror.doc.historySize() to make sure no no-op edits exist.

expect(editor._codeMirror.doc.historySize().undo).toBe(0);
expect(editor.getFirstVisibleLine()).toBe(0);
expect(editor.getLastVisibleLine()).toBe(2);
});

it("should not move the last line of the inline editor down", function () {
editor.setCursorPos({line: 2, ch: 5});
CommandManager.execute(Commands.EDIT_LINE_DOWN, editor);

expect(editor.document.getText()).toEqual(moveContent);
expect(editor._codeMirror.doc.historySize().undo).toBe(0);
expect(editor.getFirstVisibleLine()).toBe(0);
expect(editor.getLastVisibleLine()).toBe(2);
});

it("should be able to move the second to last line of the inline editor down", function () {
editor.setCursorPos({line: 1, ch: 5});
CommandManager.execute(Commands.EDIT_LINE_DOWN, editor);

var lines = moveContent.split("\n");
var temp = lines[1];
lines[1] = lines[2];
lines[2] = temp;
var expectedText = lines.join("\n");

expect(editor.document.getText()).toEqual(expectedText);
expect(editor.getFirstVisibleLine()).toBe(0);
expect(editor.getLastVisibleLine()).toBe(2);
});

it("should be able to move the last line of the inline editor up", function () {
editor.setCursorPos({line: 2, ch: 0});
CommandManager.execute(Commands.EDIT_LINE_UP, editor);

var lines = moveContent.split("\n");
var temp = lines[1];
lines[1] = lines[2];
lines[2] = temp;
var expectedText = lines.join("\n");

expect(editor.document.getText()).toEqual(expectedText);
expect(editor.getFirstVisibleLine()).toBe(0);
expect(editor.getLastVisibleLine()).toBe(2);

// This must be in the last spec in the suite.
runs(function () {
this.after(function () {
SpecRunnerUtils.closeTestWindow();
});
});
});
});


describe("Delete Line", function () {
beforeEach(setupFullEditor);

Expand Down