From 7515b9b05de00477492c6bd21bb7e40aea177a8c Mon Sep 17 00:00:00 2001 From: mathuo <6710312+mathuo@users.noreply.github.com> Date: Sat, 21 Dec 2024 13:07:18 +0000 Subject: [PATCH 1/5] bug: fixup popout group flows --- .../dockview/dockviewComponent.spec.ts | 152 +++++++++++++++++- .../src/dockview/dockviewComponent.ts | 57 +++++-- .../dockview-core/src/dockview/options.ts | 1 + 3 files changed, 198 insertions(+), 12 deletions(-) diff --git a/packages/dockview-core/src/__tests__/dockview/dockviewComponent.spec.ts b/packages/dockview-core/src/__tests__/dockview/dockviewComponent.spec.ts index c646ca0a5..c6dc7a038 100644 --- a/packages/dockview-core/src/__tests__/dockview/dockviewComponent.spec.ts +++ b/packages/dockview-core/src/__tests__/dockview/dockviewComponent.spec.ts @@ -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'); @@ -4867,6 +4868,155 @@ describe('dockviewComponent', () => { ); }); + test('basic', async () => { + jest.useRealTimers(); + + 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, 1000); + + dockview.fromJSON({ + activeGroup: 'group-1', + grid: { + root: { + type: 'branch', + data: [ + { + type: 'leaf', + data: { + views: ['panel1'], + id: 'group-1', + activeView: 'panel1', + }, + size: 1000, + }, + ], + size: 1000, + }, + height: 1000, + width: 1000, + orientation: Orientation.VERTICAL, + }, + popoutGroups: [ + { + data: { + views: ['panel2'], + id: 'group-2', + activeView: 'panel2', + }, + position: null, + }, + ], + panels: { + panel1: { + id: 'panel1', + contentComponent: 'default', + title: 'panel1', + }, + panel2: { + id: 'panel2', + contentComponent: 'default', + title: 'panel2', + }, + }, + }); + + await new Promise((resolve) => setTimeout(resolve, 0)); + + const panel2 = dockview.api.getPanel('panel2'); + + const windowObject = + panel2?.api.location.type === 'popout' + ? panel2?.api.location.getWindow() + : undefined; + + expect(windowObject).toBeTruthy(); + + windowObject!.close(); + }); + + test('grid -> floating -> popout -> popout closed', 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); + + const panel1 = dockview.addPanel({ + id: 'panel_1', + component: 'default', + }); + + const panel2 = dockview.addPanel({ + id: 'panel_2', + component: 'default', + }); + + const panel3 = dockview.addPanel({ + id: 'panel_3', + component: 'default', + position: { direction: 'right' }, + }); + + expect(panel1.api.location.type).toBe('grid'); + expect(panel2.api.location.type).toBe('grid'); + expect(panel3.api.location.type).toBe('grid'); + + dockview.addFloatingGroup(panel2); + + expect(panel1.api.location.type).toBe('grid'); + expect(panel2.api.location.type).toBe('floating'); + expect(panel3.api.location.type).toBe('grid'); + + await dockview.addPopoutGroup(panel2); + + expect(panel1.api.location.type).toBe('grid'); + expect(panel2.api.location.type).toBe('popout'); + expect(panel3.api.location.type).toBe('grid'); + + const windowObject = + panel2.api.location.type === 'popout' + ? panel2.api.location.getWindow() + : undefined; + expect(windowObject).toBeTruthy(); + + windowObject!.close(); + + expect(panel1.api.location.type).toBe('grid'); + expect(panel2.api.location.type).toBe('floating'); + expect(panel3.api.location.type).toBe('grid'); + }); + test('move popout group of 1 panel inside grid', async () => { const container = document.createElement('div'); @@ -5116,7 +5266,7 @@ describe('dockviewComponent', () => { mockWindow.close(); expect(panel1.group.api.location.type).toBe('grid'); - expect(panel2.group.api.location.type).toBe('grid'); + expect(panel2.group.api.location.type).toBe('floating'); expect(panel3.group.api.location.type).toBe('grid'); dockview.clear(); diff --git a/packages/dockview-core/src/dockview/dockviewComponent.ts b/packages/dockview-core/src/dockview/dockviewComponent.ts index cf7823d9f..9d49df053 100644 --- a/packages/dockview-core/src/dockview/dockviewComponent.ts +++ b/packages/dockview-core/src/dockview/dockviewComponent.ts @@ -706,6 +706,8 @@ export class DockviewComponent _window.window!.innerHeight ); + let floatingBox: AnchoredBox | undefined; + if (!options?.overridePopoutGroup && isGroupAddedToDom) { if (itemToPopout instanceof DockviewPanel) { this.movingLock(() => { @@ -727,7 +729,15 @@ export class DockviewComponent break; case 'floating': case 'popout': + floatingBox = this._floatingGroups + .find( + (value) => + value.group.api.id === + itemToPopout.api.id + ) + ?.overlay.toJSON(); this.removeGroup(referenceGroup); + break; } } @@ -825,20 +835,29 @@ export class DockviewComponent }); } } else if (this.getPanel(group.id)) { - this.doRemoveGroup(group, { - skipDispose: true, - skipActive: true, - skipPopoutReturn: true, - }); - const removedGroup = group; - removedGroup.model.renderContainer = - this.overlayRenderContainer; - removedGroup.model.location = { type: 'grid' }; - returnedGroup = removedGroup; + if (floatingBox) { + this.addFloatingGroup(removedGroup, { + height: floatingBox.height, + width: floatingBox.width, + position: floatingBox, + }); + } else { + this.doRemoveGroup(removedGroup, { + skipDispose: true, + skipActive: true, + skipPopoutReturn: true, + }); - this.doAddGroup(removedGroup, [0]); + removedGroup.model.renderContainer = + this.overlayRenderContainer; + removedGroup.model.location = { type: 'grid' }; + returnedGroup = removedGroup; + this.movingLock(() => { + this.doAddGroup(removedGroup, [0]); + }); + } this.doSetGroupAndPanelActive(removedGroup); } }) @@ -1477,6 +1496,22 @@ export class DockviewComponent ); } + if (options.popout) { + const group = this.createGroup(); + this._onDidAddGroup.fire(group); + + this.addPopoutGroup(group); + + const panel = this.createPanel(options, group); + + group.model.openPanel(panel, { + skipSetActive: options.inactive, + skipSetGroupActive: options.inactive, + }); + + return panel; + } + const initial = { width: options.initialWidth, height: options.initialHeight, diff --git a/packages/dockview-core/src/dockview/options.ts b/packages/dockview-core/src/dockview/options.ts index 3e87923a2..d21418730 100644 --- a/packages/dockview-core/src/dockview/options.ts +++ b/packages/dockview-core/src/dockview/options.ts @@ -252,6 +252,7 @@ export type AddPanelOptions

= { inactive?: boolean; initialWidth?: number; initialHeight?: number; + popout?: boolean; } & Partial & Partial; From 5d868e63ceaf9e1e4051bfd5ddc6cd11340b9534 Mon Sep 17 00:00:00 2001 From: mathuo <6710312+mathuo@users.noreply.github.com> Date: Sat, 21 Dec 2024 13:58:11 +0000 Subject: [PATCH 2/5] feat: additional event sequencing checks --- .../dockview/dockviewComponent.spec.ts | 8 +-- .../src/dockview/dockviewComponent.ts | 9 +++- .../src/dockview/strictEventsSequencing.ts | 54 +++++++++++++++++++ 3 files changed, 65 insertions(+), 6 deletions(-) create mode 100644 packages/dockview-core/src/dockview/strictEventsSequencing.ts diff --git a/packages/dockview-core/src/__tests__/dockview/dockviewComponent.spec.ts b/packages/dockview-core/src/__tests__/dockview/dockviewComponent.spec.ts index c6dc7a038..fd5981b78 100644 --- a/packages/dockview-core/src/__tests__/dockview/dockviewComponent.spec.ts +++ b/packages/dockview-core/src/__tests__/dockview/dockviewComponent.spec.ts @@ -3708,16 +3708,16 @@ describe('dockviewComponent', () => { floatingGroups: [ { data: { - views: ['panelB'], - activeView: 'panelB', + views: ['panelC'], + activeView: 'panelC', id: '3', }, position: { left: 0, top: 0, height: 100, width: 100 }, }, { data: { - views: ['panelC'], - activeView: 'panelC', + views: ['panelD'], + activeView: 'panelD', id: '4', }, position: { left: 0, top: 0, height: 100, width: 100 }, diff --git a/packages/dockview-core/src/dockview/dockviewComponent.ts b/packages/dockview-core/src/dockview/dockviewComponent.ts index 9d49df053..152285f39 100644 --- a/packages/dockview-core/src/dockview/dockviewComponent.ts +++ b/packages/dockview-core/src/dockview/dockviewComponent.ts @@ -73,6 +73,7 @@ import { OverlayRenderContainer, } from '../overlay/overlayRenderContainer'; import { PopoutWindow } from '../popoutWindow'; +import { StrictEventsSequencing } from './strictEventsSequencing'; const DEFAULT_ROOT_OVERLAY_MODEL: DroptargetOverlayModel = { activationSize: { type: 'pixels', value: 10 }, @@ -388,6 +389,10 @@ export class DockviewComponent toggleClass(this.gridview.element, 'dv-dockview', true); toggleClass(this.element, 'dv-debug', !!options.debug); + if (options.debug) { + this.addDisposables(new StrictEventsSequencing(this)); + } + this.addDisposables( this.overlayRenderContainer, this._onWillDragPanel, @@ -1309,6 +1314,7 @@ export class DockviewComponent locked: !!locked, hideHeader: !!hideHeader, }); + this._onDidAddGroup.fire(group); const createdPanels: IDockviewPanel[] = []; @@ -1325,8 +1331,6 @@ export class DockviewComponent createdPanels.push(panel); } - this._onDidAddGroup.fire(group); - for (let i = 0; i < views.length; i++) { const panel = createdPanels[i]; @@ -1413,6 +1417,7 @@ export class DockviewComponent '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/dockview/strictEventsSequencing.ts b/packages/dockview-core/src/dockview/strictEventsSequencing.ts new file mode 100644 index 000000000..60f91e613 --- /dev/null +++ b/packages/dockview-core/src/dockview/strictEventsSequencing.ts @@ -0,0 +1,54 @@ +import { CompositeDisposable } from '../lifecycle'; +import { DockviewComponent } from './dockviewComponent'; + +export class StrictEventsSequencing extends CompositeDisposable { + constructor(private readonly accessor: DockviewComponent) { + super(); + + this.init(); + } + + private init(): void { + const panels = new Set(); + const groups = new Set(); + + this.addDisposables( + this.accessor.onDidAddPanel((panel) => { + if (panels.has(panel.api.id)) { + throw new Error( + `dockview: Invalid event sequence. [onDidAddPanel] called for panel ${panel.api.id} but panel already exists` + ); + } else { + panels.add(panel.api.id); + } + }), + this.accessor.onDidRemovePanel((panel) => { + if (!panels.has(panel.api.id)) { + throw new Error( + `dockview: Invalid event sequence. [onDidRemovePanel] called for panel ${panel.api.id} but panel does not exists` + ); + } else { + panels.delete(panel.api.id); + } + }), + this.accessor.onDidAddGroup((group) => { + if (groups.has(group.api.id)) { + throw new Error( + `dockview: Invalid event sequence. [onDidAddGroup] called for group ${group.api.id} but group already exists` + ); + } else { + groups.add(group.api.id); + } + }), + this.accessor.onDidRemoveGroup((group) => { + if (!groups.has(group.api.id)) { + throw new Error( + `dockview: Invalid event sequence. [onDidRemoveGroup] called for group ${group.api.id} but group does not exists` + ); + } else { + groups.delete(group.api.id); + } + }) + ); + } +} From 0d32d285d2063d2b655d6bf46a5beb484175d9cd Mon Sep 17 00:00:00 2001 From: mathuo <6710312+mathuo@users.noreply.github.com> Date: Sat, 21 Dec 2024 13:59:24 +0000 Subject: [PATCH 3/5] chore: remove testing code --- .../src/dockview/dockviewComponent.ts | 16 ---------------- packages/dockview-core/src/dockview/options.ts | 1 - 2 files changed, 17 deletions(-) diff --git a/packages/dockview-core/src/dockview/dockviewComponent.ts b/packages/dockview-core/src/dockview/dockviewComponent.ts index 152285f39..547c47120 100644 --- a/packages/dockview-core/src/dockview/dockviewComponent.ts +++ b/packages/dockview-core/src/dockview/dockviewComponent.ts @@ -1501,22 +1501,6 @@ export class DockviewComponent ); } - if (options.popout) { - const group = this.createGroup(); - this._onDidAddGroup.fire(group); - - this.addPopoutGroup(group); - - const panel = this.createPanel(options, group); - - group.model.openPanel(panel, { - skipSetActive: options.inactive, - skipSetGroupActive: options.inactive, - }); - - return panel; - } - const initial = { width: options.initialWidth, height: options.initialHeight, diff --git a/packages/dockview-core/src/dockview/options.ts b/packages/dockview-core/src/dockview/options.ts index d21418730..3e87923a2 100644 --- a/packages/dockview-core/src/dockview/options.ts +++ b/packages/dockview-core/src/dockview/options.ts @@ -252,7 +252,6 @@ export type AddPanelOptions

= { inactive?: boolean; initialWidth?: number; initialHeight?: number; - popout?: boolean; } & Partial & Partial; From c216d703544dcd3686164dd3c4abefe8ef4abfbe Mon Sep 17 00:00:00 2001 From: mathuo <6710312+mathuo@users.noreply.github.com> Date: Sat, 21 Dec 2024 14:00:57 +0000 Subject: [PATCH 4/5] chore: comments --- packages/dockview-core/src/dockview/dockviewComponent.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/dockview-core/src/dockview/dockviewComponent.ts b/packages/dockview-core/src/dockview/dockviewComponent.ts index 547c47120..42cbe01c7 100644 --- a/packages/dockview-core/src/dockview/dockviewComponent.ts +++ b/packages/dockview-core/src/dockview/dockviewComponent.ts @@ -741,6 +741,7 @@ export class DockviewComponent itemToPopout.api.id ) ?.overlay.toJSON(); + this.removeGroup(referenceGroup); break; @@ -859,7 +860,9 @@ export class DockviewComponent this.overlayRenderContainer; removedGroup.model.location = { type: 'grid' }; returnedGroup = removedGroup; + this.movingLock(() => { + // suppress group add events since the group already exists this.doAddGroup(removedGroup, [0]); }); } From f749372eb2246a793717b5011fc0a2605ce3a072 Mon Sep 17 00:00:00 2001 From: mathuo <6710312+mathuo@users.noreply.github.com> Date: Sat, 21 Dec 2024 14:21:28 +0000 Subject: [PATCH 5/5] chore: memory leak fixes and tests --- .../dockview/dockviewComponent.spec.ts | 176 +++++++++--------- .../src/dockview/dockviewComponent.ts | 6 + .../src/dockview/dockviewGroupPanelModel.ts | 4 +- .../src/gridview/baseComponentGridview.ts | 2 + 4 files changed, 101 insertions(+), 87 deletions(-) diff --git a/packages/dockview-core/src/__tests__/dockview/dockviewComponent.spec.ts b/packages/dockview-core/src/__tests__/dockview/dockviewComponent.spec.ts index fd5981b78..e1ea35cbd 100644 --- a/packages/dockview-core/src/__tests__/dockview/dockviewComponent.spec.ts +++ b/packages/dockview-core/src/__tests__/dockview/dockviewComponent.spec.ts @@ -21,7 +21,6 @@ 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'); @@ -141,109 +140,114 @@ describe('dockviewComponent', () => { expect(dockview.element.className).toBe('test-b test-c'); }); - // describe('memory leakage', () => { - // beforeEach(() => { - // window.open = () => fromPartial({ - // addEventListener: jest.fn(), - // close: jest.fn(), - // }); - // }); + describe('memory leakage', () => { + beforeEach(() => { + window.open = () => setupMockWindow(); + }); - // test('event leakage', () => { - // Emitter.setLeakageMonitorEnabled(true); + test('event leakage', async () => { + Emitter.setLeakageMonitorEnabled(true); - // dockview = new DockviewComponent({ - // parentElement: container, - // components: { - // default: PanelContentPartTest, - // }, - // }); + dockview = new DockviewComponent(container, { + createComponent(options) { + switch (options.name) { + case 'default': + return new PanelContentPartTest( + options.id, + options.name + ); + default: + throw new Error(`unsupported`); + } + }, + className: 'test-a test-b', + }); - // dockview.layout(500, 1000); + dockview.layout(500, 1000); - // const panel1 = dockview.addPanel({ - // id: 'panel1', - // component: 'default', - // }); + const panel1 = dockview.addPanel({ + id: 'panel1', + component: 'default', + }); - // const panel2 = dockview.addPanel({ - // id: 'panel2', - // component: 'default', - // }); + const panel2 = dockview.addPanel({ + id: 'panel2', + component: 'default', + }); - // dockview.removePanel(panel2); + dockview.removePanel(panel2); - // const panel3 = dockview.addPanel({ - // id: 'panel3', - // component: 'default', - // position: { - // direction: 'right', - // referencePanel: 'panel1', - // }, - // }); + const panel3 = dockview.addPanel({ + id: 'panel3', + component: 'default', + position: { + direction: 'right', + referencePanel: 'panel1', + }, + }); - // const panel4 = dockview.addPanel({ - // id: 'panel4', - // component: 'default', - // position: { - // direction: 'above', - // }, - // }); + const panel4 = dockview.addPanel({ + id: 'panel4', + component: 'default', + position: { + direction: 'above', + }, + }); - // dockview.moveGroupOrPanel( - // panel4.group, - // panel3.group.id, - // panel3.id, - // 'center' - // ); + panel4.api.group.api.moveTo({ + group: panel3.api.group, + position: 'center', + }); - // dockview.addPanel({ - // id: 'panel5', - // component: 'default', - // floating: true, - // }); + dockview.addPanel({ + id: 'panel5', + component: 'default', + floating: true, + }); - // const panel6 = dockview.addPanel({ - // id: 'panel6', - // component: 'default', - // position: { - // referencePanel: 'panel5', - // direction: 'within', - // }, - // }); + const panel6 = dockview.addPanel({ + id: 'panel6', + component: 'default', + position: { + referencePanel: 'panel5', + direction: 'within', + }, + }); - // dockview.addFloatingGroup(panel4.api.group); + dockview.addFloatingGroup(panel4.api.group); - // dockview.addPopoutGroup(panel6); + await dockview.addPopoutGroup(panel2); - // dockview.moveGroupOrPanel( - // panel1.group, - // panel6.group.id, - // panel6.id, - // 'center' - // ); + panel1.api.group.api.moveTo({ + group: panel6.api.group, + position: 'center', + }); - // dockview.moveGroupOrPanel( - // panel4.group, - // panel6.group.id, - // panel6.id, - // 'center' - // ); + panel4.api.group.api.moveTo({ + group: panel6.api.group, + position: 'center', + }); - // dockview.dispose(); + dockview.dispose(); - // if (Emitter.MEMORY_LEAK_WATCHER.size > 0) { - // for (const entry of Array.from( - // Emitter.MEMORY_LEAK_WATCHER.events - // )) { - // console.log('disposal', entry[1]); - // } - // throw new Error('not all listeners disposed'); - // } + if (Emitter.MEMORY_LEAK_WATCHER.size > 0) { + console.warn( + `${Emitter.MEMORY_LEAK_WATCHER.size} undisposed resources` + ); - // Emitter.setLeakageMonitorEnabled(false); - // }); - // }); + for (const entry of Array.from( + Emitter.MEMORY_LEAK_WATCHER.events + )) { + console.log('disposal', entry[1]); + } + throw new Error( + `${Emitter.MEMORY_LEAK_WATCHER.size} undisposed resources` + ); + } + + Emitter.setLeakageMonitorEnabled(false); + }); + }); test('duplicate panel', () => { dockview.layout(500, 1000); diff --git a/packages/dockview-core/src/dockview/dockviewComponent.ts b/packages/dockview-core/src/dockview/dockviewComponent.ts index 42cbe01c7..d0d1c39aa 100644 --- a/packages/dockview-core/src/dockview/dockviewComponent.ts +++ b/packages/dockview-core/src/dockview/dockviewComponent.ts @@ -409,6 +409,7 @@ export class DockviewComponent this._onDidRemoveGroup, this._onDidActiveGroupChange, this._onUnhandledDragOverEvent, + this._onDidMaximizedGroupChange, this.onDidViewVisibilityChangeMicroTaskQueue(() => { this.updateWatermark(); }), @@ -576,6 +577,11 @@ export class DockviewComponent this.updateWatermark(); } + override dispose(): void { + this.clear(); // explicitly clear the layout before cleaning up + super.dispose(); + } + override setVisible(panel: DockviewGroupPanel, visible: boolean): void { switch (panel.api.location.type) { case 'grid': diff --git a/packages/dockview-core/src/dockview/dockviewGroupPanelModel.ts b/packages/dockview-core/src/dockview/dockviewGroupPanelModel.ts index 17af6e573..a34d5ef10 100644 --- a/packages/dockview-core/src/dockview/dockviewGroupPanelModel.ts +++ b/packages/dockview-core/src/dockview/dockviewGroupPanelModel.ts @@ -506,7 +506,9 @@ export class DockviewGroupPanelModel this._onDidAddPanel, this._onDidRemovePanel, this._onDidActivePanelChange, - this._onUnhandledDragOverEvent + this._onUnhandledDragOverEvent, + this._onDidPanelTitleChange, + this._onDidPanelParametersChange ); } diff --git a/packages/dockview-core/src/gridview/baseComponentGridview.ts b/packages/dockview-core/src/gridview/baseComponentGridview.ts index 44e810a50..9d02993f3 100644 --- a/packages/dockview-core/src/gridview/baseComponentGridview.ts +++ b/packages/dockview-core/src/gridview/baseComponentGridview.ts @@ -205,6 +205,8 @@ export abstract class BaseGrid )(() => { this._bufferOnDidLayoutChange.fire(); }), + this._onDidMaximizedChange, + this._onDidViewVisibilityChangeMicroTaskQueue, this._bufferOnDidLayoutChange ); }