Merge pull request #1127 from blink1073/savehandler-test

Savehandler refactor and tests

src/docmanager/index.ts

@@ -2,3 +2,4 @@
 // Distributed under the terms of the Modified BSD License.
 
 export * from './manager';
+export * from './savehandler';

src/docmanager/manager.ts

@@ -21,6 +21,10 @@ import {
   IDisposable
 } from 'phosphor/lib/core/disposable';
 
+import {
+  AttachedProperty
+} from 'phosphor/lib/core/properties';
+
 import {
   Widget
 } from 'phosphor/lib/ui/widget';
@@ -29,6 +33,10 @@ import {
   DocumentRegistry, Context
 } from '../docregistry';
 
+import {
+  SaveHandler
+} from './savehandler';
+
 import {
   DocumentWidgetManager
 } from './widgetmanager';
@@ -269,6 +277,14 @@ class DocumentManager implements IDisposable {
       factory,
       path
     });
+    let handler = new SaveHandler({
+      context,
+      manager: this._serviceManager
+    });
+    Private.saveHandlerProperty.set(context, handler);
+    context.populated.connect(() => {
+      handler.start();
+    });
     context.disposed.connect(() => {
       this._contexts.remove(context);
     });
@@ -332,3 +348,17 @@ namespace DocumentManager {
     open(widget: Widget): void;
   }
 }
+
+
+/**
+ * A namespace for private data.
+ */
+namespace Private {
+  /**
+   * An attached property for a context save handler.
+   */
+  export
+  const saveHandlerProperty = new AttachedProperty<DocumentRegistry.IContext<DocumentRegistry.IModel>, SaveHandler>({
+    name: 'saveHandler'
+  });
+}

src/docregistry/savehandler.ts → src/docmanager/savehandler.ts

@@ -36,20 +36,35 @@ class SaveHandler implements IDisposable {
   constructor(options: SaveHandler.IOptions) {
     this._manager = options.manager;
     this._context = options.context;
-    this._minInterval = options.saveInterval || 120;
+    this._minInterval = options.saveInterval * 1000 || 120000;
     this._interval = this._minInterval;
     // Restart the timer when the contents model is updated.
-    this._context.fileChanged.connect(() => {
-      this._setTimer();
-    });
+    this._context.fileChanged.connect(this._setTimer, this);
     this._context.disposed.connect(this.dispose, this);
   }
 
+  /**
+   * The save interval used by the timer (in seconds).
+   */
+  get saveInterval(): number {
+    return this._interval / 1000;
+  }
+  set saveInterval(value: number) {
+    this._minInterval = this._interval = value * 1000;
+    if (this._isActive) {
+      this._setTimer();
+    }
+  }
+
+  /**
+   * Get whether the handler is active.
+   */
+  get isActive(): boolean {
+    return this._isActive;
+  }
+
   /**
    * Get whether the save handler is disposed.
-   *
-   * #### Notes
-   * This is a read-only property.
    */
   get isDisposed(): boolean {
     return this._context === null;
@@ -71,7 +86,7 @@ class SaveHandler implements IDisposable {
    * Start the autosaver.
    */
   start(): void {
-    this._stopped = false;
+    this._isActive = true;
     this._setTimer();
   }
 
@@ -79,7 +94,7 @@ class SaveHandler implements IDisposable {
    * Stop the autosaver.
    */
   stop(): void {
-    this._stopped = true;
+    this._isActive = false;
     clearTimeout(this._autosaveTimer);
   }
 
@@ -88,12 +103,12 @@ class SaveHandler implements IDisposable {
    */
   private _setTimer(): void {
     clearTimeout(this._autosaveTimer);
-    if (this._stopped) {
+    if (!this._isActive) {
       return;
     }
     this._autosaveTimer = setTimeout(() => {
       this._save();
-    }, this._interval * 1000);
+    }, this._interval);
   }
 
   /**
@@ -105,6 +120,10 @@ class SaveHandler implements IDisposable {
     // Trigger the next update.
     this._setTimer();
 
+    if (!context) {
+      return;
+    }
+
     // Bail if the model is not dirty or it is read only, or the dialog
     // is already showing.
     if (!context.model.dirty || context.model.readOnly || this._inDialog) {
@@ -112,13 +131,17 @@ class SaveHandler implements IDisposable {
     }
 
     // Make sure the file has not changed on disk.
-    this._manager.contents.get(context.path).then(model => {
-      if (model.last_modified !== context.contentsModel.last_modified) {
+    let promise = this._manager.contents.get(context.path);
+    promise.then(model => {
+      if (context.contentsModel &&
+          model.last_modified !== context.contentsModel.last_modified) {
         return this._timeConflict(model.last_modified);
       }
       return this._finishSave();
+    }, (err) => {
+      return this._finishSave();
     }).catch(err => {
-      console.error('Error in Auto-Save', err);
+      console.error('Error in Auto-Save', err.message);
     });
   }
 
@@ -168,7 +191,7 @@ class SaveHandler implements IDisposable {
   private _interval = -1;
   private _context: DocumentRegistry.IContext<DocumentRegistry.IModel> = null;
   private _manager: ServiceManager.IManager = null;
-  private _stopped = false;
+  private _isActive = false;
   private _inDialog = false;
 }
 

src/docregistry/context.ts

@@ -33,10 +33,6 @@ import {
   DocumentRegistry
 } from '../docregistry';
 
-import {
-  SaveHandler
-} from './savehandler';
-
 
 /**
  * An implementation of a document context.
@@ -57,7 +53,6 @@ class Context<T extends DocumentRegistry.IModel> implements DocumentRegistry.ICo
     let lang = this._factory.preferredLanguage(ext);
     this._model = this._factory.createNew(lang);
     manager.sessions.runningChanged.connect(this._onSessionsChanged, this);
-    this._saver = new SaveHandler({ context: this, manager });
   }
 
   /**
@@ -221,10 +216,10 @@ class Context<T extends DocumentRegistry.IModel> implements DocumentRegistry.ICo
     };
 
     let promise = this._manager.contents.save(path, options);
-
     return promise.then(value => {
-      this._updateContentsModel(value);
       model.dirty = false;
+      this._updateContentsModel(value);
+
       if (!this._isPopulated) {
         return this._populate();
       }
@@ -418,9 +413,9 @@ class Context<T extends DocumentRegistry.IModel> implements DocumentRegistry.ICo
       mimetype: model.mimetype,
       format: model.format
     };
-    let prevModel = this._contentsModel;
+    let mod = this._contentsModel ? this._contentsModel.last_modified : null;
     this._contentsModel = newModel;
-    if (!prevModel || newModel.last_modified !== prevModel.last_modified) {
+    if (!mod || newModel.last_modified !== mod) {
       this.fileChanged.emit(newModel);
     }
   }
@@ -446,7 +441,6 @@ class Context<T extends DocumentRegistry.IModel> implements DocumentRegistry.ICo
    */
   private _populate(): Promise<void> {
     this._isPopulated = true;
-    this._saver.start();
     // Add a checkpoint if none exists.
     return this.listCheckpoints().then(checkpoints => {
       if (!checkpoints) {
@@ -463,7 +457,6 @@ class Context<T extends DocumentRegistry.IModel> implements DocumentRegistry.ICo
   private _path = '';
   private _session: Session.ISession = null;
   private _factory: DocumentRegistry.IModelFactory<T> = null;
-  private _saver: SaveHandler = null;
   private _isPopulated = false;
   private _contentsModel: Contents.IModel = null;
 }

test/src/docmanager/savehandler.spec.ts

@@ -0,0 +1,204 @@
+// Copyright (c) Jupyter Development Team.
+// Distributed under the terms of the Modified BSD License.
+
+import expect = require('expect.js');
+
+import {
+  ServiceManager, utils
+} from '@jupyterlab/services';
+
+import {
+  Context, DocumentRegistry, TextModelFactory
+} from '../../../lib/docregistry';
+
+import {
+  SaveHandler
+} from '../../../lib/docmanager';
+
+import {
+  acceptDialog, waitForDialog
+} from '../utils';
+
+
+describe('docregistry/savehandler', () => {
+
+  let manager: ServiceManager.IManager;
+  let factory = new TextModelFactory();
+  let context: Context<DocumentRegistry.IModel>;
+  let handler: SaveHandler;
+
+  before((done) => {
+    ServiceManager.create().then(m => {
+      manager = m;
+      done();
+    }).catch(done);
+  });
+
+  beforeEach(() => {
+    context = new Context({ manager, factory, path: utils.uuid() });
+    handler = new SaveHandler({ context, manager });
+  });
+
+  afterEach(() => {
+    context.dispose();
+    handler.dispose();
+  });
+
+  describe('SaveHandler', () => {
+
+    describe('#constructor()', () => {
+
+      it('should create a new save handler', () => {
+        expect(handler).to.be.a(SaveHandler);
+      });
+
+    });
+
+    describe('#saveInterval()', () => {
+
+      it('should be the save interval of the handler', () => {
+        expect(handler.saveInterval).to.be(120);
+      });
+
+      it('should be set-able', () => {
+        handler.saveInterval = 200;
+        expect(handler.saveInterval).to.be(200);
+      });
+
+    });
+
+    describe('#isActive', () => {
+
+      it('should test whether the handler is active', () => {
+        expect(handler.isActive).to.be(false);
+        handler.start();
+        expect(handler.isActive).to.be(true);
+      });
+
+    });
+
+    describe('#isDisposed', () => {
+
+      it('should test whether the handler is disposed', () => {
+        expect(handler.isDisposed).to.be(false);
+        handler.dispose();
+        expect(handler.isDisposed).to.be(true);
+      });
+
+      it('should be true after the context is disposed', () => {
+        context.dispose();
+        expect(handler.isDisposed).to.be(true);
+      });
+
+    });
+
+    describe('#dispose()', () => {
+
+      it('should dispose of the resources used by the handler', () => {
+        expect(handler.isDisposed).to.be(false);
+        handler.dispose();
+        expect(handler.isDisposed).to.be(true);
+        handler.dispose();
+        expect(handler.isDisposed).to.be(true);
+      });
+
+    });
+
+    describe('#start()', () => {
+
+      it('should start the save handler', () => {
+        handler.start();
+        expect(handler.isActive).to.be(true);
+      });
+
+      it('should trigger a save', (done) => {
+        context.fileChanged.connect(() => {
+          done();
+        });
+        context.model.fromString('bar');
+        expect(handler.isActive).to.be(false);
+        handler.saveInterval = 1;
+        handler.start();
+      });
+
+      it('should continue to save', (done) => {
+        let called = 0;
+        context.fileChanged.connect(() => {
+          if (called === 0) {
+            context.model.fromString('bar');
+            called++;
+          } else {
+            done();
+          }
+        });
+        context.model.fromString('foo');
+        expect(handler.isActive).to.be(false);
+        handler.saveInterval = 1;
+        handler.start();
+      });
+
+      it('should overwrite the file on disk', (done) => {
+        context.model.fromString('foo');
+        context.save().then(() => {
+          setTimeout(() => {
+            manager.contents.save(context.path, {
+              type: factory.contentType,
+              format: factory.fileFormat,
+              content: 'bar'
+            }).catch(done);
+            handler.saveInterval = 1;
+            handler.start();
+            context.model.fromString('baz');
+            context.fileChanged.connect(() => {
+              expect(context.model.toString()).to.be('baz');
+              done();
+            });
+          }, 1000);  // The server has a one second resolution for saves.
+        }).catch(done);
+        acceptDialog().catch(done);
+      });
+
+      it('should revert to the file on disk', (done) => {
+        context.model.fromString('foo');
+        context.save().then(() => {
+          setTimeout(() => {
+            manager.contents.save(context.path, {
+              type: factory.contentType,
+              format: factory.fileFormat,
+              content: 'bar'
+            }).catch(done);
+            handler.saveInterval = 1;
+            handler.start();
+            context.model.fromString('baz');
+            context.fileChanged.connect(() => {
+              expect(context.model.toString()).to.be('bar');
+              done();
+            });
+          }, 1000);  // The server has a one second resolution for saves.
+        }).catch(done);
+        waitForDialog().then(() => {
+          let dialog = document.body.getElementsByClassName('jp-Dialog')[0];
+          let buttons = dialog.getElementsByTagName('button');
+          for (let i = 0; i < buttons.length; i++) {
+            if (buttons[i].textContent === 'REVERT') {
+              buttons[i].click();
+            }
+          }
+        });
+      });
+
+    });
+
+    describe('#stop()', () => {
+
+      it('should stop the save timer', () => {
+        handler.start();
+        expect(handler.isActive).to.be(true);
+        handler.stop();
+      });
+
+    });
+
+  });
+
+});

test/src/index.ts

@@ -15,10 +15,13 @@ import './console/history.spec';
 
 import './dialog/dialog.spec';
 
+import './docmanager/savehandler.spec';
+
 import './docregistry/context.spec';
 import './docregistry/default.spec';
 import './docregistry/registry.spec';
 
+
 import './filebrowser/model.spec';
 
 import './inspector/inspector.spec';