Merge pull request #444 from blink1073/delete-select-next

Deleting cells should select the next cell

src/notebook/notebook/actions.ts

@@ -165,7 +165,7 @@ namespace NotebookActions {
    * @param widget - The target notebook widget.
    *
    * #### Notes
-   * The cell before the first selected cell will be activated.
+   * The cell after the last selected cell will be activated.
    * It will add a code cell if all cells are deleted.
    * This action can be undone.
    */
@@ -184,9 +184,7 @@ namespace NotebookActions {
     for (let i = 0; i < widget.childCount(); i++) {
       let child = widget.childAt(i);
       if (widget.isSelected(child)) {
-        if (index === -1) {
-          index = i - 1;
-        }
+        index = i;
         toDelete.push(cells.get(i));
       }
     }
@@ -196,18 +194,14 @@ namespace NotebookActions {
     for (let cell of toDelete) {
       cells.remove(cell);
     }
-
-    // Add a new code cell if all cells were deleted.
-    if (!model.cells.length) {
-      let cell = model.factory.createCodeCell();
-      model.cells.add(cell);
-    }
+    // The model will add a new code cell if there are no
+    // remaining cells.
     model.cells.endCompoundOperation();
 
-    // Activate the previous cell.
-    if (index === -1) {
-      index = 0;
-    }
+    // Select the cell *after* the last selected.
+    // Note: The activeCellIndex is clamped to the available cells,
+    // so if the last cell is deleted the previous cell will be activated.
+    index -= toDelete.length - 1;
     widget.activeCellIndex = index;
   }
 

src/notebook/notebook/model.ts

@@ -438,6 +438,10 @@ class NotebookModel extends DocumentModel implements INotebookModel {
     default:
       return;
     }
+    // Add code cell if there are no cells remaining.
+    if (!this._cells.length) {
+      this._cells.add(this._factory.createCodeCell());
+    }
     this.contentChanged.emit(void 0);
     this.dirty = true;
   }

test/src/common/activitymonitor.spec.ts

@@ -4,7 +4,7 @@
 import expect = require('expect.js');
 
 import {
-  Signal
+  ISignal, Signal
 } from 'phosphor-signaling';
 
 import {
@@ -12,21 +12,47 @@ import {
 } from '../../../lib/common/activitymonitor';
 
 
+
+class TestObject {
+
+  static oneSignal = new Signal<TestObject, number>();
+
+  static twoSignal = new Signal<TestObject, string[]>();
+
+  get one(): ISignal<TestObject, number> {
+    return TestObject.oneSignal.bind(this);
+  }
+
+  get two(): ISignal<TestObject, string[]> {
+    return TestObject.twoSignal.bind(this);
+  }
+}
+
+
+
 describe('common/activitymonitor', () => {
 
   describe('ActivityMonitor()', () => {
 
+    let testObj: TestObject;
+    let signal: ISignal<TestObject, number>;
+
+    beforeEach(() => {
+      testObj = new TestObject();
+      signal = testObj.one;
+    });
+
     describe('#constructor()', () => {
 
       it('should accept a signal', () => {
-        let signal = new Signal<Window, number>().bind(window);
-        let monitor = new ActivityMonitor<Window, number>({ signal });
+        let monitor = new ActivityMonitor<TestObject, number>({ signal });
         expect(monitor).to.be.an(ActivityMonitor);
       });
 
       it('should accept a timeout', () => {
-        let signal = new Signal<Window, number>().bind(window);
-        let monitor = new ActivityMonitor<Window, number>({ signal, timeout: 100 });
+        let monitor = new ActivityMonitor<TestObject, string[]>({
+          signal: testObj.two, timeout: 100
+        });
         expect(monitor).to.be.an(ActivityMonitor);
       });
 
@@ -35,12 +61,11 @@ describe('common/activitymonitor', () => {
     describe('#activityStopped', () => {
 
       it('should be emitted after the signal has fired and a timeout', (done) => {
-        let signal = new Signal<Window, number>().bind(window);
         let called = false;
         let monitor = new ActivityMonitor({ signal, timeout: 100 });
         monitor.activityStopped.connect((sender, args) => {
           expect(sender).to.be(monitor);
-          expect(args.sender).to.be(window);
+          expect(args.sender).to.be(testObj);
           expect(args.args).to.be(10);
           called = true;
         });
@@ -53,12 +78,11 @@ describe('common/activitymonitor', () => {
       });
 
       it('should collapse during activity', (done) => {
-        let signal = new Signal<Window, number>().bind(window);
         let called = false;
-        let monitor = new ActivityMonitor<Window, number>({ signal, timeout: 100 });
+        let monitor = new ActivityMonitor<TestObject, number>({ signal, timeout: 100 });
         monitor.activityStopped.connect((sender, args) => {
           expect(sender).to.be(monitor);
-          expect(args.sender).to.be(window);
+          expect(args.sender).to.be(testObj);
           expect(args.args).to.be(10);
           called = true;
         });
@@ -66,7 +90,7 @@ describe('common/activitymonitor', () => {
         expect(called).to.be(false);
         setTimeout(() => {
           signal.emit(10);
-        }, 90);
+        }, 70);
         setTimeout(() => {
           expect(called).to.be(false);
         }, 100);
@@ -81,14 +105,12 @@ describe('common/activitymonitor', () => {
     describe('#timeout', () => {
 
       it('should default to `1000`', () => {
-        let signal = new Signal<Window, number>().bind(window);
-        let monitor = new ActivityMonitor<Window, number>({ signal });
+        let monitor = new ActivityMonitor<TestObject, number>({ signal });
         expect(monitor.timeout).to.be(1000);
       });
 
       it('should be set-able', () => {
-        let signal = new Signal<Window, number>().bind(window);
-        let monitor = new ActivityMonitor<Window, number>({ signal });
+        let monitor = new ActivityMonitor<TestObject, number>({ signal });
         monitor.timeout = 200;
         expect(monitor.timeout).to.be(200);
       });
@@ -98,16 +120,14 @@ describe('common/activitymonitor', () => {
     describe('#isDisposed', () => {
 
       it('should test whether the monitor is disposed', () => {
-        let signal = new Signal<Window, number>().bind(window);
-        let monitor = new ActivityMonitor<Window, number>({ signal });
+        let monitor = new ActivityMonitor<TestObject, number>({ signal });
         expect(monitor.isDisposed).to.be(false);
         monitor.dispose();
         expect(monitor.isDisposed).to.be(true);
       });
 
       it('should be read-only', () => {
-        let signal = new Signal<Window, number>().bind(window);
-        let monitor = new ActivityMonitor<Window, number>({ signal });
+        let monitor = new ActivityMonitor<TestObject, number>({ signal });
         expect(() => { monitor.isDisposed = false; }).to.throwError();
       });
 
@@ -116,15 +136,13 @@ describe('common/activitymonitor', () => {
     describe('#dispose()', () => {
 
       it('should disposed of the resources used by the monitor', () => {
-        let signal = new Signal<Window, number>().bind(window);
-        let monitor = new ActivityMonitor<Window, number>({ signal });
+        let monitor = new ActivityMonitor<TestObject, number>({ signal });
         monitor.dispose();
         expect(monitor.isDisposed).to.be(true);
       });
 
       it('should be a no-op if called more than once', () => {
-        let signal = new Signal<Window, number>().bind(window);
-        let monitor = new ActivityMonitor<Window, number>({ signal });
+        let monitor = new ActivityMonitor<TestObject, number>({ signal });
         monitor.dispose();
         monitor.dispose();
         expect(monitor.isDisposed).to.be(true);

test/src/notebook/notebook/actions.spec.ts

@@ -240,12 +240,18 @@ describe('notebook/notebook/actions', () => {
         expect(widget.mode).to.be('command');
       });
 
-      it('should activate the cell before the first selected cell', () => {
+      it('should activate the cell after the last selected cell', () => {
         widget.activeCellIndex = 4;
         let prev = widget.childAt(2);
         widget.select(prev);
         NotebookActions.deleteCells(widget);
-        expect(widget.activeCellIndex).to.be(1);
+        expect(widget.activeCellIndex).to.be(3);
+      });
+
+      it('should select the previous cell if the last cell is deleted', () => {
+        widget.select(widget.childAt(widget.childCount() - 1));
+        NotebookActions.deleteCells(widget);
+        expect(widget.activeCellIndex).to.be(widget.childCount() - 1);
       });
 
       it('should add a code cell if all cells are deleted', () => {

test/src/notebook/notebook/model.spec.ts

@@ -150,6 +150,13 @@ describe('notebook/notebook', () => {
           expect(cell.isDisposed).to.be(true);
         });
 
+        it('should add a new code cell when cells are cleared', () => {
+          let model = new NotebookModel();
+          model.cells.clear();
+          expect(model.cells.length).to.be(1);
+          expect(model.cells.get(0)).to.be.a(CodeCellModel);
+        });
+
       });
 
       describe('cell `changed` signal', () => {

test/src/notebook/notebook/widget.spec.ts

@@ -268,7 +268,8 @@ describe('notebook/notebook/widget', () => {
         it('should handle changes to the model cell list', () => {
           widget = createWidget();
           widget.model.cells.clear();
-          expect(widget.childCount()).to.be(0);
+          // The model should add a single code cell.
+          expect(widget.childCount()).to.be(1);
         });
 
         it('should handle a remove', () => {