Use a symmetric serializer.

packages/coreutils/src/undoablevector.ts

@@ -2,7 +2,7 @@
 // Distributed under the terms of the Modified BSD License.
 
 import {
-  JSONObject
+  JSONValue
 } from '@phosphor/coreutils';
 
 import {
@@ -15,14 +15,20 @@ import {
 
 
 /**
- * An object which can be serialized to JSON.
+ * An object which knows how to serialize and
+ * deserialize the type T.
  */
 export
-interface ISerializable {
+interface ISerializer<T> {
   /**
    * Convert the object to JSON.
    */
-  toJSON(): JSONObject;
+  toJSON(value: T): JSONValue;
+
+  /**
+   * Deserialize the object from JSON.
+   */
+  fromJSON(value: JSONValue): T;
 }
 
 
@@ -30,7 +36,7 @@ interface ISerializable {
  * An observable vector that supports undo/redo.
  */
 export
-interface IObservableUndoableVector<T extends ISerializable> extends IObservableVector<T> {
+interface IObservableUndoableVector<T> extends IObservableVector<T> {
   /**
    * Whether the object can redo changes.
    */
@@ -75,13 +81,13 @@ interface IObservableUndoableVector<T extends ISerializable> extends IObservable
  * A concrete implementation of an observable undoable vector.
  */
 export
-class ObservableUndoableVector<T extends ISerializable> extends ObservableVector<T> implements IObservableUndoableVector<T> {
+class ObservableUndoableVector<T> extends ObservableVector<T> implements IObservableUndoableVector<T> {
   /**
    * Construct a new undoable observable vector.
    */
-  constructor(factory: (value: JSONObject) => T) {
+  constructor(serializer: ISerializer<T>) {
     super();
-    this._factory = factory;
+    this._serializer = serializer;
     this.changed.connect(this._onVectorChanged, this);
   }
 
@@ -103,7 +109,7 @@ class ObservableUndoableVector<T extends ISerializable> extends ObservableVector
    * Dispose of the resources held by the model.
    */
   dispose(): void {
-    this._factory = null;
+    this._serializer = null;
     this._stack = null;
     super.dispose();
   }
@@ -201,9 +207,9 @@ class ObservableUndoableVector<T extends ISerializable> extends ObservableVector
   /**
    * Undo a change event.
    */
-  private _undoChange(change: ObservableVector.IChangedArgs<JSONObject>): void {
+  private _undoChange(change: ObservableVector.IChangedArgs<JSONValue>): void {
     let index = 0;
-    let factory = this._factory;
+    let serializer = this._serializer;
     switch (change.type) {
     case 'add':
       each(change.newValues, () => {
@@ -213,13 +219,13 @@ class ObservableUndoableVector<T extends ISerializable> extends ObservableVector
     case 'set':
       index = change.oldIndex;
       each(change.oldValues, value => {
-        this.set(index++, factory(value));
+        this.set(index++, serializer.fromJSON(value));
       });
       break;
     case 'remove':
       index = change.oldIndex;
       each(change.oldValues, value => {
-        this.insert(index++, factory(value));
+        this.insert(index++, serializer.fromJSON(value));
       });
       break;
     case 'move':
@@ -233,20 +239,20 @@ class ObservableUndoableVector<T extends ISerializable> extends ObservableVector
   /**
    * Redo a change event.
    */
-  private _redoChange(change: ObservableVector.IChangedArgs<JSONObject>): void {
+  private _redoChange(change: ObservableVector.IChangedArgs<JSONValue>): void {
     let index = 0;
-    let factory = this._factory;
+    let serializer = this._serializer;
     switch (change.type) {
     case 'add':
       index = change.newIndex;
       each(change.newValues, value => {
-        this.insert(index++, factory(value));
+        this.insert(index++, serializer.fromJSON(value));
       });
       break;
     case 'set':
       index = change.newIndex;
       each(change.newValues, value => {
-        this.set(change.newIndex++, factory(value));
+        this.set(change.newIndex++, serializer.fromJSON(value));
       });
       break;
     case 'remove':
@@ -265,14 +271,14 @@ class ObservableUndoableVector<T extends ISerializable> extends ObservableVector
   /**
    * Copy a change as JSON.
    */
-  private _copyChange(change: ObservableVector.IChangedArgs<T>): ObservableVector.IChangedArgs<JSONObject> {
-    let oldValues: JSONObject[] = [];
+  private _copyChange(change: ObservableVector.IChangedArgs<T>): ObservableVector.IChangedArgs<JSONValue> {
+    let oldValues: JSONValue[] = [];
     each(change.oldValues, value => {
-      oldValues.push(value.toJSON());
+      oldValues.push(this._serializer.toJSON(value));
     });
-    let newValues: JSONObject[] = [];
+    let newValues: JSONValue[] = [];
     each(change.newValues, value => {
-      newValues.push(value.toJSON());
+      newValues.push(this._serializer.toJSON(value));
     });
     return {
       type: change.type,
@@ -287,6 +293,6 @@ class ObservableUndoableVector<T extends ISerializable> extends ObservableVector
   private _isUndoable = true;
   private _madeCompoundChange = false;
   private _index = -1;
-  private _stack: ObservableVector.IChangedArgs<JSONObject>[][] = [];
-  private _factory: (value: JSONObject) => T = null;
+  private _stack: ObservableVector.IChangedArgs<JSONValue>[][] = [];
+  private _serializer: ISerializer<T> = null;
 }

packages/notebook/src/celllist.ts

@@ -19,7 +19,7 @@ import {
 
 import {
   IObservableMap, ObservableMap, IObservableVector, ObservableVector,
-  IObservableUndoableVector, ObservableUndoableVector, ISerializable
+  IObservableUndoableVector, ObservableUndoableVector
 } from '@jupyterlab/coreutils';
 
 import {
@@ -27,23 +27,6 @@ import {
 } from '@jupyterlab/cells';
 
 
-/**
- * Unfortunately, the current implementation of the ObservableUndoableVector
- * requires the values to implement to/from JSON. This should not be 
- * necessary for primitives (in this case strings).
- */
-class CellID implements ISerializable { 
-  constructor(id: string) {
-    this.id = id;
-  }
-
-  readonly id: string;
-
-  toJSON(): JSONObject {
-    return { id: this.id };
-  }
-}
-
 /**
  * A cell list object that supports undo/redo.
  */
@@ -53,8 +36,9 @@ class CellList implements IObservableUndoableVector<ICellModel> {
    * Construct the cell list.
    */
   constructor() {
-    this._cellOrder = new ObservableUndoableVector<CellID>((id: JSONObject) => {
-      return new CellID((id as any).id);
+    this._cellOrder = new ObservableUndoableVector<string>({
+      toJSON: (val: string)=>{return val;},
+      fromJSON: (val: string)=>{return val;}
     });
     this._cellMap = new ObservableMap<ICellModel>();
 
@@ -163,7 +147,7 @@ class CellList implements IObservableUndoableVector<ICellModel> {
   iter(): IIterator<ICellModel> {
     let arr: ICellModel[] = [];
     for (let id of toArray(this._cellOrder)) {
-      arr.push(this._cellMap.get(id.id));
+      arr.push(this._cellMap.get(id));
     }
     return new ArrayIterator<ICellModel>(arr);
   }
@@ -202,7 +186,7 @@ class CellList implements IObservableUndoableVector<ICellModel> {
    * An `index` which is non-integral or out of range.
    */
   at(index: number): ICellModel {
-    return this._cellMap.get(this._cellOrder.at(index).id) as ICellModel;
+    return this._cellMap.get(this._cellOrder.at(index)) as ICellModel;
   }
 
   /**
@@ -231,7 +215,7 @@ class CellList implements IObservableUndoableVector<ICellModel> {
     let id = utils.uuid();
     // Set the internal data structures.
     this._cellMap.set(id, cell);
-    this._cellOrder.set(index, new CellID(id));
+    this._cellOrder.set(index, id);
   }
 
   /**
@@ -257,7 +241,7 @@ class CellList implements IObservableUndoableVector<ICellModel> {
     let id = utils.uuid();
     // Set the internal data structures.
     this._cellMap.set(id, cell);
-    let num = this._cellOrder.pushBack(new CellID(id));
+    let num = this._cellOrder.pushBack(id);
     return num;
   }
 
@@ -276,7 +260,7 @@ class CellList implements IObservableUndoableVector<ICellModel> {
   popBack(): ICellModel {
     //Don't clear the map in case we need to reinstate the cell
     let id = this._cellOrder.popBack();
-    let cell = this._cellMap.get(id.id);
+    let cell = this._cellMap.get(id);
     return cell;
   }
 
@@ -311,7 +295,7 @@ class CellList implements IObservableUndoableVector<ICellModel> {
     let id = utils.uuid();
     // Set the internal data structures.
     this._cellMap.set(id, cell);
-    let num = this._cellOrder.insert(index, new CellID(id));
+    let num = this._cellOrder.insert(index, id);
     return num;
   }
 
@@ -331,7 +315,7 @@ class CellList implements IObservableUndoableVector<ICellModel> {
    */
   remove(cell: ICellModel): number {
     let index = ArrayExt.findFirstIndex(
-      toArray(this._cellOrder), id => (this._cellMap.get(id.id)===cell));
+      toArray(this._cellOrder), id => (this._cellMap.get(id)===cell));
     this.removeAt(index);
     return index;
   }
@@ -355,7 +339,7 @@ class CellList implements IObservableUndoableVector<ICellModel> {
    */
   removeAt(index: number): ICellModel {
     let id= this._cellOrder.removeAt(index);
-    let cell = this._cellMap.get(id.id);
+    let cell = this._cellMap.get(id);
     return cell;
   }
 
@@ -420,7 +404,7 @@ class CellList implements IObservableUndoableVector<ICellModel> {
       let id = utils.uuid();
       // Set the internal data structures.
       this._cellMap.set(id, cell);
-      let num = this._cellOrder.pushBack(new CellID(id));
+      let num = this._cellOrder.pushBack(id);
     });
     return this.length;
   }
@@ -458,7 +442,7 @@ class CellList implements IObservableUndoableVector<ICellModel> {
       let id = utils.uuid();
       this._cellMap.set(id, cell);
       this._cellOrder.beginCompoundOperation();
-      this._cellOrder.insert(index++, new CellID(id));
+      this._cellOrder.insert(index++, id);
       this._cellOrder.endCompoundOperation();
     });
     return this.length;
@@ -540,7 +524,7 @@ class CellList implements IObservableUndoableVector<ICellModel> {
     // cell order.
     for (let key of this._cellMap.keys()) {
       if (ArrayExt.findFirstIndex(
-         toArray(this._cellOrder), id => (id.id)===key) === -1) {
+         toArray(this._cellOrder), id => (id)===key) === -1) {
         let cell = this._cellMap.get(key) as ICellModel;
         cell.dispose();
         this._cellMap.delete(key);
@@ -549,14 +533,14 @@ class CellList implements IObservableUndoableVector<ICellModel> {
     this._cellOrder.clearUndo();
   }
 
-  private _onOrderChanged(order: IObservableVector<CellID>, change: ObservableVector.IChangedArgs<CellID>): void {
+  private _onOrderChanged(order: IObservableVector<string>, change: ObservableVector.IChangedArgs<string>): void {
     let newValues: ICellModel[] = [];
     let oldValues: ICellModel[] = [];
     each(change.newValues, (id)=>{
-      newValues.push(this._cellMap.get(id.id));
+      newValues.push(this._cellMap.get(id));
     });
     each(change.oldValues, (id)=>{
-      oldValues.push(this._cellMap.get(id.id));
+      oldValues.push(this._cellMap.get(id));
     });
     this._changed.emit({
       type: change.type,
@@ -568,7 +552,7 @@ class CellList implements IObservableUndoableVector<ICellModel> {
   }
 
   private _isDisposed: boolean = false;
-  private _cellOrder: IObservableUndoableVector<CellID> = null;
+  private _cellOrder: IObservableUndoableVector<string> = null;
   private _cellMap: IObservableMap<ICellModel> = null;
   private _changed = new Signal<this, ObservableVector.IChangedArgs<ICellModel>>(this);
 }

test/src/coreutils/undoablevector.spec.ts

@@ -8,18 +8,17 @@ import {
 } from '@phosphor/coreutils';
 
 import {
-  ObservableUndoableVector, ISerializable
+  ObservableUndoableVector, ISerializer
 } from '@jupyterlab/coreutils';
 
 
 
-class Test implements ISerializable {
-
+class Test {
   constructor(value: JSONObject) {
     this._value = value;
   }
 
-  toJSON(): JSONObject {
+  get value(): JSONObject {
     return this._value;
   }
 
@@ -29,12 +28,18 @@ class Test implements ISerializable {
 
 let count = 0;
 
-function factory(value: JSONObject): Test {
-  value['count'] = count++;
-  return new Test(value);
-}
+class Serializer implements ISerializer<Test> {
+  fromJSON(value: JSONObject): Test {
+    value['count'] = count++;
+    return new Test(value);
+  }
 
+  toJSON(value: Test): JSONObject {
+    return value.value;
+  }
+}
 
+const serializer = new Serializer();
 const value: JSONObject = { name: 'foo' };
 
 
@@ -45,7 +50,7 @@ describe('notebook/common/undo', () => {
     describe('#constructor', () => {
 
       it('should create a new ObservableUndoableVector', () => {
-        let vector = new ObservableUndoableVector(factory);
+        let vector = new ObservableUndoableVector(serializer);
         expect(vector).to.be.an(ObservableUndoableVector);
       });
 
@@ -54,12 +59,12 @@ describe('notebook/common/undo', () => {
     describe('#canRedo', () => {
 
       it('should return false if there is no history', () => {
-        let vector = new ObservableUndoableVector(factory);
+        let vector = new ObservableUndoableVector(serializer);
         expect(vector.canRedo).to.be(false);
       });
 
       it('should return true if there is an undo that can be redone', () => {
-        let vector = new ObservableUndoableVector(factory);
+        let vector = new ObservableUndoableVector(serializer);
         vector.pushBack(new Test(value));
         vector.undo();
         expect(vector.canRedo).to.be(true);
@@ -70,13 +75,13 @@ describe('notebook/common/undo', () => {
     describe('#canUndo', () => {
 
       it('should return false if there is no history', () => {
-        let vector = new ObservableUndoableVector(factory);
+        let vector = new ObservableUndoableVector(serializer);
         expect(vector.canUndo).to.be(false);
       });
 
       it('should return true if there is a change that can be undone', () => {
-        let vector = new ObservableUndoableVector(factory);
-        vector.pushBack(factory(value));
+        let vector = new ObservableUndoableVector(serializer);
+        vector.pushBack(serializer.fromJSON(value));
         expect(vector.canUndo).to.be(true);
       });
 
@@ -85,7 +90,7 @@ describe('notebook/common/undo', () => {
     describe('#dispose()', () => {
 
       it('should dispose of the resources used by the vector', () => {
-        let vector = new ObservableUndoableVector(factory);
+        let vector = new ObservableUndoableVector(serializer);
         vector.dispose();
         expect(vector.isDisposed).to.be(true);
         vector.dispose();
@@ -97,10 +102,10 @@ describe('notebook/common/undo', () => {
     describe('#beginCompoundOperation()', () => {
 
       it('should begin a compound operation', () => {
-        let vector = new ObservableUndoableVector(factory);
+        let vector = new ObservableUndoableVector(serializer);
         vector.beginCompoundOperation();
-        vector.pushBack(factory(value));
-        vector.pushBack(factory(value));
+        vector.pushBack(serializer.fromJSON(value));
+        vector.pushBack(serializer.fromJSON(value));
         vector.endCompoundOperation();
         expect(vector.canUndo).to.be(true);
         vector.undo();
@@ -108,10 +113,10 @@ describe('notebook/common/undo', () => {
       });
 
       it('should not be undoable if isUndoAble is set to false', () => {
-        let vector = new ObservableUndoableVector(factory);
+        let vector = new ObservableUndoableVector(serializer);
         vector.beginCompoundOperation(false);
-        vector.pushBack(factory(value));
-        vector.pushBack(factory(value));
+        vector.pushBack(serializer.fromJSON(value));
+        vector.pushBack(serializer.fromJSON(value));
         vector.endCompoundOperation();
         expect(vector.canUndo).to.be(false);
       });
@@ -121,10 +126,10 @@ describe('notebook/common/undo', () => {
     describe('#endCompoundOperation()', () => {
 
       it('should end a compound operation', () => {
-        let vector = new ObservableUndoableVector(factory);
+        let vector = new ObservableUndoableVector(serializer);
         vector.beginCompoundOperation();
-        vector.pushBack(factory(value));
-        vector.pushBack(factory(value));
+        vector.pushBack(serializer.fromJSON(value));
+        vector.pushBack(serializer.fromJSON(value));
         vector.endCompoundOperation();
         expect(vector.canUndo).to.be(true);
         vector.undo();
@@ -136,39 +141,47 @@ describe('notebook/common/undo', () => {
     describe('#undo()', () => {
 
       it('should undo a pushBack', () => {
-        let vector = new ObservableUndoableVector(factory);
-        vector.pushBack(factory(value));
+        let vector = new ObservableUndoableVector(serializer);
+        vector.pushBack(serializer.fromJSON(value));
         vector.undo();
         expect(vector.length).to.be(0);
       });
 
       it('should undo a pushAll', () => {
-        let vector = new ObservableUndoableVector(factory);
-        vector.pushAll([factory(value), factory(value)]);
+        let vector = new ObservableUndoableVector(serializer);
+        vector.pushAll([serializer.fromJSON(value),
+                        serializer.fromJSON(value)]);
         vector.undo();
         expect(vector.length).to.be(0);
       });
 
       it('should undo a remove', () => {
-         let vector = new ObservableUndoableVector(factory);
-         vector.pushAll([factory(value), factory(value)]);
+         let vector = new ObservableUndoableVector(serializer);
+         vector.pushAll([serializer.fromJSON(value),
+                         serializer.fromJSON(value)]);
          vector.removeAt(0);
          vector.undo();
          expect(vector.length).to.be(2);
       });
 
       it('should undo a removeRange', () => {
-        let vector = new ObservableUndoableVector(factory);
-        vector.pushAll([factory(value), factory(value), factory(value),
-          factory(value), factory(value), factory(value)]);
+        let vector = new ObservableUndoableVector(serializer);
+        vector.pushAll([serializer.fromJSON(value),
+                        serializer.fromJSON(value),
+                        serializer.fromJSON(value),
+                        serializer.fromJSON(value),
+                        serializer.fromJSON(value),
+                        serializer.fromJSON(value)]);
         vector.removeRange(1, 3);
         vector.undo();
         expect(vector.length).to.be(6);
       });
 
       it('should undo a move', () => {
-        let items = [factory(value), factory(value), factory(value)];
-        let vector = new ObservableUndoableVector(factory);
+        let items = [serializer.fromJSON(value),
+                     serializer.fromJSON(value),
+                     serializer.fromJSON(value)];
+        let vector = new ObservableUndoableVector(serializer);
         vector.pushAll(items);
         vector.move(1, 2);
         vector.undo();
@@ -180,24 +193,26 @@ describe('notebook/common/undo', () => {
     describe('#redo()', () => {
 
       it('should redo a pushBack', () => {
-        let vector = new ObservableUndoableVector(factory);
-        vector.pushBack(factory(value));
+        let vector = new ObservableUndoableVector(serializer);
+        vector.pushBack(serializer.fromJSON(value));
         vector.undo();
         vector.redo();
         expect(vector.length).to.be(1);
       });
 
       it('should redo a pushAll', () => {
-        let vector = new ObservableUndoableVector(factory);
-        vector.pushAll([factory(value), factory(value)]);
+        let vector = new ObservableUndoableVector(serializer);
+        vector.pushAll([serializer.fromJSON(value),
+                        serializer.fromJSON(value)]);
         vector.undo();
         vector.redo();
         expect(vector.length).to.be(2);
       });
 
       it('should redo a remove', () => {
-         let vector = new ObservableUndoableVector(factory);
-         vector.pushAll([factory(value), factory(value)]);
+         let vector = new ObservableUndoableVector(serializer);
+         vector.pushAll([serializer.fromJSON(value),
+                         serializer.fromJSON(value)]);
          vector.removeAt(0);
          vector.undo();
          vector.redo();
@@ -205,9 +220,13 @@ describe('notebook/common/undo', () => {
       });
 
       it('should redo a removeRange', () => {
-        let vector = new ObservableUndoableVector(factory);
-        vector.pushAll([factory(value), factory(value), factory(value),
-          factory(value), factory(value), factory(value)]);
+        let vector = new ObservableUndoableVector(serializer);
+        vector.pushAll([serializer.fromJSON(value),
+                        serializer.fromJSON(value),
+                        serializer.fromJSON(value),
+                        serializer.fromJSON(value),
+                        serializer.fromJSON(value),
+                        serializer.fromJSON(value)]);
         vector.removeRange(1, 3);
         vector.undo();
         vector.redo();
@@ -215,8 +234,10 @@ describe('notebook/common/undo', () => {
       });
 
       it('should undo a move', () => {
-        let items = [factory(value), factory(value), factory(value)];
-        let vector = new ObservableUndoableVector(factory);
+        let items = [serializer.fromJSON(value),
+                     serializer.fromJSON(value),
+                     serializer.fromJSON(value)];
+        let vector = new ObservableUndoableVector(serializer);
         vector.pushAll(items);
         vector.move(1, 2);
         vector.undo();
@@ -229,8 +250,8 @@ describe('notebook/common/undo', () => {
     describe('#clearUndo()', () => {
 
       it('should clear the undo stack', () => {
-        let vector = new ObservableUndoableVector(factory);
-        vector.pushBack(factory(value));
+        let vector = new ObservableUndoableVector(serializer);
+        vector.pushBack(serializer.fromJSON(value));
         vector.clearUndo();
         expect(vector.canUndo).to.be(false);
       });