From 674f0a09e16b3232ae7b0bb3ea2200536e8931b5 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 | 84 +++++++++++++++++++ 3 files changed, 145 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..ce7a289dd --- /dev/null +++ b/arduino-ide-extension/src/test/browser/widgets.test.ts @@ -0,0 +1,84 @@ +import { enableJSDOM } from '@theia/core/lib/browser/test/jsdom'; +const disableJSDOM = enableJSDOM(); + +import { PanelLayout, Widget } from '@theia/core/shared/@phosphor/widgets'; +import { expect } from 'chai'; +import { + removeWidgetIfPresent, + unshiftWidgetIfNotPresent, +} from '../../browser/theia/dialogs/widgets'; + +describe('widgets', () => { + after(() => disableJSDOM()); + + describe('removeWidgetIfPresent', () => { + it('should remove the widget if present', () => { + const layout = new PanelLayout(); + const widget = new Widget(); + layout.addWidget(widget); + const toRemoveWidget = new Widget(); + layout.addWidget(toRemoveWidget); + + expect(layout.widgets).to.be.deep.equal([widget, toRemoveWidget]); + + removeWidgetIfPresent(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 = new PanelLayout(); + const widget = new Widget(); + layout.addWidget(widget); + + expect(layout.widgets).to.be.deep.equal([widget]); + + removeWidgetIfPresent(layout, new Widget()); + + expect(layout.widgets).to.be.deep.equal([widget]); + }); + }); + + describe('unshiftWidgetIfNotPresent', () => { + it('should unshift the widget if not present', () => { + const layout = new PanelLayout(); + const widget = new Widget(); + layout.addWidget(widget); + + expect(layout.widgets).to.be.deep.equal([widget]); + + const toAdd = new Widget(); + unshiftWidgetIfNotPresent(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 = new PanelLayout(); + const toAdd = new Widget(); + layout.addWidget(toAdd); + const widget = new Widget(); + layout.addWidget(widget); + + expect(layout.widgets).to.be.deep.equal([toAdd, widget]); + + unshiftWidgetIfNotPresent(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 = new PanelLayout(); + const widget = new Widget(); + layout.addWidget(widget); + const toAdd = new Widget(); + layout.addWidget(toAdd); + + expect(layout.widgets).to.be.deep.equal([widget, toAdd]); + + unshiftWidgetIfNotPresent(layout, toAdd); + + expect(layout.widgets).to.be.deep.equal([widget, toAdd]); + }); + }); +});