Improve signal lifecycle (#1009)

Use the filebrowser tracker everywhere

Author: fcollonval

package.json

@@ -91,7 +91,6 @@
     "@types/jest": "^26.0.0",
     "@types/react": "^17.0.0",
     "@types/react-dom": "^17.0.0",
-    "@types/react-textarea-autosize": "^4.3.5",
     "@types/react-virtualized-auto-sizer": "^1.0.0",
     "@types/react-window": "^1.8.2",
     "@typescript-eslint/eslint-plugin": "^4.13.0",

src/commandsAndMenu.tsx

@@ -10,8 +10,8 @@ import {
   WidgetTracker
 } from '@jupyterlab/apputils';
 import { PathExt, URLExt } from '@jupyterlab/coreutils';
-import { FileBrowser, FileBrowserModel } from '@jupyterlab/filebrowser';
-import { Contents } from '@jupyterlab/services';
+import { FileBrowser } from '@jupyterlab/filebrowser';
+import { Contents, ContentsManager } from '@jupyterlab/services';
 import { ISettingRegistry } from '@jupyterlab/settingregistry';
 import { ITerminal } from '@jupyterlab/terminal';
 import { TranslationBundle } from '@jupyterlab/translation';
@@ -101,7 +101,7 @@ function pluralizedContextLabel(singular: string, plural: string) {
 export function addCommands(
   app: JupyterFrontEnd,
   gitModel: GitExtension,
-  fileBrowserModel: FileBrowserModel,
+  browserTracker: WidgetTracker<FileBrowser>,
   settings: ISettingRegistry.ISettings,
   trans: TranslationBundle
 ): void {
@@ -178,7 +178,7 @@ export function addCommands(
       'Create an empty Git repository or reinitialize an existing one'
     ),
     execute: async () => {
-      const currentPath = fileBrowserModel.path;
+      const currentPath = browserTracker.currentWidget.model.path;
       const result = await showDialog({
         title: trans.__('Initialize a Repository'),
         body: trans.__('Do you really want to make this directory a Git Repo?'),
@@ -311,14 +311,14 @@ export function addCommands(
             gitModel,
             Operation.Clone,
             trans,
-            { path: fileBrowserModel.path, url: result.value }
+            { path: browserTracker.currentWidget.model.path, url: result.value }
           );
           logger.log({
             message: trans.__('Successfully cloned'),
             level: Level.SUCCESS,
             details
           });
-          await fileBrowserModel.refresh();
+          await browserTracker.currentWidget.model.refresh();
         } catch (error) {
           console.error(
             'Encountered an error when cloning the repository. Error: ',
@@ -483,8 +483,13 @@ export function addCommands(
             refreshButton.hide();
             diffWidget.toolbar.addItem('refresh', refreshButton);
 
-            model.changed.connect(() => {
+            const refresh = () => {
               refreshButton.show();
+            };
+
+            model.changed.connect(refresh);
+            widget.disposed.connect(() => {
+              model.changed.disconnect(refresh);
             });
 
             // Load the diff widget
@@ -629,17 +634,26 @@ export function addCommands(
         if (widget) {
           // Trigger diff model update
           if (diffContext.previousRef === 'HEAD') {
-            gitModel.headChanged.connect(() => {
+            const updateHead = () => {
               model.reference = {
                 ...model.reference,
                 updateAt: Date.now()
               };
+            };
+
+            gitModel.headChanged.connect(updateHead);
+
+            widget.disposed.connect(() => {
+              gitModel.headChanged.disconnect(updateHead);
             });
           }
+
           // If the diff is on the current file and it is updated => diff model changed
           if (diffContext.currentRef === Git.Diff.SpecialRef.WORKING) {
-            // More robust than fileBrowser.model.fileChanged
-            app.serviceManager.contents.fileChanged.connect((_, change) => {
+            const updateCurrent = (
+              m: ContentsManager,
+              change: Contents.IChangedArgs
+            ) => {
               const updateAt = new Date(
                 change.newValue.last_modified
               ).valueOf();
@@ -652,6 +666,13 @@ export function addCommands(
                   updateAt
                 };
               }
+            };
+
+            // More robust than fileBrowser.model.fileChanged
+            app.serviceManager.contents.fileChanged.connect(updateCurrent);
+
+            widget.disposed.connect(() => {
+              app.serviceManager.contents.fileChanged.disconnect(updateCurrent);
             });
           }
         }

src/components/StatusWidget.tsx

@@ -89,7 +89,12 @@ export function addStatusBarWidget(
     isActive: Private.isStatusWidgetActive(settings),
     activeStateChanged: settings && settings.changed
   });
-  model.taskChanged.connect(Private.createEventCallback(statusWidget));
+
+  const callback = Private.createEventCallback(statusWidget);
+  model.taskChanged.connect(callback);
+  statusWidget.disposed.connect(() => {
+    model.taskChanged.disconnect(callback);
+  });
 }
 /* eslint-disable no-inner-declarations */
 namespace Private {

src/index.ts

@@ -74,7 +74,7 @@ async function activate(
   let settings: ISettingRegistry.ISettings;
   let serverSettings: Git.IServerSettings;
   // Get a reference to the default file browser extension
-  const filebrowser = factory.tracker.currentWidget;
+  let filebrowser = factory.tracker.currentWidget;
   translator = translator || nullTranslator;
   const trans = translator.load('jupyterlab_git');
 
@@ -137,16 +137,34 @@ async function activate(
     gitExtension.pathRepository = filebrowser.model.path;
   });
 
+  const onPathChanged = (
+    model: FileBrowserModel,
+    change: IChangedArgs<string>
+  ) => {
+    gitExtension.pathRepository = change.newValue;
+  };
+
   // Whenever the file browser path changes, sync the Git extension path
-  filebrowser.model.pathChanged.connect(
-    (model: FileBrowserModel, change: IChangedArgs<string>) => {
-      gitExtension.pathRepository = change.newValue;
-    }
-  );
+  filebrowser.model.pathChanged.connect(onPathChanged);
 
-  // Whenever the `HEAD` of the Git repository changes, refresh the file browser
-  gitExtension.headChanged.connect(() => {
+  const refreshBrowser = () => {
     filebrowser.model.refresh();
+  };
+
+  // Whenever the `HEAD` of the Git repository changes, refresh the file browser
+  gitExtension.headChanged.connect(refreshBrowser);
+
+  // Handle file browser changes
+  factory.tracker.currentChanged.connect((_, browser) => {
+    filebrowser.model.pathChanged.disconnect(onPathChanged);
+
+    filebrowser = browser;
+    gitExtension.pathRepository = filebrowser.model.path;
+    filebrowser.model.pathChanged.connect(onPathChanged);
+
+    if (settings) {
+      addCloneButton(gitExtension, filebrowser, app.commands);
+    }
   });
 
   // Whenever a user adds/renames/saves/deletes/modifies a file within the lab environment, refresh the Git status
@@ -157,14 +175,14 @@ async function activate(
   // Provided we were able to load application settings, create the extension widgets
   if (settings) {
     // Add JupyterLab commands
-    addCommands(app, gitExtension, filebrowser.model, settings, trans);
+    addCommands(app, gitExtension, factory.tracker, settings, trans);
 
     // Create the Git widget sidebar
     const gitPlugin = new GitWidget(
       gitExtension,
       settings,
       app.commands,
-      filebrowser.model,
+      factory.tracker,
       trans
     );
     gitPlugin.id = 'jp-git-sessions';

src/model.ts

@@ -633,6 +633,7 @@ export class GitExtension implements IGitExtension {
     this._fetchPoll.dispose();
     this._statusPoll.dispose();
     this._taskHandler.dispose();
+    this._settings.changed.disconnect(this._onSettingsChange, this);
     Signal.clearData(this);
   }
 

src/widgets/GitWidget.tsx

@@ -1,5 +1,5 @@
-import { ReactWidget, UseSignal } from '@jupyterlab/apputils';
-import { FileBrowserModel } from '@jupyterlab/filebrowser';
+import { ReactWidget, UseSignal, WidgetTracker } from '@jupyterlab/apputils';
+import { FileBrowser } from '@jupyterlab/filebrowser';
 import { ISettingRegistry } from '@jupyterlab/settingregistry';
 import { TranslationBundle } from '@jupyterlab/translation';
 import { CommandRegistry } from '@lumino/commands';
@@ -22,7 +22,7 @@ export class GitWidget extends ReactWidget {
     model: GitExtension,
     settings: ISettingRegistry.ISettings,
     commands: CommandRegistry,
-    filebrowser: FileBrowserModel,
+    browserTracker: WidgetTracker<FileBrowser>,
     trans: TranslationBundle,
     options?: Widget.IOptions
   ) {
@@ -32,7 +32,7 @@ export class GitWidget extends ReactWidget {
 
     this._trans = trans;
     this._commands = commands;
-    this._filebrowser = filebrowser;
+    this._browserTracker = browserTracker;
     this._model = model;
     this._settings = settings;
 
@@ -67,14 +67,21 @@ export class GitWidget extends ReactWidget {
         <LoggerContext.Consumer>
           {logger => (
             <React.Fragment>
-              <GitPanel
-                commands={this._commands}
-                filebrowser={this._filebrowser}
-                logger={logger}
-                model={this._model}
-                settings={this._settings}
-                trans={this._trans}
-              />
+              <UseSignal
+                signal={this._browserTracker.currentChanged}
+                initialArgs={this._browserTracker.currentWidget}
+              >
+                {(tracker, filebrowser) => (
+                  <GitPanel
+                    commands={this._commands}
+                    filebrowser={filebrowser.model}
+                    logger={logger}
+                    model={this._model}
+                    settings={this._settings}
+                    trans={this._trans}
+                  />
+                )}
+              </UseSignal>
               <UseSignal
                 signal={logger.signal}
                 initialArgs={{ message: '', level: Level.INFO } as ILogMessage}
@@ -97,7 +104,7 @@ export class GitWidget extends ReactWidget {
   }
 
   private _commands: CommandRegistry;
-  private _filebrowser: FileBrowserModel;
+  private _browserTracker: WidgetTracker<FileBrowser>;
   private _model: GitExtension;
   private _settings: ISettingRegistry.ISettings;
   private _trans: TranslationBundle;

yarn.lock

@@ -7047,7 +7047,7 @@ prop-types-exact@^1.2.0:
     object.assign "^4.1.0"
     reflect.ownkeys "^0.2.0"
 
-prop-types@^15.6.0, prop-types@^15.6.1, prop-types@^15.6.2, prop-types@^15.7.2:
+prop-types@^15.6.1, prop-types@^15.6.2, prop-types@^15.7.2:
   version "15.7.2"
   resolved "https://registry.yarnpkg.com/prop-types/-/prop-types-15.7.2.tgz#52c41e75b8c87e72b9d9360e0206b99dcbffa6c5"
   integrity sha512-8QQikdH7//R2vurIJSutZ1smHYTcLpRWEOlHnzcWHmBYrOGUysKwSsrC89BCiFj3CbrfJ/nXFdJepOVrY1GCHQ==
@@ -7213,14 +7213,6 @@ react-test-renderer@^17.0.0:
     react-shallow-renderer "^16.13.1"
     scheduler "^0.20.2"
 
-react-textarea-autosize@^7.1.2:
-  version "7.1.2"
-  resolved "https://registry.yarnpkg.com/react-textarea-autosize/-/react-textarea-autosize-7.1.2.tgz#70fdb333ef86bcca72717e25e623e90c336e2cda"
-  integrity sha512-uH3ORCsCa3C6LHxExExhF4jHoXYCQwE5oECmrRsunlspaDAbS4mGKNlWZqjLfInWtFQcf0o1n1jC/NGXFdUBCg==
-  dependencies:
-    "@babel/runtime" "^7.1.2"
-    prop-types "^15.6.0"
-
 react-transition-group@^2.9.0:
   version "2.9.0"
   resolved "https://registry.yarnpkg.com/react-transition-group/-/react-transition-group-2.9.0.tgz#df9cdb025796211151a436c69a8f3b97b5b07c8d"