Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use composition to improve tab switch #231

Merged
merged 16 commits into from
Nov 4, 2021
Merged

Use composition to improve tab switch #231

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
3d9e625
Use composition to improve tab switch
fcollonval Sep 14, 2021
4212eb6
Allow to switch `hiddenMode` a posteriori
fcollonval Sep 16, 2021
6bb08b7
Rename enum
fcollonval Sep 16, 2021
1da9dd4
Add tests and improve documentation
fcollonval Sep 16, 2021
b9dff75
Lint the code
fcollonval Sep 30, 2021
fa04b57
Edit documentation strings for consistency and completeness.
jasongrout Oct 9, 2021
e1f6bdc
Do an early return if setting is a no-op for consistency with other c…
jasongrout Oct 9, 2021
29114e6
Apply review suggestions
fcollonval Oct 13, 2021
e222200
Lint files
fcollonval Oct 13, 2021
6d3df4e
Do not set style willChange attributes in stackedlayout.
jasongrout Oct 14, 2021
15ae9c6
Set the hiddenMode for widgets that are added to docklayout and stack…
jasongrout Oct 14, 2021
418e206
Do not change the widget hidden mode styling in stackedlayout.
jasongrout Oct 14, 2021
922b0f5
Fix vocabulary in tests from old hidden modes to current terminology.
jasongrout Oct 14, 2021
46962a7
Reset the widget hidden mode after the widget is detached from a stac…
jasongrout Oct 14, 2021
ea4db6a
Lint code
fcollonval Oct 15, 2021
8cb3932
no hidden mode change when removing from dockpanel
fcollonval Oct 16, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Rename enum
  • Loading branch information
@fcollonval
fcollonval committed Sep 30, 2021
commit 6bb08b780f9116ddd172261fc6a5c8f33da45375
26 changes: 10 additions & 16 deletions packages/widgets/src/docklayout.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,10 +53,7 @@ export class DockLayout extends Layout {
if (options.spacing !== undefined) {
this._spacing = Utils.clampDimension(options.spacing);
}
this._hiddenMode =
options.hiddenMode !== undefined
? options.hiddenMode
: Widget.HiddenMode.Class;
this._hiddenMode = options.hiddenMode !== undefined ? options.hiddenMode : Widget.HiddenMode.Display;
}

/**
Expand Down Expand Up @@ -108,10 +105,10 @@ export class DockLayout extends Layout {
title.owner.hiddenMode = this._hiddenMode;

switch(this._hiddenMode) {
case Widget.HiddenMode.Class:
case Widget.HiddenMode.Display:
title.owner.node.style.willChange = 'auto';
fcollonval marked this conversation as resolved.
Show resolved Hide resolved
break;
case Widget.HiddenMode.Composition:
case Widget.HiddenMode.Scale:
title.owner.node.style.willChange = 'transform';
break;
}
Expand Down Expand Up @@ -682,15 +679,15 @@ export class DockLayout extends Layout {

// We don't switch back the hidden mode of the widget to avoid triggering
// style recomputation
if (this._hiddenMode === Widget.HiddenMode.Composition) {
if (this._hiddenMode === Widget.HiddenMode.Scale) {
widget.node.style.willChange = 'auto';
Copy link
Contributor

@jasongrout jasongrout Oct 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain a bit more this logic around setting the node.style.willChange attribute here and just below this, and in particular why we don't want to set the hidden mode directly, and let these style.willChange attributes change at the widget level now? I think that's the only part about this PR I don't understand.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this line is changed to widget.hiddenMode = Widget.HiddenMode.Display, it will be more consistent. But if the widget is hidden, that will result in the classes on the widget node to change (addition of lm-mod-hidden). This will trigger style recomputation as the widget is not yet detached that could take some times to resolve. So this was the reason not to switch back the hidden mode. But it brings indeed unconsistency. The best would be to reset the hidden mode once we are sure the widget is detached. Do you have any suggestion to improve that part?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this does seem a bit tricky. Presumably the browser optimizes things if the widget is currently hidden?

Have we profiled this optimization of not using scaled when there is one widget, and in practice how much that helps?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Presumably the browser optimizes things if the widget is currently hidden?

Definitely. Not changing anything may indeed be the best approach.

Have we profiled this optimization of not using scaled when there is one widget, and in practice how much that helps?

Pete (Colab) commented that creating too many layers will have a negative performance impact. I try the composition change with 20 big notebooks opened simultaneously and did not see major impact (unfortunately the negative impact is probably related to the user hardware).
To limit the risk, I went for not creating unneeded layer if the stack panel was only containing one widget.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll udpate the PR to not change the hidden mode of the widget being removed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 8cb3932

}

// If there are multiple tabs, just remove the widget's tab.
if (tabNode.tabBar.titles.length > 1) {
tabNode.tabBar.removeTab(widget.title);
if (this._hiddenMode === Widget.HiddenMode.Composition) {
const existingWidget = tabNode.tabBar.titles[0].owner;
if (this._hiddenMode === Widget.HiddenMode.Scale) {
const existingWidget = tabNode.tabBar.titles[0].owner
existingWidget.node.style.willChange = 'auto';
}
return;
Expand Down Expand Up @@ -850,17 +847,14 @@ export class DockLayout extends Layout {
index = refNode.tabBar.currentIndex;
}

if (
this._hiddenMode === Widget.HiddenMode.Composition &&
refNode.tabBar.titles.length > 0
) {
if (this._hiddenMode === Widget.HiddenMode.Scale && refNode.tabBar.titles.length > 0) {
if (refNode.tabBar.titles.length == 1) {
const existingWidget = refNode.tabBar.titles[0].owner;
existingWidget.hiddenMode = Widget.HiddenMode.Composition;
const existingWidget = refNode.tabBar.titles[0].owner
existingWidget.hiddenMode = Widget.HiddenMode.Scale;
existingWidget.node.style.willChange = 'transform';
}

widget.hiddenMode = Widget.HiddenMode.Composition;
widget.hiddenMode = Widget.HiddenMode.Scale;
widget.node.style.willChange = 'transform';
}

Expand Down
17 changes: 7 additions & 10 deletions packages/widgets/src/stackedlayout.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,7 @@ import { Widget } from './widget';
export class StackedLayout extends PanelLayout {
constructor(options: StackedLayout.IOptions = {}) {
super(options);
this._hiddenMode =
options.hiddenMode !== undefined
? options.hiddenMode
: Widget.HiddenMode.Class;
this._hiddenMode = options.hiddenMode !== undefined ? options.hiddenMode : Widget.HiddenMode.Display;
}

/**
Expand All @@ -47,10 +44,10 @@ export class StackedLayout extends PanelLayout {
w.hiddenMode = this._hiddenMode;

switch(this._hiddenMode) {
case Widget.HiddenMode.Class:
case Widget.HiddenMode.Display:
w.node.style.willChange = 'auto';
break;
case Widget.HiddenMode.Composition:
case Widget.HiddenMode.Scale:
w.node.style.willChange = 'transform';
break;
}
Expand Down Expand Up @@ -86,8 +83,8 @@ export class StackedLayout extends PanelLayout {
* This is a reimplementation of the superclass method.
*/
protected attachWidget(index: number, widget: Widget): void {
if (this._hiddenMode === Widget.HiddenMode.Composition) {
widget.hiddenMode = Widget.HiddenMode.Composition;
if(this._hiddenMode === Widget.HiddenMode.Scale){
widget.hiddenMode = Widget.HiddenMode.Scale;
widget.node.style.willChange = 'transform';
}

Expand Down Expand Up @@ -146,8 +143,8 @@ export class StackedLayout extends PanelLayout {
* This is a reimplementation of the superclass method.
*/
protected detachWidget(index: number, widget: Widget): void {
if (this._hiddenMode === Widget.HiddenMode.Composition) {
widget.hiddenMode = Widget.HiddenMode.Class;
if (this._hiddenMode === Widget.HiddenMode.Scale) {
widget.hiddenMode = Widget.HiddenMode.Display;
widget.node.style.willChange = 'auto';
}

Expand Down
12 changes: 6 additions & 6 deletions packages/widgets/src/widget.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ export class Widget implements IMessageHandler, IObservableDisposable {
set hiddenMode(value: Widget.HiddenMode) {
if (value !== this._hiddenMode) {
if (this.isHidden) {
if (value === Widget.HiddenMode.Class) {
if (value === Widget.HiddenMode.Display) {
this.addClass('lm-mod-hidden');
/* <DEPRECATED> */
this.addClass('p-mod-hidden');
Expand Down Expand Up @@ -417,7 +417,7 @@ export class Widget implements IMessageHandler, IObservableDisposable {
}
this.clearFlag(Widget.Flag.IsHidden);
this.node.removeAttribute('aria-hidden');
if (this.hiddenMode === Widget.HiddenMode.Class) {
if (this.hiddenMode === Widget.HiddenMode.Display) {
this.removeClass('lm-mod-hidden');
/* <DEPRECATED> */
this.removeClass('p-mod-hidden');
Expand Down Expand Up @@ -452,7 +452,7 @@ export class Widget implements IMessageHandler, IObservableDisposable {
}
this.setFlag(Widget.Flag.IsHidden);
this.node.setAttribute('aria-hidden', 'true');
if (this.hiddenMode === Widget.HiddenMode.Class) {
if (this.hiddenMode === Widget.HiddenMode.Display) {
this.addClass('lm-mod-hidden');
/* <DEPRECATED> */
this.addClass('p-mod-hidden');
Expand Down Expand Up @@ -746,7 +746,7 @@ export class Widget implements IMessageHandler, IObservableDisposable {
private _layout: Layout | null = null;
private _parent: Widget | null = null;
private _disposed = new Signal<this, void>(this);
private _hiddenMode: Widget.HiddenMode = Widget.HiddenMode.Class;
private _hiddenMode: Widget.HiddenMode = Widget.HiddenMode.Display;
}

/**
Expand Down Expand Up @@ -792,11 +792,11 @@ export namespace Widget {
/**
* Set a 'lm-mod-hidden' class to deal with the widget visibility
*/
Class = 0,
Display = 0,
/**
* Set 'transform' to 'scale(0)' to hide it
*/
Composition
Scale
}

/**
Expand Down
8 changes: 4 additions & 4 deletions packages/widgets/tests/src/widget.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -713,7 +713,7 @@ describe('@lumino/widgets', () => {
it('should use transformation if in "composition" mode', () => {
let widget = new Widget();
Widget.attach(widget, document.body);
widget.hiddenMode = Widget.HiddenMode.Composition;
widget.hiddenMode = Widget.HiddenMode.Scale;
widget.hide();
expect(widget.hasClass('lm-mod-hidden')).to.equal(false);
expect(widget.node.style.transform).to.equal('scale(0)');
Expand All @@ -724,7 +724,7 @@ describe('@lumino/widgets', () => {
let widget = new Widget();
Widget.attach(widget, document.body);
widget.hide();
widget.hiddenMode = Widget.HiddenMode.Composition;
widget.hiddenMode = Widget.HiddenMode.Scale;
expect(widget.hasClass('lm-mod-hidden')).to.equal(false);
expect(widget.node.style.transform).to.equal('scale(0)');
widget.dispose();
Expand All @@ -733,9 +733,9 @@ describe('@lumino/widgets', () => {
it('should add class when switching from composition to class', () => {
let widget = new Widget();
Widget.attach(widget, document.body);
widget.hiddenMode = Widget.HiddenMode.Composition;
widget.hiddenMode = Widget.HiddenMode.Scale;
widget.hide();
widget.hiddenMode = Widget.HiddenMode.Class;
widget.hiddenMode = Widget.HiddenMode.Display;
expect(widget.hasClass('lm-mod-hidden')).to.equal(true);
expect(widget.node.style.transform).to.equal('');
widget.dispose();
Expand Down