From 7384f1b3782c09257622f799f42f62c4f64a7891 Mon Sep 17 00:00:00 2001 From: Akos Kitta Date: Thu, 8 Feb 2024 10:21:09 +0100 Subject: [PATCH] fix: debug widget layout updates Use customized `PanelLayout#removeWidget` and `PanelLayout#insertWidget` logic for the layout updates. The customized functions ensure no side effect if adding/removing the widget to/from the layout but it's already present/absent. Unlike the default [`PanelLayout#removeWidget`](https://github.com/phosphorjs/phosphor/blob/9f5e11025b62d2c4a6fb59e2681ae1ed323dcde4/packages/widgets/src/panellayout.ts#L154-L156) behavior, do not try to remove the widget if it's not present (has a negative index). Otherwise, required widgets might be removed based on the default [`ArrayExt#removeAt`](https://github.com/phosphorjs/phosphor/blob/9f5e11025b62d2c4a6fb59e2681ae1ed323dcde4/packages/algorithm/src/array.ts#L1075-L1077) behavior. Closes: arduino/arduino-ide#2354 Signed-off-by: Akos Kitta --- .../src/browser/theia/debug/debug-widget.ts | 21 ++- .../src/browser/theia/dialogs/widgets.ts | 51 ++++++++ .../src/test/browser/widgets.test.ts | 121 ++++++++++++++++++ 3 files changed, 182 insertions(+), 11 deletions(-) create mode 100644 arduino-ide-extension/src/browser/theia/dialogs/widgets.ts create mode 100644 arduino-ide-extension/src/test/browser/widgets.test.ts diff --git a/arduino-ide-extension/src/browser/theia/debug/debug-widget.ts b/arduino-ide-extension/src/browser/theia/debug/debug-widget.ts index cca4d1b06..9f332bbe7 100644 --- a/arduino-ide-extension/src/browser/theia/debug/debug-widget.ts +++ b/arduino-ide-extension/src/browser/theia/debug/debug-widget.ts @@ -1,8 +1,5 @@ -import { - codicon, - PanelLayout, - Widget, -} from '@theia/core/lib/browser/widgets/widget'; +import { codicon } from '@theia/core/lib/browser/widgets/widget'; +import { Widget } from '@theia/core/shared/@phosphor/widgets'; import { inject, injectable, @@ -10,6 +7,10 @@ import { } from '@theia/core/shared/inversify'; import { DebugWidget as TheiaDebugWidget } from '@theia/debug/lib/browser/view/debug-widget'; import { DebugDisabledStatusMessageSource } from '../../contributions/debug'; +import { + removeWidgetIfPresent, + unshiftWidgetIfNotPresent, +} from '../dialogs/widgets'; @injectable() export class DebugWidget extends TheiaDebugWidget { @@ -38,12 +39,10 @@ export class DebugWidget extends TheiaDebugWidget { this.messageNode.textContent = message ?? ''; const enabled = !message; updateVisibility(enabled, this.toolbar, this.sessionWidget); - if (this.layout instanceof PanelLayout) { - if (enabled) { - this.layout.removeWidget(this.statusMessageWidget); - } else { - this.layout.insertWidget(0, this.statusMessageWidget); - } + if (enabled) { + removeWidgetIfPresent(this.layout, this.statusMessageWidget); + } else { + unshiftWidgetIfNotPresent(this.layout, this.statusMessageWidget); } this.title.iconClass = enabled ? codicon('debug-alt') : 'fa fa-ban'; // TODO: find a better icon? }); diff --git a/arduino-ide-extension/src/browser/theia/dialogs/widgets.ts b/arduino-ide-extension/src/browser/theia/dialogs/widgets.ts new file mode 100644 index 000000000..1b64c0a40 --- /dev/null +++ b/arduino-ide-extension/src/browser/theia/dialogs/widgets.ts @@ -0,0 +1,51 @@ +import { + Layout, + PanelLayout, + Widget, +} from '@theia/core/shared/@phosphor/widgets'; + +/** + * + * Removes the widget from the layout if the `layout` is a `PanelLayout` and the widget is present in the layout. + * Otherwise, it's NOOP + * @param layout the layout to remove the widget from. Must be a `PanelLayout`. + * @param toRemove the widget to remove from the layout + */ +export function removeWidgetIfPresent( + layout: Layout | null, + toRemove: Widget +): void { + if (layout instanceof PanelLayout) { + const index = layout.widgets.indexOf(toRemove); + if (index < 0) { + // Unlike the default `PanelLayout#removeWidget` behavior, (https://github.com/phosphorjs/phosphor/blob/9f5e11025b62d2c4a6fb59e2681ae1ed323dcde4/packages/widgets/src/panellayout.ts#L154-L156) + // do not try to remove widget if it's not present (the index is negative). + // Otherwise, required widgets could be removed based on the default ArrayExt behavior (https://github.com/phosphorjs/phosphor/blob/9f5e11025b62d2c4a6fb59e2681ae1ed323dcde4/packages/algorithm/src/array.ts#L1075-L1077) + // See https://github.com/arduino/arduino-ide/issues/2354 for more details. + return; + } + layout.removeWidget(toRemove); + } +} + +/** + * + * Inserts the widget to the `0` index of the layout if the `layout` is a `PanelLayout` and the widget is not yet part of the layout. + * Otherwise, it's NOOP + * @param layout the layout to add the widget to. Must be a `PanelLayout`. + * @param toAdd the widget to add to the layout + */ +export function unshiftWidgetIfNotPresent( + layout: Layout | null, + toAdd: Widget +): void { + if (layout instanceof PanelLayout) { + const index = layout.widgets.indexOf(toAdd); + if (index >= 0) { + // Do not try to add the widget to the layout if it's already present. + // This is the counterpart logic of the `removeWidgetIfPresent` function. + return; + } + layout.insertWidget(0, toAdd); + } +} diff --git a/arduino-ide-extension/src/test/browser/widgets.test.ts b/arduino-ide-extension/src/test/browser/widgets.test.ts new file mode 100644 index 000000000..542cbac97 --- /dev/null +++ b/arduino-ide-extension/src/test/browser/widgets.test.ts @@ -0,0 +1,121 @@ +import { + Disposable, + DisposableCollection, +} from '@theia/core/lib/common/disposable'; +import type { PanelLayout, Widget } from '@theia/core/shared/@phosphor/widgets'; +import { expect } from 'chai'; +import type { + removeWidgetIfPresent, + unshiftWidgetIfNotPresent, +} from '../../browser/theia/dialogs/widgets'; + +describe('widgets', () => { + let toDispose: DisposableCollection; + + beforeEach(() => { + const disableJSDOM = + require('@theia/core/lib/browser/test/jsdom').enableJSDOM(); + toDispose = new DisposableCollection( + Disposable.create(() => disableJSDOM()) + ); + }); + + afterEach(() => toDispose.dispose()); + + describe('removeWidgetIfPresent', () => { + let testMe: typeof removeWidgetIfPresent; + + beforeEach( + () => + (testMe = + require('../../browser/theia/dialogs/widgets').removeWidgetIfPresent) + ); + + it('should remove the widget if present', () => { + const layout = newPanelLayout(); + const widget = newWidget(); + layout.addWidget(widget); + const toRemoveWidget = newWidget(); + layout.addWidget(toRemoveWidget); + + expect(layout.widgets).to.be.deep.equal([widget, toRemoveWidget]); + + testMe(layout, toRemoveWidget); + + expect(layout.widgets).to.be.deep.equal([widget]); + }); + + it('should be noop if the widget is not part of the layout', () => { + const layout = newPanelLayout(); + const widget = newWidget(); + layout.addWidget(widget); + + expect(layout.widgets).to.be.deep.equal([widget]); + + testMe(layout, newWidget()); + + expect(layout.widgets).to.be.deep.equal([widget]); + }); + }); + + describe('unshiftWidgetIfNotPresent', () => { + let testMe: typeof unshiftWidgetIfNotPresent; + + beforeEach( + () => + (testMe = + require('../../browser/theia/dialogs/widgets').unshiftWidgetIfNotPresent) + ); + + it('should unshift the widget if not present', () => { + const layout = newPanelLayout(); + const widget = newWidget(); + layout.addWidget(widget); + + expect(layout.widgets).to.be.deep.equal([widget]); + + const toAdd = newWidget(); + testMe(layout, toAdd); + + expect(layout.widgets).to.be.deep.equal([toAdd, widget]); + }); + + it('should be NOOP if widget is already part of the layout (at 0 index)', () => { + const layout = newPanelLayout(); + const toAdd = newWidget(); + layout.addWidget(toAdd); + const widget = newWidget(); + layout.addWidget(widget); + + expect(layout.widgets).to.be.deep.equal([toAdd, widget]); + + testMe(layout, toAdd); + + expect(layout.widgets).to.be.deep.equal([toAdd, widget]); + }); + + it('should be NOOP if widget is already part of the layout (at >0 index)', () => { + const layout = newPanelLayout(); + const widget = newWidget(); + layout.addWidget(widget); + const toAdd = newWidget(); + layout.addWidget(toAdd); + + expect(layout.widgets).to.be.deep.equal([widget, toAdd]); + + testMe(layout, toAdd); + + expect(layout.widgets).to.be.deep.equal([widget, toAdd]); + }); + }); + + function newWidget(): Widget { + const { Widget } = require('@theia/core/shared/@phosphor/widgets'); + return new Widget(); + } + + function newPanelLayout(): PanelLayout { + const { PanelLayout } = require('@theia/core/shared/@phosphor/widgets'); + return new PanelLayout(); + } +});