From ff5fed756c854ca8e77372433c3009e34933c071 Mon Sep 17 00:00:00 2001 From: mathuo <6710312+mathuo@users.noreply.github.com> Date: Thu, 31 Oct 2024 19:31:11 +0000 Subject: [PATCH] bug: duplicate .close() call --- .../src/__tests__/__mocks__/mockWindow.ts | 10 ++- .../dockview/dockviewComponent.spec.ts | 86 ++++++++++++++++++- .../src/dockview/dockviewComponent.ts | 77 ++++++++++------- packages/dockview-core/src/popoutWindow.ts | 1 - .../demo-dockview/src/gridActions.tsx | 11 ++- 5 files changed, 148 insertions(+), 37 deletions(-) diff --git a/packages/dockview-core/src/__tests__/__mocks__/mockWindow.ts b/packages/dockview-core/src/__tests__/__mocks__/mockWindow.ts index cc5f5ed0d..dbf85dc28 100644 --- a/packages/dockview-core/src/__tests__/__mocks__/mockWindow.ts +++ b/packages/dockview-core/src/__tests__/__mocks__/mockWindow.ts @@ -1,4 +1,4 @@ -import { fromPartial } from "@total-typescript/shoehorn"; +import { fromPartial } from '@total-typescript/shoehorn'; export function setupMockWindow() { const listeners: Record void)[]> = {}; @@ -16,6 +16,14 @@ export function setupMockWindow() { listener(); } }, + removeEventListener: (type: string, listener: () => void) => { + if (listeners[type]) { + const index = listeners[type].indexOf(listener); + if (index > -1) { + listeners[type].splice(index, 1); + } + } + }, dispatchEvent: (event: Event) => { const items = listeners[event.type]; if (!items) { diff --git a/packages/dockview-core/src/__tests__/dockview/dockviewComponent.spec.ts b/packages/dockview-core/src/__tests__/dockview/dockviewComponent.spec.ts index d544001fa..04816842d 100644 --- a/packages/dockview-core/src/__tests__/dockview/dockviewComponent.spec.ts +++ b/packages/dockview-core/src/__tests__/dockview/dockviewComponent.spec.ts @@ -8,7 +8,7 @@ import { PanelUpdateEvent } from '../../panel/types'; import { Orientation } from '../../splitview/splitview'; import { CompositeDisposable } from '../../lifecycle'; import { Emitter } from '../../events'; -import { IDockviewPanel } from '../../dockview/dockviewPanel'; +import { DockviewPanel, IDockviewPanel } from '../../dockview/dockviewPanel'; import { DockviewGroupPanel } from '../../dockview/dockviewGroupPanel'; import { fireEvent, queryByTestId } from '@testing-library/dom'; import { getPanelData } from '../../dnd/dataTransfer'; @@ -21,6 +21,7 @@ import { DockviewApi } from '../../api/component.api'; import { DockviewDndOverlayEvent } from '../../dockview/options'; import { SizeEvent } from '../../api/gridviewPanelApi'; import { setupMockWindow } from '../__mocks__/mockWindow'; +import { exhaustMicrotaskQueue } from '../__test_utils__/utils'; class PanelContentPartTest implements IContentRenderer { element: HTMLElement = document.createElement('div'); @@ -4886,6 +4887,89 @@ describe('dockviewComponent', () => { ]); }); + test('popout / floating layouts', async () => { + const container = document.createElement('div'); + + window.open = () => setupMockWindow(); + + const dockview = new DockviewComponent(container, { + createComponent(options) { + switch (options.name) { + case 'default': + return new PanelContentPartTest( + options.id, + options.name + ); + default: + throw new Error(`unsupported`); + } + }, + }); + + dockview.layout(1000, 500); + + let panel1 = dockview.addPanel({ + id: 'panel_1', + component: 'default', + }); + + let panel2 = dockview.addPanel({ + id: 'panel_2', + component: 'default', + }); + + let panel3 = dockview.addPanel({ + id: 'panel_3', + component: 'default', + }); + + let panel4 = dockview.addPanel({ + id: 'panel_4', + component: 'default', + }); + + expect(panel1.api.location.type).toBe('grid'); + expect(panel2.api.location.type).toBe('grid'); + expect(panel3.api.location.type).toBe('grid'); + expect(panel4.api.location.type).toBe('grid'); + + dockview.addFloatingGroup(panel2); + dockview.addFloatingGroup(panel3); + + expect(panel1.api.location.type).toBe('grid'); + expect(panel2.api.location.type).toBe('floating'); + expect(panel3.api.location.type).toBe('floating'); + expect(panel4.api.location.type).toBe('grid'); + + await dockview.addPopoutGroup(panel2); + await dockview.addPopoutGroup(panel4); + + expect(panel1.api.location.type).toBe('grid'); + expect(panel2.api.location.type).toBe('popout'); + expect(panel3.api.location.type).toBe('floating'); + expect(panel4.api.location.type).toBe('popout'); + + const state = dockview.toJSON(); + dockview.fromJSON(state); + + /** + * exhaust task queue since popout group completion is async but not awaited in `fromJSON(...)` + */ + await new Promise((resolve) => setTimeout(resolve, 0)); + + expect(dockview.panels.length).toBe(4); + + panel1 = dockview.api.getPanel('panel_1') as DockviewPanel; + panel2 = dockview.api.getPanel('panel_2') as DockviewPanel; + panel3 = dockview.api.getPanel('panel_3') as DockviewPanel; + panel4 = dockview.api.getPanel('panel_4') as DockviewPanel; + + expect(panel1.api.location.type).toBe('grid'); + expect(panel2.api.location.type).toBe('popout'); + expect(panel3.api.location.type).toBe('floating'); + expect(panel4.api.location.type).toBe('popout'); + }); + test('remove all panels from popout group', async () => { const container = document.createElement('div'); diff --git a/packages/dockview-core/src/dockview/dockviewComponent.ts b/packages/dockview-core/src/dockview/dockviewComponent.ts index a7b71827d..dd5baaf22 100644 --- a/packages/dockview-core/src/dockview/dockviewComponent.ts +++ b/packages/dockview-core/src/dockview/dockviewComponent.ts @@ -541,7 +541,6 @@ export class DockviewComponent addPopoutGroup( itemToPopout: DockviewPanel | DockviewGroupPanel, options?: { - skipRemoveGroup?: boolean; position?: Box; popoutUrl?: string; onDidOpen?: (event: { id: string; window: Window }) => void; @@ -627,41 +626,50 @@ export class DockviewComponent const referenceLocation = itemToPopout.api.location.type; - const group = - options?.overridePopoutGroup ?? - this.createGroup({ id: groupId }); + /** + * The group that is being added doesn't already exist within the DOM, the most likely occurance + * of this case is when being called from the `fromJSON(...)` method + */ + const isGroupAddedToDom = referenceGroup.element.parentElement !== null; + + const group = !isGroupAddedToDom + ? referenceGroup + : options?.overridePopoutGroup ?? + this.createGroup({ id: groupId }); group.model.renderContainer = overlayRenderContainer; group.layout( _window.window!.innerWidth, _window.window!.innerHeight ); - if (!options?.overridePopoutGroup) { + if (!this._groups.has(group.api.id)) { this._onDidAddGroup.fire(group); } - if (itemToPopout instanceof DockviewPanel) { - this.movingLock(() => { - const panel = - referenceGroup.model.removePanel(itemToPopout); - group.model.openPanel(panel); - }); - } else { - this.movingLock(() => - moveGroupWithoutDestroying({ - from: referenceGroup, - to: group, - }) - ); + if (!options?.overridePopoutGroup && isGroupAddedToDom) { + if (itemToPopout instanceof DockviewPanel) { + this.movingLock(() => { + const panel = + referenceGroup.model.removePanel(itemToPopout); + group.model.openPanel(panel); + }); + } else { + this.movingLock(() => + moveGroupWithoutDestroying({ + from: referenceGroup, + to: group, + }) + ); - switch (referenceLocation) { - case 'grid': - referenceGroup.api.setVisible(false); - break; - case 'floating': - case 'popout': - this.removeGroup(referenceGroup); - break; + switch (referenceLocation) { + case 'grid': + referenceGroup.api.setVisible(false); + break; + case 'floating': + case 'popout': + this.removeGroup(referenceGroup); + break; + } } } @@ -676,7 +684,7 @@ export class DockviewComponent getWindow: () => _window.window!, }; - if (itemToPopout.api.location.type === 'grid') { + if (isGroupAddedToDom && itemToPopout.api.location.type === 'grid') { itemToPopout.api.setVisible(false); } @@ -698,8 +706,12 @@ export class DockviewComponent const value = { window: _window, popoutGroup: group, - referenceGroup: this.getPanel(referenceGroup.id) - ? referenceGroup.id + referenceGroup: !isGroupAddedToDom + ? undefined + : referenceGroup + ? this.getPanel(referenceGroup.id) + ? referenceGroup.id + : undefined : undefined, disposable: { dispose: () => { @@ -727,7 +739,7 @@ export class DockviewComponent ), overlayRenderContainer, Disposable.from(() => { - if (this.getPanel(referenceGroup.id)) { + if (isGroupAddedToDom && this.getPanel(referenceGroup.id)) { this.movingLock(() => moveGroupWithoutDestroying({ from: group, @@ -1279,7 +1291,6 @@ export class DockviewComponent ? this.getPanel(gridReferenceGroup) : undefined) ?? group, { - skipRemoveGroup: true, position: position ?? undefined, overridePopoutGroup: gridReferenceGroup ? group @@ -1299,6 +1310,10 @@ export class DockviewComponent } } } catch (err) { + console.error( + 'dockview: failed to deserialize layout. Reverting changes', + err + ); /** * Takes all the successfully created groups and remove all of their panels. */ diff --git a/packages/dockview-core/src/popoutWindow.ts b/packages/dockview-core/src/popoutWindow.ts index 9e4926f09..fd52afbec 100644 --- a/packages/dockview-core/src/popoutWindow.ts +++ b/packages/dockview-core/src/popoutWindow.ts @@ -59,7 +59,6 @@ export class PopoutWindow extends CompositeDisposable { }); this._window.disposable.dispose(); - this._window.value.close(); this._window = null; this._onDidClose.fire(); diff --git a/packages/docs/sandboxes/react/dockview/demo-dockview/src/gridActions.tsx b/packages/docs/sandboxes/react/dockview/demo-dockview/src/gridActions.tsx index 14e0b9aec..8feed27c9 100644 --- a/packages/docs/sandboxes/react/dockview/demo-dockview/src/gridActions.tsx +++ b/packages/docs/sandboxes/react/dockview/demo-dockview/src/gridActions.tsx @@ -103,7 +103,8 @@ export const GridActions = (props: { if (state) { try { props.api?.fromJSON(JSON.parse(state)); - } catch { + } catch (err) { + console.error('failed to load state', err); localStorage.removeItem('dv-demo-state'); } } @@ -121,8 +122,12 @@ export const GridActions = (props: { const onReset = () => { if (props.api) { - props.api.clear(); - defaultConfig(props.api); + try { + props.api.clear(); + defaultConfig(props.api); + } catch (err) { + localStorage.removeItem('dv-demo-state'); + } } };