Merge pull request #1932 from adobe/nj/issue-1764

Fix Delete Line to avoid exceptions and work intuitively in inline editors

Author: peterflynn

src/editor/Editor.js

@@ -537,15 +537,6 @@ define(function (require, exports, module) {
         // note: this change might have been a real edit made by the user, OR this might have
         // been a change synced from another editor
 
-        if (this._visibleRange) {
-            // _visibleRange has already updated via its own Document listener, when we pushed our
-            // change into the Document above (_masterEditor._applyChanges()). But changes due to our
-            // own edits should never cause the range to lose sync - verify that.
-            if (this._visibleRange.startLine === null || this._visibleRange.endLine === null) {
-                throw new Error("ERROR: Typing in Editor should not destroy its own _visibleRange");
-            }
-        }
-        
         CodeHintManager.handleChange(this);
     };
 

src/editor/EditorCommandHandlers.js

@@ -170,17 +170,25 @@ define(function (require, exports, module) {
             return;
         }
 
-        var from, to, sel, doc;
+        var from,
+            to,
+            sel = editor.getSelection(),
+            doc = editor.document;
 
-        doc = editor._codeMirror;
-        sel = doc.getCursor(true);
-        from = {line: sel.line, ch: 0};
-        sel = doc.getCursor(false);
-        to = {line: sel.line + 1, ch: 0};
-        if (doc.lineCount() === to.line && from.line > 0) {
-            from.line -= 1;
-            from.ch = doc.getLine(from.line).length;
+        from = {line: sel.start.line, ch: 0};
+        to = {line: sel.end.line + 1, ch: 0};
+        if (to.line === editor.getLastVisibleLine() + 1) {
+            // Instead of deleting the newline after the last line, delete the newline
+            // before the first line--unless this is the entire visible content of the editor,
+            // in which case just delete the line content.
+            if (from.line > editor.getFirstVisibleLine()) {
+                from.line -= 1;
+                from.ch = doc.getLine(from.line).length;
+            }
+            to.line -= 1;
+            to.ch = doc.getLine(to.line).length;
         }
+        
         doc.replaceRange("", from, to);
     }
 

test/spec/EditorCommandHandlers-test.js

@@ -47,14 +47,15 @@ define(function (require, exports, module) {
                              "}";
 
         var myDocument, myEditor;
-        beforeEach(function () {
+        
+        function setupFullEditor() {
             // create dummy Document and Editor
             var mocks = SpecRunnerUtils.createMockEditor(defaultContent, "javascript");
             myDocument = mocks.doc;
             myEditor = mocks.editor;
 
             myEditor.focus();
-        });
+        }
 
         afterEach(function () {
             SpecRunnerUtils.destroyMockEditor(myDocument);
@@ -75,7 +76,8 @@ define(function (require, exports, module) {
 
 
         describe("Line comment/uncomment", function () {
-
+            beforeEach(setupFullEditor);
+            
             it("should comment/uncomment a single line, cursor at start", function () {
                 myEditor.setCursorPos(3, 0);
 
@@ -352,6 +354,8 @@ define(function (require, exports, module) {
 
 
         describe("Duplicate", function () {
+            beforeEach(setupFullEditor);
+
             it("should duplicate whole line if no selection", function () {
                 // place cursor in middle of line 1
                 myEditor.setCursorPos(1, 10);
@@ -498,6 +502,7 @@ define(function (require, exports, module) {
         });
 
         describe("Move Lines Up/Down", function () {
+            beforeEach(setupFullEditor);
 
             it("should move whole line up if no selection", function () {
                 // place cursor in middle of line 1
@@ -805,6 +810,140 @@ define(function (require, exports, module) {
             });
         });
 
+        describe("Delete Line", function () {
+            beforeEach(setupFullEditor);
+            
+            it("should delete the first line when selection is an IP in that line", function () {
+                myEditor.setSelection({line: 0, ch: 5}, {line: 0, ch: 5});
+                CommandManager.execute(Commands.EDIT_DELETE_LINES, myEditor);
+                
+                var expectedText = defaultContent.split("\n").slice(1).join("\n");
+                expect(myDocument.getText()).toEqual(expectedText);
+            });
+            
+            it("should delete the first line when selection is a range in that line", function () {
+                myEditor.setSelection({line: 0, ch: 5}, {line: 0, ch: 8});
+                CommandManager.execute(Commands.EDIT_DELETE_LINES, myEditor);
+                
+                var expectedText = defaultContent.split("\n").slice(1).join("\n");
+                expect(myDocument.getText()).toEqual(expectedText);
+            });
+            
+            it("should delete a middle line when selection is an IP in that line", function () {
+                myEditor.setSelection({line: 2, ch: 5}, {line: 2, ch: 5});
+                CommandManager.execute(Commands.EDIT_DELETE_LINES, myEditor);
+                
+                var lines = defaultContent.split("\n");
+                lines.splice(2, 1);
+                expect(myDocument.getText()).toEqual(lines.join("\n"));
+            });
+            
+            it("should delete a middle line when selection is a range in that line", function () {
+                myEditor.setSelection({line: 2, ch: 5}, {line: 2, ch: 8});
+                CommandManager.execute(Commands.EDIT_DELETE_LINES, myEditor);
+                
+                var lines = defaultContent.split("\n");
+                lines.splice(2, 1);
+                expect(myDocument.getText()).toEqual(lines.join("\n"));
+            });
+            
+            it("should delete the last line when selection is an IP in that line", function () {
+                myEditor.setSelection({line: 7, ch: 0}, {line: 7, ch: 0});
+                CommandManager.execute(Commands.EDIT_DELETE_LINES, myEditor);
+                
+                var expectedText = defaultContent.split("\n").slice(0, 7).join("\n");
+                expect(myDocument.getText()).toEqual(expectedText);
+            });
+            
+            it("should delete the last line when selection is a range in that line", function () {
+                myEditor.setSelection({line: 7, ch: 0}, {line: 7, ch: 1});
+                CommandManager.execute(Commands.EDIT_DELETE_LINES, myEditor);
+                
+                var expectedText = defaultContent.split("\n").slice(0, 7).join("\n");
+                expect(myDocument.getText()).toEqual(expectedText);
+            });
+            
+            it("should delete multiple lines starting at the top when selection spans them", function () {
+                myEditor.setSelection({line: 0, ch: 5}, {line: 2, ch: 4});
+                CommandManager.execute(Commands.EDIT_DELETE_LINES, myEditor);
+                
+                var expectedText = defaultContent.split("\n").slice(3).join("\n");
+                expect(myDocument.getText()).toEqual(expectedText);
+            });
+
+            it("should delete multiple lines ending at the bottom when selection spans them", function () {
+                myEditor.setSelection({line: 5, ch: 5}, {line: 7, ch: 0});
+                CommandManager.execute(Commands.EDIT_DELETE_LINES, myEditor);
+                
+                var expectedText = defaultContent.split("\n").slice(0, 5).join("\n");
+                expect(myDocument.getText()).toEqual(expectedText);
+            });
+            
+            it("should leave empty text when all lines are selected", function () {
+                myEditor.setSelection({line: 0, ch: 4}, {line: 7, ch: 1});
+                CommandManager.execute(Commands.EDIT_DELETE_LINES, myEditor);
+                expect(myDocument.getText()).toEqual("");
+            });
+        });
+        
+        describe("Delete Line - editor with visible range", function () {
+            function makeEditorWithRange(range) {
+                // create editor with a visible range
+                var mocks = SpecRunnerUtils.createMockEditor(defaultContent, "javascript", range);
+                myDocument = mocks.doc;
+                myEditor = mocks.editor;
+                
+                myEditor.focus();
+            }
+
+            it("should delete the top line of the visible range", function () {
+                makeEditorWithRange({startLine: 1, endLine: 5});
+                myEditor.setSelection({line: 1, ch: 5}, {line: 1, ch: 5});
+                CommandManager.execute(Commands.EDIT_DELETE_LINES, myEditor);
+                
+                var lines = defaultContent.split("\n");
+                lines.splice(1, 1);
+                expect(myDocument.getText()).toEqual(lines.join("\n"));
+                expect(myEditor._visibleRange.startLine).toNotBe(null);
+                expect(myEditor._visibleRange.endLine).toNotBe(null);
+            });
+
+            it("should delete the bottom line of the visible range", function () {
+                makeEditorWithRange({startLine: 1, endLine: 5});
+                myEditor.setSelection({line: 5, ch: 2}, {line: 5, ch: 2});
+                CommandManager.execute(Commands.EDIT_DELETE_LINES, myEditor);
+                
+                var lines = defaultContent.split("\n");
+                lines.splice(5, 1);
+                expect(myDocument.getText()).toEqual(lines.join("\n"));
+                expect(myEditor._visibleRange.startLine).toNotBe(null);
+                expect(myEditor._visibleRange.endLine).toNotBe(null);
+            });
+            
+            it("should leave a single newline when all visible lines are selected", function () {
+                makeEditorWithRange({startLine: 1, endLine: 5});
+                myEditor.setSelection({line: 1, ch: 5}, {line: 5, ch: 2});
+                CommandManager.execute(Commands.EDIT_DELETE_LINES, myEditor);
+                
+                var lines = defaultContent.split("\n");
+                lines.splice(1, 5, "");
+                expect(myDocument.getText()).toEqual(lines.join("\n"));
+                expect(myEditor._visibleRange.startLine).toNotBe(null);
+                expect(myEditor._visibleRange.endLine).toNotBe(null);
+            });
+            
+            it("should leave a single newline when only one line is visible", function () {
+                makeEditorWithRange({startLine: 3, endLine: 3});
+                myEditor.setSelection({line: 3, ch: 4}, {line: 3, ch: 4});
+                CommandManager.execute(Commands.EDIT_DELETE_LINES, myEditor);
+                
+                var lines = defaultContent.split("\n");
+                lines.splice(3, 1, "");
+                expect(myDocument.getText()).toEqual(lines.join("\n"));
+                expect(myEditor._visibleRange.startLine).toNotBe(null);
+                expect(myEditor._visibleRange.endLine).toNotBe(null);
+            });
+        });
 
     });
 });

test/spec/SpecRunnerUtils.js

@@ -129,7 +129,7 @@ define(function (require, exports, module) {
      * currentDocument or added to the working set.
      * @return {!{doc:{Document}, editor:{Editor}}}
      */
-    function createMockEditor(initialContent, mode) {
+    function createMockEditor(initialContent, mode, visibleRange) {
         mode = mode || "";
 
         // Initialize EditorManager
@@ -142,7 +142,7 @@ define(function (require, exports, module) {
         var doc = createMockDocument(initialContent);
 
         // create Editor instance
-        var editor = new Editor(doc, true, mode, $editorHolder.get(0), {});
+        var editor = new Editor(doc, true, mode, $editorHolder.get(0), {}, visibleRange);
 
         return { doc: doc, editor: editor };
     }