فهرست منبع

Merge pull request #1069 from blink1073/filebrowser-model-test

Filebrowser model cleanup and tests
Afshin Darian 8 سال پیش
والد
کامیت
1804cfb045
5فایلهای تغییر یافته به همراه403 افزوده شده و 115 حذف شده
  1. 7 4
      src/filebrowser/browser.ts
  2. 8 4
      src/filebrowser/dialogs.ts
  3. 24 14
      src/filebrowser/listing.ts
  4. 55 84
      src/filebrowser/model.ts
  5. 309 9
      test/src/filebrowser/model.spec.ts

+ 7 - 4
src/filebrowser/browser.ts

@@ -5,6 +5,10 @@ import {
   Contents
 } from 'jupyter-js-services';
 
+import {
+  each
+} from 'phosphor/lib/algorithm/iteration';
+
 import {
   Message
 } from 'phosphor/lib/core/messaging';
@@ -190,10 +194,9 @@ class FileBrowserWidget extends Widget {
    */
   open(): void {
     let foundDir = false;
-    let items = this._model.items;
-    for (let item of items) {
+    each(this._model.items, item => {
       if (!this._listing.isSelected(item.name)) {
-        continue;
+        return;
       }
       if (item.type === 'directory') {
         if (!foundDir) {
@@ -205,7 +208,7 @@ class FileBrowserWidget extends Widget {
       } else {
         this.openPath(item.path);
       }
-    }
+    });
   }
 
   /**

+ 8 - 4
src/filebrowser/dialogs.ts

@@ -5,6 +5,10 @@ import {
   Contents, Kernel, Session
 } from 'jupyter-js-services';
 
+import {
+  each
+} from 'phosphor/lib/algorithm/iteration';
+
 import {
   Widget
 } from 'phosphor/lib/ui/widget';
@@ -240,12 +244,12 @@ class CreateFromHandler extends Widget {
     this.inputNode.addEventListener('input', () => {
       let value = this.inputNode.value;
       if (value !== this._orig) {
-        for (let item of this._model.items) {
+        each(this._model.items, item => {
           if (item.name === value) {
             this.addClass(FILE_CONFLICT_CLASS);
             return;
           }
-        }
+        });
       }
       this.removeClass(FILE_CONFLICT_CLASS);
     });
@@ -473,12 +477,12 @@ class CreateNewHandler extends Widget {
    */
   protected inputNodeChanged(): void {
     let path = this.inputNode.value;
-    for (let item of this._model.items) {
+    each(this._model.items, item => {
       if (item.path === path) {
         this.addClass(FILE_CONFLICT_CLASS);
         return;
       }
-    }
+    });
     let ext = this.ext;
     if (ext === this._prevExt) {
       return;

+ 24 - 14
src/filebrowser/listing.ts

@@ -5,10 +5,18 @@ import {
   Contents
 } from 'jupyter-js-services';
 
+import {
+  each
+} from 'phosphor/lib/algorithm/iteration';
+
 import {
   find, findIndex
 } from 'phosphor/lib/algorithm/searching';
 
+import {
+  ISequence
+} from 'phosphor/lib/algorithm/sequence';
+
 import {
   Message
 } from 'phosphor/lib/core/messaging';
@@ -360,12 +368,11 @@ class DirListing extends Widget {
    */
   delete(): Promise<void> {
     let names: string[] = [];
-    let items = this._model.items;
-    for (let item of items) {
+    each(this._model.items, item => {
       if (this._selection[item.name]) {
         names.push(item.name);
       }
-    }
+    });
     let message = `Permanently delete these ${names.length} files?`;
     if (names.length === 1) {
       message = `Permanently delete file "${names[0]}"?`;
@@ -418,12 +425,12 @@ class DirListing extends Widget {
     let promises: Promise<void>[] = [];
     let items = this.sortedItems;
     let paths = items.map(item => item.path);
-    for (let session of this._model.sessions) {
+    each(this._model.sessions, session => {
       let index = paths.indexOf(session.notebook.path);
       if (this._selection[items[index].name]) {
         promises.push(this._model.shutdown(session.id));
       }
-    }
+    });
     return Promise.all(promises).then(
       () => this._model.refresh(),
       error => utils.showErrorMessage(this, 'Shutdown kernel', error)
@@ -658,12 +665,12 @@ class DirListing extends Widget {
     // Handle notebook session statuses.
     let paths = items.map(item => item.path);
     let specs = this._model.kernelspecs;
-    for (let session of this._model.sessions) {
+    each(this._model.sessions, session => {
       let index = paths.indexOf(session.notebook.path);
       let node = this._items[index];
       node.classList.add(RUNNING_CLASS);
       node.title = specs.kernelspecs[session.kernel.name].spec.display_name;
-    }
+    });
 
     this._prevPath = this._model.path;
   }
@@ -1186,12 +1193,12 @@ class DirListing extends Widget {
     // Update the selection.
     let existing = Object.keys(this._selection);
     this._selection = Object.create(null);
-    for (let item of this._model.items) {
+    each(this._model.items, item => {
       let name = item.name;
       if (existing.indexOf(name) !== -1) {
         this._selection[name] = true;
       }
-    }
+    });
     // Update the sorted items.
     this.sort(this.sortState);
   }
@@ -1595,10 +1602,13 @@ namespace Private {
    * Sort a list of items by sort state as a new array.
    */
   export
-  function sort(items: Contents.IModel[], state: DirListing.ISortState) : Contents.IModel[] {
-    let output = items.slice();
+  function sort(items: ISequence<Contents.IModel>, state: DirListing.ISortState) : Contents.IModel[] {
+    let copy: Contents.IModel[] = [];
+    each(items, item => {
+      copy.push(item);
+    });
     if (state.key === 'last_modified') {
-      output.sort((a, b) => {
+      copy.sort((a, b) => {
         let valA = new Date(a.last_modified).getTime();
         let valB = new Date(b.last_modified).getTime();
         return valB - valA;
@@ -1607,8 +1617,8 @@ namespace Private {
 
     // Reverse the order if descending.
     if (state.direction === 'descending') {
-      output.reverse();
+      copy.reverse();
     }
-    return output;
+    return copy;
   }
 }

+ 55 - 84
src/filebrowser/model.ts

@@ -2,12 +2,16 @@
 // Distributed under the terms of the Modified BSD License.
 
 import {
-  Contents, Kernel, IServiceManager, Session
+  Contents, ContentsManager, Kernel, IServiceManager, Session
 } from 'jupyter-js-services';
 
 import {
-  deepEqual
-} from 'phosphor/lib/algorithm/json';
+  ISequence
+} from 'phosphor/lib/algorithm/sequence';
+
+import {
+  Vector
+} from 'phosphor/lib/collections/vector';
 
 import {
   IDisposable
@@ -22,7 +26,6 @@ import {
 } from '../common/interfaces';
 
 
-
 /**
  * An implementation of a file browser model.
  *
@@ -37,7 +40,7 @@ class FileBrowserModel implements IDisposable {
    */
   constructor(options: FileBrowserModel.IOptions) {
     this._manager = options.manager;
-    this._model = { path: '', name: '/', type: 'directory', content: [] };
+    this._model = { path: '', name: '/', type: 'directory' };
     this.cd();
     this._manager.sessions.runningChanged.connect(this._onRunningChanged, this);
   }
@@ -59,36 +62,30 @@ class FileBrowserModel implements IDisposable {
 
   /**
    * Get the current path.
-   *
-   * #### Notes
-   * This is a read-only property.
    */
   get path(): string {
-    return this._model.path;
+    return this._model ? this._model.path : '';
   }
 
   /**
    * Get a read-only list of the items in the current path.
    */
-  get items(): Contents.IModel[] {
-    return this._model.content ? this._model.content.slice() : [];
+  get items(): ISequence<Contents.IModel> {
+    return this._items;
   }
 
   /**
-   * Get whether the view model is disposed.
+   * Get whether the model is disposed.
    */
   get isDisposed(): boolean {
     return this._model === null;
   }
 
   /**
-   * Get the session models for active notebooks.
-   *
-   * #### Notes
-   * This is a read-only property.
+   * Get the session models for active notebooks in the current directory.
    */
-  get sessions(): Session.IModel[] {
-    return this._sessions.slice();
+  get sessions(): ISequence<Session.IModel> {
+    return this._sessions;
   }
 
   /**
@@ -99,10 +96,15 @@ class FileBrowserModel implements IDisposable {
   }
 
   /**
-   * Dispose of the resources held by the view model.
+   * Dispose of the resources held by the model.
    */
   dispose(): void {
+    if (this.isDisposed) {
+      return;
+    }
     this._model = null;
+    this._sessions.clear();
+    this._items.clear();
     this._manager = null;
     clearSignalData(this);
   }
@@ -120,7 +122,7 @@ class FileBrowserModel implements IDisposable {
     }
     // Collapse requests to the same directory.
     if (newValue === this._pendingPath) {
-      return Promise.resolve(void 0);
+      return this._pending;
     }
     let oldValue = this.path;
     let options: Contents.IFetchOptions = { content: true };
@@ -129,13 +131,18 @@ class FileBrowserModel implements IDisposable {
       newValue = this.path;
     }
     if (oldValue !== newValue) {
-      this._sessions = [];
+      this._sessions.clear();
     }
-    this._pending = this._manager.contents.get(newValue, options).then(contents => {
-      this._model = contents;
-      return this._manager.sessions.listRunning();
+    let manager = this._manager;
+    this._pending = manager.contents.get(newValue, options).then(contents => {
+      this._handleContents(contents);
+      this._pendingPath = null;
+      return manager.sessions.listRunning();
     }).then(models => {
-      this._onRunningChanged(this._manager.sessions, models);
+      if (this.isDisposed) {
+        return;
+      }
+      this._onRunningChanged(manager.sessions, models);
       if (oldValue !== newValue) {
         this.pathChanged.emit({
           name: 'path',
@@ -144,7 +151,6 @@ class FileBrowserModel implements IDisposable {
         });
       }
       this.refreshed.emit(void 0);
-      this._pendingPath = null;
     });
     return this._pending;
   }
@@ -198,7 +204,7 @@ class FileBrowserModel implements IDisposable {
     return this._manager.contents.delete(path).then(() => {
       this.fileChanged.emit({
         name: 'file',
-        oldValue: {path: path},
+        oldValue: { path: path },
         newValue: void 0
       });
     });
@@ -299,7 +305,7 @@ class FileBrowserModel implements IDisposable {
     path = path ? path + '/' + file.name : file.name;
     return this._manager.contents.get(path, {}).then(() => {
       let msg = `"${file.name}" already exists`;
-      return Private.typedThrow<Contents.IModel>(msg);
+      throw new Error(msg);
     }, () => {
       return this._upload(file);
     });
@@ -359,25 +365,29 @@ class FileBrowserModel implements IDisposable {
 
   }
 
+  /**
+   * Handle an updated contents model.
+   */
+  private _handleContents(contents: Contents.IModel): void {
+    // Update our internal data.
+    this._model = contents;
+    this._items.clear();
+    this._paths = contents.content.map((model: Contents.IModel) => {
+      return model.path;
+    });
+    this._items = new Vector<Contents.IModel>(contents.content);
+    this._model.content = null;
+  }
+
   /**
    * Handle a change to the running sessions.
    */
   private _onRunningChanged(sender: Session.IManager, models: Session.IModel[]): void {
-    if (deepEqual(models, this._sessions)) {
-      return;
-    }
-    this._sessions = [];
-    if (!models.length) {
-      this.refreshed.emit(void 0);
-      return;
-    }
-    let paths = this._model.content.map((contents: Contents.IModel) => {
-      return contents.path;
-    });
+    this._sessions.clear();
     for (let model of models) {
-      let index = paths.indexOf(model.notebook.path);
+      let index = this._paths.indexOf(model.notebook.path);
       if (index !== -1) {
-        this._sessions.push(model);
+        this._sessions.pushBack(model);
       }
     }
     this.refreshed.emit(void 0);
@@ -385,7 +395,9 @@ class FileBrowserModel implements IDisposable {
 
   private _maxUploadSizeMb = 15;
   private _manager: IServiceManager = null;
-  private _sessions: Session.IModel[] = [];
+  private _sessions = new Vector<Session.IModel>();
+  private _items = new Vector<Contents.IModel>();
+  private _paths: string[] = [];
   private _model: Contents.IModel;
   private _pendingPath: string = null;
   private _pending: Promise<void> = null;
@@ -446,47 +458,6 @@ namespace Private {
    */
   export
   function normalizePath(root: string, path: string): string {
-    // Current directory
-    if (path === '.') {
-      return root;
-    }
-    // Root path.
-    if (path.indexOf('/') === 0) {
-      path = path.slice(1, path.length);
-      root = '';
-    // Current directory.
-    } else if (path.indexOf('./') === 0) {
-      path = path.slice(2, path.length);
-    // Grandparent directory.
-    } else if (path.indexOf('../../') === 0) {
-      let parts = root.split('/');
-      root = parts.splice(0, parts.length - 2).join('/');
-      path = path.slice(6, path.length);
-    // Parent directory.
-    } else if (path.indexOf('../') === 0) {
-      let parts = root.split('/');
-      root = parts.splice(0, parts.length - 1).join('/');
-      path = path.slice(3, path.length);
-    } else {
-      // Current directory.
-    }
-    if (path[path.length - 1] === '/') {
-      path = path.slice(0, path.length - 1);
-    }
-    // Combine the root and the path if necessary.
-    if (root && path) {
-      path = root + '/' + path;
-    } else if (root) {
-      path = root;
-    }
-    return path;
-  }
-
-  /**
-   * Work around TS 1.8 type inferencing in promises which only throw.
-   */
-  export
-  function typedThrow<T>(msg: string): T {
-    throw new Error(msg);
+    return ContentsManager.getAbsolutePath(path, root);
   }
 }

+ 309 - 9
test/src/filebrowser/model.spec.ts

@@ -4,7 +4,7 @@
 import expect = require('expect.js');
 
 import {
-  ServiceManager
+  ServiceManager, IServiceManager, ISession
 } from 'jupyter-js-services';
 
 import {
@@ -14,90 +14,390 @@ import {
 
 describe('filebrowser/model', () => {
 
+  let manager: IServiceManager;
+  let model: FileBrowserModel;
+  let name: string;
+
+  before((done) => {
+    ServiceManager.create().then(m => {
+      manager = m;
+      done();
+    });
+  });
+
+  beforeEach((done) => {
+    model = new FileBrowserModel({ manager });
+    model.newUntitled({ type: 'file' }).then(contents => {
+      name = contents.name;
+      return model.refresh();
+    }).then(done, done);
+  });
+
+  afterEach(() => {
+    model.dispose();
+  });
+
   describe('FileBrowserModel', () => {
 
     describe('#constructor()', () => {
 
-      it('should construct a new file browser model', (done) => {
-        ServiceManager.create().then(manager => {
-          let model = new FileBrowserModel({ manager });
-          expect(model).to.be.a(FileBrowserModel);
-          done();
-        });
+      it('should construct a new file browser model', () => {
+        model = new FileBrowserModel({ manager });
+        expect(model).to.be.a(FileBrowserModel);
       });
 
     });
 
     describe('#pathChanged', () => {
 
-      it('should be emitted when the path changes', () => {
-
+      it('should be emitted when the path changes', (done) => {
+        model.pathChanged.connect((sender, args) => {
+          expect(sender).to.be(model);
+          expect(args.name).to.be('path');
+          expect(args.oldValue).to.be('');
+          expect(args.newValue).to.be('src');
+          done();
+        });
+        model.cd('src').catch(done);
       });
 
     });
 
     describe('#refreshed', () => {
 
+      it('should be emitted after a refresh', (done) => {
+        model.refreshed.connect((sender, arg) => {
+          expect(sender).to.be(model);
+          expect(arg).to.be(void 0);
+          done();
+        });
+        model.refresh().catch(done);
+      });
+
+      it('should be emitted when the path changes', (done) => {
+        model.refreshed.connect((sender, arg) => {
+          expect(sender).to.be(model);
+          expect(arg).to.be(void 0);
+          done();
+        });
+        model.cd('src').catch(done);
+      });
+
     });
 
     describe('#fileChanged', () => {
 
+      it('should be emitted when a file is created', (done) => {
+        model.fileChanged.connect((sender, args) => {
+          expect(sender).to.be(model);
+          expect(args.name).to.be('file');
+          expect(args.oldValue).to.be(void 0);
+          expect(args.newValue.type).to.be('file');
+          done();
+        });
+        model.newUntitled({ type: 'file' }).catch(done);
+      });
+
+      it('should be emitted when a file is renamed', (done) => {
+        model.fileChanged.connect((sender, args) => {
+          expect(sender).to.be(model);
+          expect(args.name).to.be('file');
+          expect(args.oldValue.path).to.be(name);
+          expect(args.newValue.path).to.be(name + '.bak');
+          done();
+        });
+        model.rename(name, name + '.bak').catch(done);
+      });
+
     });
 
     describe('#path', () => {
 
+      it('should be the current path of the model', (done) => {
+        expect(model.path).to.be('');
+        model.cd('src/').then(() => {
+          expect(model.path).to.be('src');
+          done();
+        }).catch(done);
+      });
+
     });
 
     describe('#items', () => {
 
+      it('should get a read-only sequence of items in the current path', () => {
+        let items = model.items;
+        expect(items.length).to.be.above(0);
+      });
+
     });
 
     describe('#isDisposed', () => {
 
+      it('should test whether the model is disposed', () => {
+        expect(model.isDisposed).to.be(false);
+        model.dispose();
+        expect(model.isDisposed).to.be(true);
+      });
+
     });
 
     describe('#sessions', () => {
 
+      it('should be the session models for the active notebooks', (done) => {
+        let session: ISession;
+        model.newUntitled({ type: 'notebook' }).then(contents => {
+          return manager.sessions.startNew({ path: contents.path });
+        }).then(s => {
+          session = s;
+          return model.refresh();
+        }).then(() => {
+          expect(model.sessions.length).to.be.above(0);
+          return session.shutdown();
+        }).then(() => {
+          done();
+        }).catch(done);
+      });
+
     });
 
     describe('#kernelspecs', () => {
 
+      it('should get the kernelspecs models', () => {
+        expect(model.kernelspecs.default).to.be(manager.kernelspecs.default);
+      });
+
     });
 
     describe('#dispose()', () => {
 
+      it('should dispose of the resources held by the model', () => {
+        model.dispose();
+        expect(model.isDisposed).to.be(true);
+      });
+
+      it('should be safe to call more than once', () => {
+        model.dispose();
+        model.dispose();
+        expect(model.isDisposed).to.be(true);
+      });
+
     });
 
     describe('#cd()', () => {
 
+      it('should change directory', (done) => {
+        model.cd('src').then(() => {
+          expect(model.path).to.be('src');
+          done();
+        }).catch(done);
+      });
+
+      it('should accept a relative path', (done) => {
+        model.cd('./src').then(() => {
+          expect(model.path).to.be('src');
+          done();
+        }).catch(done);
+      });
+
+      it('should accept a parent directory', (done) => {
+        model.cd('src').then(() => {
+          return model.cd('..');
+        }).then(() => {
+          expect(model.path).to.be('');
+          done();
+        }).catch(done);
+      });
+
     });
 
     describe('#refresh()', () => {
 
+      it('should refresh the current directory', (done) => {
+        let len = model.items.length;
+        model.newUntitled({ type: 'file' }).then(contents => {
+          expect(model.items.length).to.be(len);
+          return model.refresh();
+        }).then(() => {
+          expect(model.items.length).to.be(len + 1);
+          done();
+        }).catch(done);
+      });
+
+    });
+
+    describe('#copy()', () => {
+
+      it('should copy a file', (done) => {
+        model.copy(name, 'src').then(contents => {
+          expect(contents.path).to.be(`src/${name}`);
+          done();
+        }).catch(done);
+      });
+
+      it('should emit a fileChanged signal', (done) => {
+        model.fileChanged.connect((sender, args) => {
+          expect(sender).to.be(model);
+          expect(args.name).to.be('file');
+          expect(args.oldValue).to.be(void 0);
+          expect(args.newValue.path).to.be(`src/${name}`);
+          done();
+        });
+        model.copy(name, 'src').catch(done);
+      });
+
     });
 
     describe('#deleteFile()', () => {
 
+      it('should delete a file ', (done) => {
+        let len = model.items.length;
+        model.deleteFile(name).then(() => {
+          return model.refresh();
+        }).then(() => {
+          expect(model.items.length).to.be(len - 1);
+          done();
+        }).catch(done);
+      });
+
+      it('should emit a fileChanged signal', (done) => {
+        model.fileChanged.connect((sender, args) => {
+          expect(sender).to.be(model);
+          expect(args.name).to.be('file');
+          expect(args.oldValue.path).to.be(name);
+          expect(args.newValue).to.be(void 0);
+          done();
+        });
+        model.deleteFile(name).catch(done);
+      });
+
     });
 
     describe('#download()', () => {
 
+      it('should download the file without error', () => {
+        // TODO: how to test this?
+      });
+
     });
 
     describe('#newUntitled()', () => {
 
+      it('should create a new untitled file in the current directory', (done) => {
+        model.cd('src').then(() => {
+          return model.newUntitled({ type: 'file', ext: '.py' });
+        }).then(contents => {
+          expect(contents.path.indexOf('src/')).to.be(0);
+          expect(contents.path.indexOf('.py')).to.not.be(-1);
+          done();
+        }).catch(done);
+      });
+
+      it('should emit a fileChanged signal', (done) => {
+        model.fileChanged.connect((sender, args) => {
+          expect(sender).to.be(model);
+          expect(args.name).to.be('file');
+          expect(args.oldValue).to.be(void 0);
+          expect(args.newValue.type).to.be('directory');
+          done();
+        });
+        model.newUntitled({ type: 'directory' }).catch(done);
+      });
+
     });
 
     describe('#rename()', () => {
 
+      it('should rename a file', (done) => {
+        model.rename(name, name + '.bak').then(contents => {
+          expect(contents.name).to.be(name + '.bak');
+          done();
+        }).catch(done);
+      });
+
+      it('should emit the fileChanged signal', (done) => {
+        model.fileChanged.connect((sender, args) => {
+          expect(sender).to.be(model);
+          expect(args.name).to.be('file');
+          expect(args.oldValue.path).to.be(name);
+          expect(args.newValue.path).to.be(name + '.new');
+          done();
+        });
+        model.rename(name, name + '.new').catch(done);
+      });
+
     });
 
     describe('#upload()', () => {
 
+      it('should upload a file object', (done) => {
+        let file = new File(['<p>Hello world!</p>'], 'hello.html',
+                            { type: 'text/html' });
+        model.upload(file).then(contents => {
+          expect(contents.name).to.be('hello.html');
+          done();
+        }).catch(done);
+      });
+
+      it('should allow overwrite', (done) => {
+        let file = new File(['<p>Hello world!</p>'], 'hello2.html',
+                            { type: 'text/html' });
+        model.upload(file).then(contents => {
+          expect(contents.name).to.be('hello2.html');
+          return model.upload(file, true);
+        }).then(contents => {
+          expect(contents.name).to.be('hello2.html');
+          done();
+        }).catch(done);
+      });
+
+      it('should fail without an overwrite if the file exists', (done) => {
+        let file = new File(['<p>Hello world!</p>'], 'hello2.html',
+                            { type: 'text/html' });
+        model.upload(file).then(contents => {
+          expect(contents.name).to.be('hello2.html');
+          return model.upload(file);
+        }).catch(err => {
+          expect(err.message).to.be(`"${file.name}" already exists`);
+          done();
+        });
+      });
+
+      it('should emit the fileChanged signal', (done) => {
+        model.fileChanged.connect((sender, args) => {
+          expect(sender).to.be(model);
+          expect(args.name).to.be('file');
+          expect(args.oldValue).to.be(void 0);
+          expect(args.newValue.path).to.be('hello3.html');
+          done();
+        });
+        let file = new File(['<p>Hello world!</p>'], 'hello3.html',
+                            { type: 'text/html' });
+        model.upload(file).catch(done);
+      });
+
     });
 
     describe('#shutdown()', () => {
 
+      it('should shut down a session by session id', (done) => {
+        let length = 0;
+        manager.sessions.listRunning().then(running => {
+          length = running.length;
+          return model.newUntitled({ type: 'notebook' });
+        }).then(contents => {
+          return manager.sessions.startNew({ path: contents.path });
+        }).then(session => {
+          session.dispose();
+          return model.shutdown(session.id);
+        }).then(() => {
+          return manager.sessions.listRunning();
+        }).then(running => {
+          expect(running.length).to.be(length);
+          done();
+        }).catch(done);
+      });
+
     });
 
   });