From 0ee10912a2d868cb659dc1b0acb3176447579d52 Mon Sep 17 00:00:00 2001 From: mathuo <6710312+mathuo@users.noreply.github.com> Date: Mon, 2 Oct 2023 20:34:19 +0100 Subject: [PATCH] feat: fromJSON error handling and cleanup --- .../dockview/dockviewComponent.spec.ts | 148 +++++++++- .../gridview/gridviewComponent.spec.ts | 80 ++++++ .../src/dockview/dockviewComponent.ts | 252 +++++++++++++----- .../src/gridview/baseComponentGridview.ts | 2 +- .../src/gridview/gridviewComponent.ts | 110 +++++--- 5 files changed, 477 insertions(+), 115 deletions(-) diff --git a/packages/dockview-core/src/__tests__/dockview/dockviewComponent.spec.ts b/packages/dockview-core/src/__tests__/dockview/dockviewComponent.spec.ts index 1dfac7ed4..295ce3256 100644 --- a/packages/dockview-core/src/__tests__/dockview/dockviewComponent.spec.ts +++ b/packages/dockview-core/src/__tests__/dockview/dockviewComponent.spec.ts @@ -4064,13 +4064,14 @@ describe('dockviewComponent', () => { expect(groupDragEvents.length).toBe(1); }); - test('that loading a corrupt layout throws an error and leaves a clean dockview behind', () => { + test('corrupt layout: bad inline view', () => { const container = document.createElement('div'); const dockview = new DockviewComponent({ parentElement: container, components: { - default: PanelContentPartTest, + panelA: PanelContentPartTest, + panelB: PanelContentPartTest, }, tabComponents: { test_tab_id: PanelTabPartTest, @@ -4080,14 +4081,22 @@ describe('dockviewComponent', () => { dockview.layout(1000, 500); + let el = dockview.element.querySelector('.view-container'); + expect(el).toBeTruthy(); + expect(el!.childNodes.length).toBe(0); + dockview.addPanel({ id: 'panel_1', - component: 'default', + component: 'panelA', }); expect(dockview.groups.length).toBe(1); expect(dockview.panels.length).toBe(1); + el = dockview.element.querySelector('.view-container'); + expect(el).toBeTruthy(); + expect(el!.childNodes.length).toBeGreaterThan(0); + expect(() => { dockview.fromJSON({ grid: { @@ -4122,7 +4131,124 @@ describe('dockviewComponent', () => { panels: { panelA: { id: 'panelA', + contentComponent: 'panelA', + title: 'Panel A', + }, + panelB: { + id: 'panelB', contentComponent: 'somethingBad', + title: 'Panel B', + }, + }, + activeGroup: '1', + }); + }).toThrow( + "Cannot create 'panelB', no component 'somethingBad' provided" + ); + + expect(dockview.groups.length).toBe(0); + expect(dockview.panels.length).toBe(0); + + el = dockview.element.querySelector('.view-container'); + expect(el).toBeTruthy(); + expect(el!.childNodes.length).toBe(0); + }); + + test('corrupt layout: bad floating view', () => { + const container = document.createElement('div'); + + const dockview = new DockviewComponent({ + parentElement: container, + components: { + panelA: PanelContentPartTest, + panelB: PanelContentPartTest, + }, + tabComponents: { + test_tab_id: PanelTabPartTest, + }, + orientation: Orientation.HORIZONTAL, + }); + + dockview.layout(1000, 500); + + let el = dockview.element.querySelector('.view-container'); + expect(el).toBeTruthy(); + expect(el!.childNodes.length).toBe(0); + + dockview.addPanel({ + id: 'panel_1', + component: 'panelA', + }); + + dockview.addPanel({ + id: 'panel_2', + component: 'panelA', + floating: true, + }); + + expect(dockview.groups.length).toBe(2); + expect(dockview.panels.length).toBe(2); + + el = dockview.element.querySelector('.dv-resize-container'); + expect(el).toBeTruthy(); + + el = dockview.element.querySelector('.view-container'); + expect(el).toBeTruthy(); + expect(el!.childNodes.length).toBeGreaterThan(0); + + expect(() => { + dockview.fromJSON({ + grid: { + root: { + type: 'branch', + data: [ + { + type: 'leaf', + data: { + views: ['panelA'], + activeView: 'panelA', + id: '1', + }, + size: 841, + }, + { + type: 'leaf', + data: { + views: ['panelB'], + activeView: 'panelB', + id: '2', + }, + size: 842, + }, + ], + size: 530, + }, + width: 1683, + height: 530, + orientation: Orientation.HORIZONTAL, + }, + floatingGroups: [ + { + data: { + views: ['panelB'], + activeView: 'panelB', + id: '3', + }, + position: { left: 0, top: 0, height: 100, width: 100 }, + }, + { + data: { + views: ['panelC'], + activeView: 'panelC', + id: '4', + }, + position: { left: 0, top: 0, height: 100, width: 100 }, + }, + ], + panels: { + panelA: { + id: 'panelA', + contentComponent: 'panelA', title: 'Panel A', }, panelB: { @@ -4130,14 +4256,24 @@ describe('dockviewComponent', () => { contentComponent: 'panelB', title: 'Panel B', }, + panelC: { + id: 'panelC', + contentComponent: 'panelC', + title: 'Panel C', + }, }, activeGroup: '1', }); - }).toThrow( - "Cannot create 'panelA', no component 'somethingBad' provided" - ); + }).toThrow("Cannot create 'panelC', no component 'panelC' provided"); expect(dockview.groups.length).toBe(0); expect(dockview.panels.length).toBe(0); + + el = dockview.element.querySelector('.dv-resize-container'); + expect(el).toBeFalsy(); + + el = dockview.element.querySelector('.view-container'); + expect(el).toBeTruthy(); + expect(el!.childNodes.length).toBe(0); }); }); diff --git a/packages/dockview-core/src/__tests__/gridview/gridviewComponent.spec.ts b/packages/dockview-core/src/__tests__/gridview/gridviewComponent.spec.ts index 99e237a6f..6fad26d2f 100644 --- a/packages/dockview-core/src/__tests__/gridview/gridviewComponent.spec.ts +++ b/packages/dockview-core/src/__tests__/gridview/gridviewComponent.spec.ts @@ -2597,4 +2597,84 @@ describe('gridview', () => { activePanel: 'panel_1', }); }); + + test('that loading a corrupt layout throws an error and leaves a clean gridview behind', () => { + const gridview = new GridviewComponent({ + parentElement: container, + proportionalLayout: true, + orientation: Orientation.HORIZONTAL, + components: { default: TestGridview }, + }); + + let el = gridview.element.querySelector('.view-container'); + expect(el).toBeTruthy(); + expect(el!.childNodes.length).toBe(0); + + expect(() => { + gridview.fromJSON({ + grid: { + height: 400, + width: 800, + orientation: Orientation.HORIZONTAL, + root: { + type: 'branch', + size: 400, + data: [ + { + type: 'leaf', + size: 200, + data: { + id: 'panel_1', + component: 'default', + snap: false, + }, + }, + { + type: 'branch', + size: 400, + data: [ + { + type: 'leaf', + size: 250, + data: { + id: 'panel_1', + component: 'default', + snap: false, + }, + }, + { + type: 'leaf', + size: 150, + data: { + id: 'panel_1', + component: 'somethingBad', + snap: false, + }, + }, + ], + }, + { + type: 'leaf', + size: 200, + data: { + id: 'panel_1', + component: 'default', + snap: false, + }, + }, + ], + }, + }, + activePanel: 'panel_1', + }); + }).toThrow( + "Cannot create 'panel_1', no component 'somethingBad' provided" + ); + + expect(gridview.groups.length).toBe(0); + + el = gridview.element.querySelector('.view-container'); + expect(el).toBeTruthy(); + expect(el!.childNodes.length).toBe(0); + }); }); diff --git a/packages/dockview-core/src/dockview/dockviewComponent.ts b/packages/dockview-core/src/dockview/dockviewComponent.ts index 53d96d250..074b22840 100644 --- a/packages/dockview-core/src/dockview/dockviewComponent.ts +++ b/packages/dockview-core/src/dockview/dockviewComponent.ts @@ -55,6 +55,7 @@ import { GroupDragEvent, TabDragEvent, } from './components/titlebar/tabsContainer'; +import { DockviewGroupPanelApi } from '../api/dockviewGroupPanelApi'; const DEFAULT_FLOATING_GROUP_OVERFLOW_SIZE = 100; @@ -80,6 +81,92 @@ export interface SerializedDockview { floatingGroups?: SerializedFloatingGroup[]; } +function typeValidate3(data: GroupPanelViewState, path: string): void { + if (typeof data.id !== 'string') { + throw new Error(`${path}.id must be a string`); + } + + if ( + typeof data.activeView !== 'string' || + typeof data.activeView !== 'undefined' + ) { + throw new Error(`${path}.activeView must be a string of undefined`); + } +} + +function typeValidate2( + data: SerializedGridObject, + path: string +): void { + if (typeof data.size !== 'number' && typeof data.size !== 'undefined') { + throw new Error(`${path}.size must be a number or undefined`); + } + + if ( + typeof data.visible !== 'boolean' && + typeof data.visible !== 'undefined' + ) { + throw new Error(`${path}.visible must be a boolean or undefined`); + } + + if (data.type === 'leaf') { + if ( + typeof data.data !== 'object' || + data.data === null || + Array.isArray(data.data) + ) { + throw new Error('object must be a non-null object'); + } + + typeValidate3(data.data, `${path}.data`); + } else if (data.type === 'branch') { + if (!Array.isArray(data.data)) { + throw new Error(`${path}.data must be an array`); + } + } else { + throw new Error(`${path}.type must be onew of {'branch', 'leaf'}`); + } +} + +function typeValidate(data: SerializedDockview): void { + if (typeof data !== 'object' || data === null) { + throw new Error('object must be a non-null object'); + } + + const { grid, panels, activeGroup, floatingGroups } = data; + + if (typeof grid !== 'object' || grid === null) { + throw new Error("'.grid' must be a non-null object"); + } + + if (typeof grid.height !== 'number') { + throw new Error("'.grid.height' must be a number"); + } + + if (typeof grid.width !== 'number') { + throw new Error("'.grid.width' must be a number"); + } + + if (typeof grid.root !== 'object' || grid.root === null) { + throw new Error("'.grid.root' must be a non-null object"); + } + + if (grid.root.type !== 'branch') { + throw new Error(".grid.root.type must be of type 'branch'"); + } + + if ( + grid.orientation !== Orientation.HORIZONTAL && + grid.orientation !== Orientation.VERTICAL + ) { + throw new Error( + `'.grid.width' must be one of {${Orientation.HORIZONTAL}, ${Orientation.VERTICAL}}` + ); + } + + typeValidate2(grid.root, '.grid.root'); +} + export type DockviewComponentUpdateOptions = Pick< DockviewComponentOptions, | 'orientation' @@ -636,39 +723,55 @@ export class DockviewComponent fromJSON(data: SerializedDockview): void { this.clear(); + if (typeof data !== 'object' || data === null) { + throw new Error('serialized layout must be a non-null object'); + } + const { grid, panels, activeGroup } = data; if (grid.root.type !== 'branch' || !Array.isArray(grid.root.data)) { throw new Error('root must be of type branch'); } - // take note of the existing dimensions - const width = this.width; - const height = this.height; + try { + // take note of the existing dimensions + const width = this.width; + const height = this.height; - const createGroupFromSerializedState = (data: GroupPanelViewState) => { - const { id, locked, hideHeader, views, activeView } = data; + const createGroupFromSerializedState = ( + data: GroupPanelViewState + ) => { + const { id, locked, hideHeader, views, activeView } = data; - if (typeof id !== 'string') { - throw new Error('group id must be of type string'); - } + if (typeof id !== 'string') { + throw new Error('group id must be of type string'); + } - let group: DockviewGroupPanel | undefined; - - try { - group = this.createGroup({ + const group = this.createGroup({ id, locked: !!locked, hideHeader: !!hideHeader, }); - this._onDidAddGroup.fire(group); + const createdPanels: IDockviewPanel[] = []; for (const child of views) { + /** + * Run the deserializer step seperately since this may fail to due corrupted external state. + * In running this section first we avoid firing lots of 'add' events in the event of a failure + * due to a corruption of input data. + */ const panel = this._deserializer.fromJSON( panels[child], group ); + createdPanels.push(panel); + } + + this._onDidAddGroup.fire(group); + + for (let i = 0; i < views.length; i++) { + const panel = createdPanels[i]; const isActive = typeof activeView === 'string' && @@ -690,61 +793,82 @@ export class DockviewComponent } return group; - } catch (err) { - /** - * This is an odd case... we have failed to deserialize a view but we have already created a group, - * but we havn't registered that group with the gridview. - * We cannot use the removeGroup method because the group has only been partially added, we must - * manually dipose() of the view and remove it from being stored in the map. - */ - if (group) { - group.dispose(); - this._groups.delete(group.id); - } + }; - /** - * re-throw the error becasue we don't actually want to catch it, we just - * needed to do some clean-up before continuing. - */ - throw err; - } - }; - - this.gridview.deserialize(grid, { - fromJSON: (node: ISerializedLeafNode) => { - return createGroupFromSerializedState(node.data); - }, - }); - - this.layout(width, height, true); - - const serializedFloatingGroups = data.floatingGroups ?? []; - - for (const serializedFloatingGroup of serializedFloatingGroups) { - const { data, position } = serializedFloatingGroup; - const group = createGroupFromSerializedState(data); - - this.addFloatingGroup( - group, - { - x: position.left, - y: position.top, - height: position.height, - width: position.width, + this.gridview.deserialize(grid, { + fromJSON: (node: ISerializedLeafNode) => { + return createGroupFromSerializedState(node.data); }, - { skipRemoveGroup: true, inDragMode: false } - ); - } + }); - for (const floatingGroup of this.floatingGroups) { - floatingGroup.overlay.setBounds(); - } + this.layout(width, height, true); - if (typeof activeGroup === 'string') { - const panel = this.getPanel(activeGroup); - if (panel) { - this.doSetGroupActive(panel); + const serializedFloatingGroups = data.floatingGroups ?? []; + + for (const serializedFloatingGroup of serializedFloatingGroups) { + const { data, position } = serializedFloatingGroup; + + const group = createGroupFromSerializedState(data); + + this.addFloatingGroup( + group, + { + x: position.left, + y: position.top, + height: position.height, + width: position.width, + }, + { skipRemoveGroup: true, inDragMode: false } + ); } + + for (const floatingGroup of this.floatingGroups) { + floatingGroup.overlay.setBounds(); + } + + if (typeof activeGroup === 'string') { + const panel = this.getPanel(activeGroup); + if (panel) { + this.doSetGroupActive(panel); + } + } + } catch (err) { + /** + * Takes all the successfully created groups and remove all of their panels. + */ + for (const group of this.groups) { + for (const panel of group.panels) { + this.removePanel(panel, { + removeEmptyGroup: false, + skipDispose: false, + }); + } + } + + /** + * To remove a group we cannot call this.removeGroup(...) since this makes assumptions about + * the underlying HTMLElement existing in the Gridview. + */ + for (const group of this.groups) { + group.dispose(); + this._groups.delete(group.id); + this._onDidRemoveGroup.fire(group); + } + + // iterate over a reassigned array since original array will be modified + for (const floatingGroup of [...this.floatingGroups]) { + floatingGroup.dispose(); + } + + // fires clean-up events and clears the underlying HTML gridview. + this.clear(); + + /** + * even though we have cleaned-up we still want to inform the caller of their error + * and we'll do this through re-throwing the original error since afterall you would + * expect trying to load a corrupted layout to result in an error and not silently fail... + */ + throw err; } this._onDidLayoutFromJSON.fire(); @@ -1051,11 +1175,11 @@ export class DockviewComponent const floatingGroup = this.floatingGroups.find( (_) => _.group === group ); - if (floatingGroup) { if (!options?.skipDispose) { floatingGroup.group.dispose(); this._groups.delete(group.id); + // TODO: fire group removed event? } floatingGroup.dispose(); diff --git a/packages/dockview-core/src/gridview/baseComponentGridview.ts b/packages/dockview-core/src/gridview/baseComponentGridview.ts index 93c6861eb..f82db4870 100644 --- a/packages/dockview-core/src/gridview/baseComponentGridview.ts +++ b/packages/dockview-core/src/gridview/baseComponentGridview.ts @@ -78,7 +78,7 @@ export abstract class BaseGrid private _onDidLayoutChange = new Emitter(); readonly onDidLayoutChange = this._onDidLayoutChange.event; - private readonly _onDidRemoveGroup = new Emitter(); + protected readonly _onDidRemoveGroup = new Emitter(); readonly onDidRemoveGroup: Event = this._onDidRemoveGroup.event; protected readonly _onDidAddGroup = new Emitter(); diff --git a/packages/dockview-core/src/gridview/gridviewComponent.ts b/packages/dockview-core/src/gridview/gridviewComponent.ts index 91b348fdc..0505cdc38 100644 --- a/packages/dockview-core/src/gridview/gridviewComponent.ts +++ b/packages/dockview-core/src/gridview/gridviewComponent.ts @@ -177,60 +177,82 @@ export class GridviewComponent const { grid, activePanel } = serializedGridview; - const queue: Function[] = []; + try { + const queue: Function[] = []; - // take note of the existing dimensions - const width = this.width; - const height = this.height; + // take note of the existing dimensions + const width = this.width; + const height = this.height; - this.gridview.deserialize(grid, { - fromJSON: (node) => { - const { data } = node; - const view = createComponent( - data.id, - data.component, - this.options.components || {}, - this.options.frameworkComponents || {}, - this.options.frameworkComponentFactory - ? { - createComponent: - this.options.frameworkComponentFactory - .createComponent, - } - : undefined - ); + this.gridview.deserialize(grid, { + fromJSON: (node) => { + const { data } = node; + const view = createComponent( + data.id, + data.component, + this.options.components || {}, + this.options.frameworkComponents || {}, + this.options.frameworkComponentFactory + ? { + createComponent: + this.options.frameworkComponentFactory + .createComponent, + } + : undefined + ); - queue.push(() => - view.init({ - params: data.params, - minimumWidth: data.minimumWidth, - maximumWidth: data.maximumWidth, - minimumHeight: data.minimumHeight, - maximumHeight: data.maximumHeight, - priority: data.priority, - snap: !!data.snap, - accessor: this, - isVisible: node.visible, - }) - ); + queue.push(() => + view.init({ + params: data.params, + minimumWidth: data.minimumWidth, + maximumWidth: data.maximumWidth, + minimumHeight: data.minimumHeight, + maximumHeight: data.maximumHeight, + priority: data.priority, + snap: !!data.snap, + accessor: this, + isVisible: node.visible, + }) + ); - this._onDidAddGroup.fire(view); + this._onDidAddGroup.fire(view); - this.registerPanel(view); + this.registerPanel(view); - return view; - }, - }); + return view; + }, + }); - this.layout(width, height, true); + this.layout(width, height, true); - queue.forEach((f) => f()); + queue.forEach((f) => f()); - if (typeof activePanel === 'string') { - const panel = this.getPanel(activePanel); - if (panel) { - this.doSetGroupActive(panel); + if (typeof activePanel === 'string') { + const panel = this.getPanel(activePanel); + if (panel) { + this.doSetGroupActive(panel); + } } + } catch (err) { + /** + * To remove a group we cannot call this.removeGroup(...) since this makes assumptions about + * the underlying HTMLElement existing in the Gridview. + */ + for (const group of this.groups) { + group.dispose(); + this._groups.delete(group.id); + this._onDidRemoveGroup.fire(group); + } + + // fires clean-up events and clears the underlying HTML gridview. + this.clear(); + + /** + * even though we have cleaned-up we still want to inform the caller of their error + * and we'll do this through re-throwing the original error since afterall you would + * expect trying to load a corrupted layout to result in an error and not silently fail... + */ + throw err; } this._onDidLayoutfromJSON.fire();