From a43945dd5d8869a740d5b9da8cf2393485a7dc3b Mon Sep 17 00:00:00 2001 From: Nicolas Brichet Date: Wed, 5 Jul 2023 09:35:56 +0200 Subject: [PATCH 1/7] Add a focusTrackable attribute to Widget, to disable the focusTracker --- packages/widgets/src/focustracker.ts | 4 ++-- packages/widgets/src/tabbar.ts | 1 + packages/widgets/src/widget.ts | 14 ++++++++++++++ 3 files changed, 17 insertions(+), 2 deletions(-) diff --git a/packages/widgets/src/focustracker.ts b/packages/widgets/src/focustracker.ts index c5241833c..5eea8fcae 100644 --- a/packages/widgets/src/focustracker.ts +++ b/packages/widgets/src/focustracker.ts @@ -159,8 +159,8 @@ export class FocusTracker implements IDisposable { * If the widget is already tracked, this is a no-op. */ add(widget: T): void { - // Do nothing if the widget is already tracked. - if (this._numbers.has(widget)) { + // Do nothing if the widget is already tracked or is not trackable. + if (this._numbers.has(widget) || !widget.focusTrackable) { return; } diff --git a/packages/widgets/src/tabbar.ts b/packages/widgets/src/tabbar.ts index 3580e7157..5087ff986 100644 --- a/packages/widgets/src/tabbar.ts +++ b/packages/widgets/src/tabbar.ts @@ -62,6 +62,7 @@ export class TabBar extends Widget { this.orientation = options.orientation || 'horizontal'; this.removeBehavior = options.removeBehavior || 'select-tab-after'; this.renderer = options.renderer || TabBar.defaultRenderer; + this.focusTrackable = false; } /** diff --git a/packages/widgets/src/widget.ts b/packages/widgets/src/widget.ts index 53bf771fe..1c8239946 100644 --- a/packages/widgets/src/widget.ts +++ b/packages/widgets/src/widget.ts @@ -270,6 +270,19 @@ export class Widget implements IMessageHandler, IObservableDisposable { value!.parent = this; } + /** + * The 'focusTrackable' attribute allows the FocusTracker to track the widget focus. + * Its default value is true. + * + * If false, the FocusTracker will never set this widget as current or active. + */ + get focusTrackable(): boolean { + return this._focusTrackable; + } + set focusTrackable(value: boolean) { + this._focusTrackable = value; + } + /** * Create an iterator over the widget's children. * @@ -772,6 +785,7 @@ export class Widget implements IMessageHandler, IObservableDisposable { private _parent: Widget | null = null; private _disposed = new Signal(this); private _hiddenMode: Widget.HiddenMode = Widget.HiddenMode.Display; + private _focusTrackable = true; } /** From 9324ea610eeb333a5d5532e957da9849de469316 Mon Sep 17 00:00:00 2001 From: Nicolas Brichet Date: Wed, 5 Jul 2023 12:11:14 +0200 Subject: [PATCH 2/7] Add tests on trackable attribute --- .../widgets/tests/src/focustracker.spec.ts | 95 +++++++++++++++++++ 1 file changed, 95 insertions(+) diff --git a/packages/widgets/tests/src/focustracker.spec.ts b/packages/widgets/tests/src/focustracker.spec.ts index 702be459a..69f0524c5 100644 --- a/packages/widgets/tests/src/focustracker.spec.ts +++ b/packages/widgets/tests/src/focustracker.spec.ts @@ -109,6 +109,22 @@ describe('@lumino/widgets', () => { focusWidget(widget); expect(emitArgs).to.equal(null); }); + + it('should not be emitted when the focus widget is not tracked', () => { + let tracker = createTracker(); + let widget0 = createWidget(); + let widget1 = createWidget(); + widget1.focusTrackable = false; + tracker.add(widget0); + tracker.add(widget1); + focusWidget(widget0); + let emitArgs: FocusTracker.IChangedArgs | null = null; + tracker.currentChanged.connect((sender, args) => { + emitArgs = args; + }); + focusWidget(widget1); + expect(emitArgs).to.equal(null); + }); }); describe('#activeChanged', () => { @@ -143,6 +159,24 @@ describe('@lumino/widgets', () => { expect(emitArgs!.oldValue).to.equal(widget); expect(emitArgs!.newValue).to.equal(null); }); + + it('should be emitted even if the focus widget is not tracked', () => { + let tracker = createTracker(); + let widget0 = createWidget(); + let widget1 = createWidget(); + widget1.focusTrackable = false; + tracker.add(widget0); + tracker.add(widget1); + focusWidget(widget0); + let emitArgs: FocusTracker.IChangedArgs | null = null; + tracker.activeChanged.connect((sender, args) => { + emitArgs = args; + }); + focusWidget(widget1); + expect(emitArgs).to.not.equal(null); + expect(emitArgs!.oldValue).to.equal(widget0); + expect(emitArgs!.newValue).to.equal(null); + }); }); describe('#isDisposed', () => { @@ -185,6 +219,19 @@ describe('@lumino/widgets', () => { expect(tracker.currentWidget).to.equal(widget1); }); + it('should not be set to the non-trackable widget that gained focus', () => { + let tracker = createTracker(); + let widget0 = createWidget(); + let widget1 = createWidget(); + widget1.focusTrackable = false; + focusWidget(widget0); + tracker.add(widget0); + tracker.add(widget1); + expect(tracker.currentWidget).to.equal(widget0); + focusWidget(widget1); + expect(tracker.currentWidget).to.equal(widget0); + }); + it('should revert to the previous widget if the current widget is removed', () => { let tracker = createTracker(); let widget0 = createWidget(); @@ -208,6 +255,20 @@ describe('@lumino/widgets', () => { widget.dispose(); expect(tracker.currentWidget).to.equal(null); }); + + it('should be `null` if the current widget is disposed', () => { + let tracker = createTracker(); + expect(tracker.currentWidget).to.equal(null); + let widget0 = createWidget(); + let widget1 = createWidget(); + widget1.focusTrackable = false; + focusWidget(widget0); + tracker.add(widget0); + tracker.add(widget1); + expect(tracker.currentWidget).to.equal(widget0); + widget0.dispose(); + expect(tracker.currentWidget).to.equal(null); + }); }); describe('#activeWidget', () => { @@ -229,6 +290,18 @@ describe('@lumino/widgets', () => { expect(tracker.activeWidget).to.equal(null); }); + it('should be set to `null` when an non-trackable widget gain focus', () => { + let tracker = createTracker(); + let widget0 = createWidget(); + let widget1 = createWidget(); + widget1.focusTrackable = false; + focusWidget(widget0); + tracker.add(widget0); + expect(tracker.activeWidget).to.equal(widget0); + focusWidget(widget1); + expect(tracker.activeWidget).to.equal(null); + }); + it('should be set to the widget that gained focus', () => { let tracker = createTracker(); let widget0 = createWidget(); @@ -251,6 +324,20 @@ describe('@lumino/widgets', () => { widget.dispose(); expect(tracker.activeWidget).to.equal(null); }); + + it('should be `null` if the active widget is disposed', () => { + let tracker = createTracker(); + expect(tracker.currentWidget).to.equal(null); + let widget0 = createWidget(); + let widget1 = createWidget(); + widget1.focusTrackable = false; + focusWidget(widget0); + tracker.add(widget0); + tracker.add(widget1); + expect(tracker.activeWidget).to.equal(widget0); + widget0.dispose(); + expect(tracker.activeWidget).to.equal(null); + }); }); describe('#widgets', () => { @@ -349,6 +436,14 @@ describe('@lumino/widgets', () => { tracker.add(widget); expect(tracker.has(widget)).to.equal(true); }); + + it('should be a no-op if the widget is not trackable', () => { + let tracker = createTracker(); + let widget = createWidget(); + widget.focusTrackable = false; + tracker.add(widget); + expect(tracker.has(widget)).to.equal(false); + }); }); describe('#remove()', () => { From 1d92d9e668ff9de9af1aa2cab7ce421f7d95627e Mon Sep 17 00:00:00 2001 From: Nicolas Brichet Date: Mon, 17 Jul 2023 15:25:25 +0200 Subject: [PATCH 3/7] Revert "Add tests on trackable attribute" This reverts commit 9324ea610eeb333a5d5532e957da9849de469316. --- .../widgets/tests/src/focustracker.spec.ts | 95 ------------------- 1 file changed, 95 deletions(-) diff --git a/packages/widgets/tests/src/focustracker.spec.ts b/packages/widgets/tests/src/focustracker.spec.ts index 69f0524c5..702be459a 100644 --- a/packages/widgets/tests/src/focustracker.spec.ts +++ b/packages/widgets/tests/src/focustracker.spec.ts @@ -109,22 +109,6 @@ describe('@lumino/widgets', () => { focusWidget(widget); expect(emitArgs).to.equal(null); }); - - it('should not be emitted when the focus widget is not tracked', () => { - let tracker = createTracker(); - let widget0 = createWidget(); - let widget1 = createWidget(); - widget1.focusTrackable = false; - tracker.add(widget0); - tracker.add(widget1); - focusWidget(widget0); - let emitArgs: FocusTracker.IChangedArgs | null = null; - tracker.currentChanged.connect((sender, args) => { - emitArgs = args; - }); - focusWidget(widget1); - expect(emitArgs).to.equal(null); - }); }); describe('#activeChanged', () => { @@ -159,24 +143,6 @@ describe('@lumino/widgets', () => { expect(emitArgs!.oldValue).to.equal(widget); expect(emitArgs!.newValue).to.equal(null); }); - - it('should be emitted even if the focus widget is not tracked', () => { - let tracker = createTracker(); - let widget0 = createWidget(); - let widget1 = createWidget(); - widget1.focusTrackable = false; - tracker.add(widget0); - tracker.add(widget1); - focusWidget(widget0); - let emitArgs: FocusTracker.IChangedArgs | null = null; - tracker.activeChanged.connect((sender, args) => { - emitArgs = args; - }); - focusWidget(widget1); - expect(emitArgs).to.not.equal(null); - expect(emitArgs!.oldValue).to.equal(widget0); - expect(emitArgs!.newValue).to.equal(null); - }); }); describe('#isDisposed', () => { @@ -219,19 +185,6 @@ describe('@lumino/widgets', () => { expect(tracker.currentWidget).to.equal(widget1); }); - it('should not be set to the non-trackable widget that gained focus', () => { - let tracker = createTracker(); - let widget0 = createWidget(); - let widget1 = createWidget(); - widget1.focusTrackable = false; - focusWidget(widget0); - tracker.add(widget0); - tracker.add(widget1); - expect(tracker.currentWidget).to.equal(widget0); - focusWidget(widget1); - expect(tracker.currentWidget).to.equal(widget0); - }); - it('should revert to the previous widget if the current widget is removed', () => { let tracker = createTracker(); let widget0 = createWidget(); @@ -255,20 +208,6 @@ describe('@lumino/widgets', () => { widget.dispose(); expect(tracker.currentWidget).to.equal(null); }); - - it('should be `null` if the current widget is disposed', () => { - let tracker = createTracker(); - expect(tracker.currentWidget).to.equal(null); - let widget0 = createWidget(); - let widget1 = createWidget(); - widget1.focusTrackable = false; - focusWidget(widget0); - tracker.add(widget0); - tracker.add(widget1); - expect(tracker.currentWidget).to.equal(widget0); - widget0.dispose(); - expect(tracker.currentWidget).to.equal(null); - }); }); describe('#activeWidget', () => { @@ -290,18 +229,6 @@ describe('@lumino/widgets', () => { expect(tracker.activeWidget).to.equal(null); }); - it('should be set to `null` when an non-trackable widget gain focus', () => { - let tracker = createTracker(); - let widget0 = createWidget(); - let widget1 = createWidget(); - widget1.focusTrackable = false; - focusWidget(widget0); - tracker.add(widget0); - expect(tracker.activeWidget).to.equal(widget0); - focusWidget(widget1); - expect(tracker.activeWidget).to.equal(null); - }); - it('should be set to the widget that gained focus', () => { let tracker = createTracker(); let widget0 = createWidget(); @@ -324,20 +251,6 @@ describe('@lumino/widgets', () => { widget.dispose(); expect(tracker.activeWidget).to.equal(null); }); - - it('should be `null` if the active widget is disposed', () => { - let tracker = createTracker(); - expect(tracker.currentWidget).to.equal(null); - let widget0 = createWidget(); - let widget1 = createWidget(); - widget1.focusTrackable = false; - focusWidget(widget0); - tracker.add(widget0); - tracker.add(widget1); - expect(tracker.activeWidget).to.equal(widget0); - widget0.dispose(); - expect(tracker.activeWidget).to.equal(null); - }); }); describe('#widgets', () => { @@ -436,14 +349,6 @@ describe('@lumino/widgets', () => { tracker.add(widget); expect(tracker.has(widget)).to.equal(true); }); - - it('should be a no-op if the widget is not trackable', () => { - let tracker = createTracker(); - let widget = createWidget(); - widget.focusTrackable = false; - tracker.add(widget); - expect(tracker.has(widget)).to.equal(false); - }); }); describe('#remove()', () => { From 9f467b1677b4fd3906186ed036437f95e0936d06 Mon Sep 17 00:00:00 2001 From: Nicolas Brichet Date: Mon, 17 Jul 2023 15:25:40 +0200 Subject: [PATCH 4/7] Revert "Add a focusTrackable attribute to Widget, to disable the focusTracker" This reverts commit a43945dd5d8869a740d5b9da8cf2393485a7dc3b. --- packages/widgets/src/focustracker.ts | 4 ++-- packages/widgets/src/tabbar.ts | 1 - packages/widgets/src/widget.ts | 14 -------------- 3 files changed, 2 insertions(+), 17 deletions(-) diff --git a/packages/widgets/src/focustracker.ts b/packages/widgets/src/focustracker.ts index 5eea8fcae..c5241833c 100644 --- a/packages/widgets/src/focustracker.ts +++ b/packages/widgets/src/focustracker.ts @@ -159,8 +159,8 @@ export class FocusTracker implements IDisposable { * If the widget is already tracked, this is a no-op. */ add(widget: T): void { - // Do nothing if the widget is already tracked or is not trackable. - if (this._numbers.has(widget) || !widget.focusTrackable) { + // Do nothing if the widget is already tracked. + if (this._numbers.has(widget)) { return; } diff --git a/packages/widgets/src/tabbar.ts b/packages/widgets/src/tabbar.ts index 5087ff986..3580e7157 100644 --- a/packages/widgets/src/tabbar.ts +++ b/packages/widgets/src/tabbar.ts @@ -62,7 +62,6 @@ export class TabBar extends Widget { this.orientation = options.orientation || 'horizontal'; this.removeBehavior = options.removeBehavior || 'select-tab-after'; this.renderer = options.renderer || TabBar.defaultRenderer; - this.focusTrackable = false; } /** diff --git a/packages/widgets/src/widget.ts b/packages/widgets/src/widget.ts index 1c8239946..53bf771fe 100644 --- a/packages/widgets/src/widget.ts +++ b/packages/widgets/src/widget.ts @@ -270,19 +270,6 @@ export class Widget implements IMessageHandler, IObservableDisposable { value!.parent = this; } - /** - * The 'focusTrackable' attribute allows the FocusTracker to track the widget focus. - * Its default value is true. - * - * If false, the FocusTracker will never set this widget as current or active. - */ - get focusTrackable(): boolean { - return this._focusTrackable; - } - set focusTrackable(value: boolean) { - this._focusTrackable = value; - } - /** * Create an iterator over the widget's children. * @@ -785,7 +772,6 @@ export class Widget implements IMessageHandler, IObservableDisposable { private _parent: Widget | null = null; private _disposed = new Signal(this); private _hiddenMode: Widget.HiddenMode = Widget.HiddenMode.Display; - private _focusTrackable = true; } /** From a0fb62797b59abfd995c9805793a1baadf3ce3b7 Mon Sep 17 00:00:00 2001 From: Nicolas Brichet Date: Mon, 17 Jul 2023 15:27:06 +0200 Subject: [PATCH 5/7] Attach the tabbar to the dock panel without setting its parent --- packages/widgets/src/docklayout.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/widgets/src/docklayout.ts b/packages/widgets/src/docklayout.ts index 9f75c4ea4..7128405ad 100644 --- a/packages/widgets/src/docklayout.ts +++ b/packages/widgets/src/docklayout.ts @@ -1128,9 +1128,8 @@ export class DockLayout extends Layout { // Enforce necessary tab bar behavior. tabBar.orientation = 'horizontal'; - // Reparent and attach the tab bar to the parent if possible. + // Attach the tab bar to the parent if possible. if (this.parent) { - tabBar.parent = this.parent; this.attachWidget(tabBar); } From 63fa5e93fa3d56a4884fd96076bcf51bd96712bf Mon Sep 17 00:00:00 2001 From: Nicolas Brichet Date: Mon, 17 Jul 2023 16:52:01 +0200 Subject: [PATCH 6/7] Add test on tabbar not parented to docklayout --- packages/widgets/tests/src/dockpanel.spec.ts | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/packages/widgets/tests/src/dockpanel.spec.ts b/packages/widgets/tests/src/dockpanel.spec.ts index e41b56e7d..93d1d9071 100644 --- a/packages/widgets/tests/src/dockpanel.spec.ts +++ b/packages/widgets/tests/src/dockpanel.spec.ts @@ -44,6 +44,15 @@ describe('@lumino/widgets', () => { let panel = new DockPanel(); expect(panel.hasClass('lm-DockPanel')).to.equal(true); }); + + it('should not have tabbar as child', () => { + let panel = new DockPanel(); + let w1 = new Widget(); + panel.addWidget(w1); + for (const tabBar of panel.tabBars()) { + expect(panel.contains(tabBar)).to.be.false; + } + }); }); describe('#dispose()', () => { From f0a3d334983c5a31a6c52a8433293cee64c0f9f2 Mon Sep 17 00:00:00 2001 From: Nicolas Brichet Date: Wed, 19 Jul 2023 13:07:52 +0200 Subject: [PATCH 7/7] Add comment in test --- packages/widgets/tests/src/dockpanel.spec.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/widgets/tests/src/dockpanel.spec.ts b/packages/widgets/tests/src/dockpanel.spec.ts index 93d1d9071..785476191 100644 --- a/packages/widgets/tests/src/dockpanel.spec.ts +++ b/packages/widgets/tests/src/dockpanel.spec.ts @@ -47,8 +47,9 @@ describe('@lumino/widgets', () => { it('should not have tabbar as child', () => { let panel = new DockPanel(); - let w1 = new Widget(); - panel.addWidget(w1); + // Adding a widget in the dock panel adds the DOM of a TabBar, but the TabBar + // widget should not be a in the children list of the DockPanel widget. + panel.addWidget(new Widget()); for (const tabBar of panel.tabBars()) { expect(panel.contains(tabBar)).to.be.false; }