Skip to content

Commit

Permalink
Fix tables delete bug in perspective-workspace
Browse files Browse the repository at this point in the history
  • Loading branch information
zemeolotu authored and texodus committed Feb 24, 2020
1 parent e95f4c4 commit e201021
Show file tree
Hide file tree
Showing 3 changed files with 83 additions and 9 deletions.
19 changes: 10 additions & 9 deletions packages/perspective-workspace/src/js/workspace/workspace.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,15 +40,19 @@ class ObservableMap extends Map {
}

delete(name) {
this._delete_listener?.(name);
super.delete(name);
const result = this._delete_listener?.(name);
if (result) {
return super.delete(name);
} else {
return false;
}
}

addSetListener(listener) {
this._set_listener = listener;
}

addDeleteistener(listener) {
addDeleteListener(listener) {
this._delete_listener = listener;
}
}
Expand Down Expand Up @@ -78,7 +82,7 @@ export class PerspectiveWorkspace extends DiscreteSplitPanel {
this.listeners = new WeakMap();
this._tables = new ObservableMap();
this._tables.addSetListener(this._set_listener.bind(this));
this._tables.addDeleteistener(this._delete_listener.bind(this));
this._tables.addDeleteListener(this._delete_listener.bind(this));
this.commands = createCommands(this);
this.menuRenderer = new MenuRenderer(this.element);

Expand Down Expand Up @@ -269,12 +273,9 @@ export class PerspectiveWorkspace extends DiscreteSplitPanel {
const isUsed = this.getAllWidgets().some(widget => widget.viewer.getAttribute("table") === name);
if (isUsed) {
console.error(`Cannot remove table: '${name}' because it's still bound to widget(s)`);
} else {
const result = this.tables.delete(name);
if (!result) {
console.warn(`Table: '${name}' does not exist`);
}
return false;
}
return true;
}

update_widget_for_viewer(viewer) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ document.createElement = name => {

const patchUnknownElement = element => {
let config = {};
element.load = jest.fn();
element.save = () => config;
element.restore = value => {
config = {...config, ...value};
Expand Down
72 changes: 72 additions & 0 deletions packages/perspective-workspace/test/js/unit/tables.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
/******************************************************************************
*
* Copyright (c) 2018, the Perspective Authors.
*
* This file is part of the Perspective library, distributed under the terms of
* the Apache License 2.0. The full license can be found in the LICENSE file.
*
*/

import {PerspectiveWorkspace} from "../../../src/js/workspace/workspace";
import perspective from "@finos/perspective";

describe("tables", () => {
test("setting a table calls load on a subscribed viewer", () => {
const table = perspective.table([{a: 1}]);
const viewers = {One: {table: "test", name: "One"}};
const config = {
viewers,
detail: {
main: {
currentIndex: 0,
type: "tab-area",
widgets: ["One"]
}
}
};

const workspace = new PerspectiveWorkspace(document.body);
workspace.restore(config);

const widget = workspace.getAllWidgets()[0];
expect(widget.viewer.load).not.toBeCalled();

workspace.tables.set("test", table);

expect(widget.viewer.load).toBeCalled();
});

test("delete a table without subscribers works", () => {
const table = perspective.table([{a: 1}]);
const workspace = new PerspectiveWorkspace(document.body);

workspace.tables.set("test", table);
expect(workspace.tables.has("test")).toBeTruthy();

expect(workspace.tables.delete("test")).toBeTruthy();
expect(workspace.tables.has("test")).toBeFalsy();
});

test("delete a table with subscribers fails", () => {
const table = perspective.table([{a: 1}]);
const viewers = {One: {table: "test", name: "One"}};
const config = {
viewers,
detail: {
main: {
currentIndex: 0,
type: "tab-area",
widgets: ["One"]
}
}
};

const workspace = new PerspectiveWorkspace(document.body);
workspace.restore(config);
workspace.tables.set("test", table);
expect(workspace.tables.has("test")).toBeTruthy();

expect(workspace.tables.delete("test")).toBeFalsy();
expect(workspace.tables.has("test")).toBeTruthy();
});
});

0 comments on commit e201021

Please sign in to comment.