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.

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

Fix extra launcher flimflam #4805

Merged
merged 4 commits into from Jul 2, 2018

Conversation

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Jun 28, 2018

Fixes #4228.

This wound up being pretty subtle. We have some logic with the launcher where, when the dock layout is modified, we check whether the dock is empty. If the dock is empty, we add a new launcher. The problem is: some dock modified signals are emitted during the rehydration of the layout from single document mode. In particular, one of them is emitted in a transient state when the dock is empty. This triggers a new launcher being added to the main area in the middle of rehydration. This seems to make the DOM structure (to use technical terms) all messed up. I found a bunch of different ways to trigger this behavior upon leaving single document mode, one of which is demonstrated in #4228.

This PR does the following:

  1. Adds a guard to the application layoutModified signal so that it is only emitted once upon changing the dock mode.
  2. Caches the widget add options while in single document mode so that they may be added in the location that the user requested once you go back to multi-document mode.
  3. Fixes a bug in the logic for which widget gets focus upon leaving single document mode.

It is a bit messy, so if you have other ideas about how to fix this, I am all ears.

@afshin
Copy link
Member

@afshin afshin commented Jun 29, 2018

I think this could be fixed by debouncing emissions of the layout modified signal to make sure they effectively become conflated and are emitted after modification has finished. The diff below appears to fix these issues and is pretty unobtrusive. What do you think?

diff --git a/packages/application/src/shell.ts b/packages/application/src/shell.ts
index 7ebecf43c..35c0fe70c 100644
--- a/packages/application/src/shell.ts
+++ b/packages/application/src/shell.ts
@@ -376,7 +376,7 @@ class ApplicationShell extends Widget {
     }
     let rank = 'rank' in options ? options.rank : DEFAULT_RANK;
     this._leftHandler.addWidget(widget, rank!);
-    this._layoutModified.emit(void 0);
+    this._onLayoutModified();
   }

   /**
@@ -667,7 +667,10 @@ class ApplicationShell extends Widget {
    * Handle a change to the layout.
    */
   private _onLayoutModified(): void {
-    this._layoutModified.emit(void 0);
+    window.clearTimeout(this._debouncer);
+    this._debouncer = window.setTimeout(() => {
+      this._layoutModified.emit(undefined);
+    });
   }

   /**
@@ -692,6 +695,7 @@ class ApplicationShell extends Widget {
   private _activeChanged = new Signal<this, ApplicationShell.IChangedArgs>(this);
   private _cachedLayout: DockLayout.ILayoutConfig | null = null;
   private _currentChanged = new Signal<this, ApplicationShell.IChangedArgs>(this);
+  private _debouncer = 0;
   private _dockPanel: DockPanel;
   private _hboxPanel: BoxPanel;
   private _hsplitPanel: SplitPanel;

afshin
afshin approved these changes Jul 2, 2018
Copy link
Member

@afshin afshin left a comment

👍
Thanks, @ian-r-rose!

@afshin afshin merged commit cfa1a15 into jupyterlab:master Jul 2, 2018
2 checks passed
@ian-r-rose ian-r-rose mentioned this pull request Jul 20, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Aug 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants