Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

Commit ca779e5

Browse files
Akos Kittakittaakos
Akos Kitta
authored andcommitted
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: #2354 Signed-off-by: Akos Kitta <a.kitta@arduino.cc>
1 parent 74c5801 commit ca779e5

File tree

3 files changed

+182
-11
lines changed

3 files changed

+182
-11
lines changed

‎arduino-ide-extension/src/browser/theia/debug/debug-widget.ts‎

Lines changed: 10 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,16 @@
1-
import {
2-
codicon,
3-
PanelLayout,
4-
Widget,
5-
} from '@theia/core/lib/browser/widgets/widget';
1+
import { codicon } from '@theia/core/lib/browser/widgets/widget';
2+
import { Widget } from '@theia/core/shared/@phosphor/widgets';
63
import {
74
inject,
85
injectable,
96
postConstruct,
107
} from '@theia/core/shared/inversify';
118
import { DebugWidget as TheiaDebugWidget } from '@theia/debug/lib/browser/view/debug-widget';
129
import { DebugDisabledStatusMessageSource } from '../../contributions/debug';
10+
import {
11+
removeWidgetIfPresent,
12+
unshiftWidgetIfNotPresent,
13+
} from '../dialogs/widgets';
1314

1415
@injectable()
1516
export class DebugWidget extends TheiaDebugWidget {
@@ -38,12 +39,10 @@ export class DebugWidget extends TheiaDebugWidget {
3839
this.messageNode.textContent = message ?? '';
3940
const enabled = !message;
4041
updateVisibility(enabled, this.toolbar, this.sessionWidget);
41-
if (this.layout instanceof PanelLayout) {
42-
if (enabled) {
43-
this.layout.removeWidget(this.statusMessageWidget);
44-
} else {
45-
this.layout.insertWidget(0, this.statusMessageWidget);
46-
}
42+
if (enabled) {
43+
removeWidgetIfPresent(this.layout, this.statusMessageWidget);
44+
} else {
45+
unshiftWidgetIfNotPresent(this.layout, this.statusMessageWidget);
4746
}
4847
this.title.iconClass = enabled ? codicon('debug-alt') : 'fa fa-ban'; // TODO: find a better icon?
4948
});
Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
import {
2+
Layout,
3+
PanelLayout,
4+
Widget,
5+
} from '@theia/core/shared/@phosphor/widgets';
6+
7+
/**
8+
*
9+
* Removes the widget from the layout if the `layout` is a `PanelLayout` and the widget is present in the layout.
10+
* Otherwise, it's NOOP
11+
* @param layout the layout to remove the widget from. Must be a `PanelLayout`.
12+
* @param toRemove the widget to remove from the layout
13+
*/
14+
export function removeWidgetIfPresent(
15+
layout: Layout | null,
16+
toRemove: Widget
17+
): void {
18+
if (layout instanceof PanelLayout) {
19+
const index = layout.widgets.indexOf(toRemove);
20+
if (index < 0) {
21+
// Unlike the default `PanelLayout#removeWidget` behavior, (https://github.com/phosphorjs/phosphor/blob/9f5e11025b62d2c4a6fb59e2681ae1ed323dcde4/packages/widgets/src/panellayout.ts#L154-L156)
22+
// do not try to remove widget if it's not present (the index is negative).
23+
// 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)
24+
// See https://github.com/arduino/arduino-ide/issues/2354 for more details.
25+
return;
26+
}
27+
layout.removeWidget(toRemove);
28+
}
29+
}
30+
31+
/**
32+
*
33+
* 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.
34+
* Otherwise, it's NOOP
35+
* @param layout the layout to add the widget to. Must be a `PanelLayout`.
36+
* @param toAdd the widget to add to the layout
37+
*/
38+
export function unshiftWidgetIfNotPresent(
39+
layout: Layout | null,
40+
toAdd: Widget
41+
): void {
42+
if (layout instanceof PanelLayout) {
43+
const index = layout.widgets.indexOf(toAdd);
44+
if (index >= 0) {
45+
// Do not try to add the widget to the layout if it's already present.
46+
// This is the counterpart logic of the `removeWidgetIfPresent` function.
47+
return;
48+
}
49+
layout.insertWidget(0, toAdd);
50+
}
51+
}
Lines changed: 121 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,121 @@
1+
import {
2+
Disposable,
3+
DisposableCollection,
4+
} from '@theia/core/lib/common/disposable';
5+
import type { PanelLayout, Widget } from '@theia/core/shared/@phosphor/widgets';
6+
import { expect } from 'chai';
7+
import type {
8+
removeWidgetIfPresent,
9+
unshiftWidgetIfNotPresent,
10+
} from '../../browser/theia/dialogs/widgets';
11+
12+
describe('widgets', () => {
13+
let toDispose: DisposableCollection;
14+
15+
beforeEach(() => {
16+
const disableJSDOM =
17+
require('@theia/core/lib/browser/test/jsdom').enableJSDOM();
18+
toDispose = new DisposableCollection(
19+
Disposable.create(() => disableJSDOM())
20+
);
21+
});
22+
23+
afterEach(() => toDispose.dispose());
24+
25+
describe('removeWidgetIfPresent', () => {
26+
let testMe: typeof removeWidgetIfPresent;
27+
28+
beforeEach(
29+
() =>
30+
(testMe =
31+
require('../../browser/theia/dialogs/widgets').removeWidgetIfPresent)
32+
);
33+
34+
it('should remove the widget if present', () => {
35+
const layout = newPanelLayout();
36+
const widget = newWidget();
37+
layout.addWidget(widget);
38+
const toRemoveWidget = newWidget();
39+
layout.addWidget(toRemoveWidget);
40+
41+
expect(layout.widgets).to.be.deep.equal([widget, toRemoveWidget]);
42+
43+
testMe(layout, toRemoveWidget);
44+
45+
expect(layout.widgets).to.be.deep.equal([widget]);
46+
});
47+
48+
it('should be noop if the widget is not part of the layout', () => {
49+
const layout = newPanelLayout();
50+
const widget = newWidget();
51+
layout.addWidget(widget);
52+
53+
expect(layout.widgets).to.be.deep.equal([widget]);
54+
55+
testMe(layout, newWidget());
56+
57+
expect(layout.widgets).to.be.deep.equal([widget]);
58+
});
59+
});
60+
61+
describe('unshiftWidgetIfNotPresent', () => {
62+
let testMe: typeof unshiftWidgetIfNotPresent;
63+
64+
beforeEach(
65+
() =>
66+
(testMe =
67+
require('../../browser/theia/dialogs/widgets').unshiftWidgetIfNotPresent)
68+
);
69+
70+
it('should unshift the widget if not present', () => {
71+
const layout = newPanelLayout();
72+
const widget = newWidget();
73+
layout.addWidget(widget);
74+
75+
expect(layout.widgets).to.be.deep.equal([widget]);
76+
77+
const toAdd = newWidget();
78+
testMe(layout, toAdd);
79+
80+
expect(layout.widgets).to.be.deep.equal([toAdd, widget]);
81+
});
82+
83+
it('should be NOOP if widget is already part of the layout (at 0 index)', () => {
84+
const layout = newPanelLayout();
85+
const toAdd = newWidget();
86+
layout.addWidget(toAdd);
87+
const widget = newWidget();
88+
layout.addWidget(widget);
89+
90+
expect(layout.widgets).to.be.deep.equal([toAdd, widget]);
91+
92+
testMe(layout, toAdd);
93+
94+
expect(layout.widgets).to.be.deep.equal([toAdd, widget]);
95+
});
96+
97+
it('should be NOOP if widget is already part of the layout (at >0 index)', () => {
98+
const layout = newPanelLayout();
99+
const widget = newWidget();
100+
layout.addWidget(widget);
101+
const toAdd = newWidget();
102+
layout.addWidget(toAdd);
103+
104+
expect(layout.widgets).to.be.deep.equal([widget, toAdd]);
105+
106+
testMe(layout, toAdd);
107+
108+
expect(layout.widgets).to.be.deep.equal([widget, toAdd]);
109+
});
110+
});
111+
112+
function newWidget(): Widget {
113+
const { Widget } = require('@theia/core/shared/@phosphor/widgets');
114+
return new Widget();
115+
}
116+
117+
function newPanelLayout(): PanelLayout {
118+
const { PanelLayout } = require('@theia/core/shared/@phosphor/widgets');
119+
return new PanelLayout();
120+
}
121+
});

0 commit comments

Comments
(0)

AltStyle によって変換されたページ (->オリジナル) /