fix: Update cursor location in response to SELECT events (#194)

* refactor!: Add a .workspace property to LineCursor

  Add a .workspace property to LineCursor instances, and a
  corresponding workspace parameter to the class constructor, so
  that LineCursor instances know explicitly which WorkspaceSvg
  they are associated with.

* fix: Update cursor location in response to SELECT events

  This solves the issue where, when the cursor was not on a block,
  clicking on a block to select it would not immediately cause
  the displayed cursor to disappear, but the cursor location would
  only be updated when a cursor key was pressed.

* refactor: Make it possible to uninstall the LineCursor

  Turn installCursor into an install method on LineCursor, and add
  a coresponding uninstall method.

* chore: Update comments for PR #194.

Author: cpcallen

src/index.ts

@@ -6,7 +6,7 @@
 
 import * as Blockly from 'blockly/core';
 import {NavigationController} from './navigation_controller';
-import {installCursor} from './line_cursor';
+import {LineCursor} from './line_cursor';
 
 /** Plugin for keyboard navigation. */
 export class KeyboardNavigation {
@@ -22,14 +22,16 @@ export class KeyboardNavigation {
   /** Keyboard navigation controller instance for the workspace. */
   private navigationController: NavigationController;
 
+  /** Cursor for the main workspace. */
+  private cursor: LineCursor;
+
   /**
    * These fields are used to preserve the workspace's initial state to restore
    * it when/if keyboard navigation is disabled.
    */
   private injectionDivTabIndex: string | null;
   private workspaceParentTabIndex: string | null;
   private originalTheme: Blockly.Theme;
-  private originalCursor: Blockly.Cursor | null;
 
   /**
    * Constructs the keyboard navigation.
@@ -48,8 +50,9 @@ export class KeyboardNavigation {
 
     this.originalTheme = workspace.getTheme();
     this.setGlowTheme();
-    this.originalCursor = workspace.getMarkerManager().getCursor();
-    installCursor(workspace.getMarkerManager());
+
+    this.cursor = new LineCursor(workspace);
+    this.cursor.install();
 
     // Ensure that only the root SVG G (group) has a tab index.
     this.injectionDivTabIndex = workspace
@@ -106,10 +109,7 @@ export class KeyboardNavigation {
         .setAttribute('tabindex', this.injectionDivTabIndex);
     }
 
-    if (this.originalCursor) {
-      const markerManager = this.workspace.getMarkerManager();
-      markerManager.setCursor(this.originalCursor);
-    }
+    this.cursor.uninstall();
 
     this.workspace.setTheme(this.originalTheme);
 

src/line_cursor.ts

@@ -22,11 +22,49 @@ import {ASTNode, Marker} from 'blockly/core';
 export class LineCursor extends Marker {
   override type = 'cursor';
 
+  /** Has the cursor been installed in a workspace's marker manager? */
+  private installed = false;
+
+  /** Old Cursor instance, saved during installation. */
+  private oldCursor: Blockly.Cursor | null = null;
+
   /**
-   * Constructor for a line cursor.
+   * @param workspace The workspace this cursor belongs to.
    */
-  constructor() {
+  constructor(public readonly workspace: Blockly.WorkspaceSvg) {
     super();
+    // Bind selectListener to facilitate future install/uninstall.
+    this.selectListener = this.selectListener.bind(this);
+  }
+
+  /**
+   * Install this LineCursor in its workspace's marker manager and set
+   * up the select listener.  The original cursor (if any) is saved
+   * for future use by .uninstall(), and its location is used to set
+   * this one's.
+   */
+  install() {
+    if (this.installed) throw new Error('LineCursor already installed');
+    const markerManager = this.workspace.getMarkerManager();
+    this.oldCursor = markerManager.getCursor();
+    markerManager.setCursor(this);
+    if (this.oldCursor) this.setCurNode(this.oldCursor.getCurNode());
+    this.workspace.addChangeListener(this.selectListener);
+    this.installed = true;
+  }
+
+  /**
+   * Remove the select listener and uninstall this LineCursor from its
+   * workspace's marker manager, restoring any previously-existing
+   * cursor.  Does not attempt to adjust original cursor's location.
+   */
+  uninstall() {
+    if (!this.installed) throw new Error('LineCursor not yet installed');
+    this.workspace.removeChangeListener(this.selectListener.bind(this));
+    if (this.oldCursor) {
+      this.workspace.getMarkerManager().setCursor(this.oldCursor);
+    }
+    this.installed = false;
   }
 
   /**
@@ -410,25 +448,24 @@ export class LineCursor extends Marker {
    * if so, update the cursor location (and any highlighting) to
    * match.
    *
-   * This works reasonably well but has some glitches, most notably
-   * that if the cursor is not on a block (e.g. it is on a connection
-   * or the workspace) then it will remain visible in its previous
-   * location until a cursor key is pressed.
+   * Doing this only when getCurNode would naturally be called works
+   * reasonably well but has some glitches, most notably that if the
+   * cursor was not on a block (e.g. it was on a connection or the
+   * workspace) when the user selected a block then it will remain
+   * visible in its previous location until some keyboard navigation occurs.
+   *
+   * To ameliorate this, the LineCursor constructor adds an event
+   * listener that calls getCurNode in response to SELECTED events.
    *
-   * TODO(#97): Remove this hack once Blockly is modified to update
-   * the cursor/focus itself.
+   * Remove this hack once Blockly is modified to update the
+   * cursor/focus itself.
    *
    * @returns The current field, connection, or block the cursor is on.
    */
   override getCurNode(): ASTNode {
     const curNode = super.getCurNode();
     const selected = Blockly.common.getSelected();
-    if (
-      (selected?.workspace as Blockly.WorkspaceSvg)
-        ?.getMarkerManager()
-        .getCursor() !== this
-    )
-      return curNode;
+    if (selected?.workspace !== this.workspace) return curNode;
 
     // Selected item is on workspace that this cursor belongs to.
     const curLocation = curNode?.getLocation();
@@ -511,6 +548,17 @@ export class LineCursor extends Marker {
 
     drawer.draw(oldNode, newNode);
   }
+
+  /**
+   * Event listener that syncs the cursor location to the selected
+   * block on SELECTED events.
+   */
+  private selectListener(event: Blockly.Events.Abstract) {
+    if (event.type !== Blockly.Events.SELECTED) return;
+    const selectedEvent = event as Blockly.Events.Selected;
+    if (selectedEvent.workspaceId !== this.workspace.id) return;
+    this.getCurNode();
+  }
 }
 
 export const registrationName = 'LineCursor';
@@ -521,18 +569,3 @@ Blockly.registry.register(registrationType, registrationName, LineCursor);
 export const pluginInfo = {
   [registrationType.toString()]: registrationName,
 };
-
-/**
- * Install this cursor on the marker manager in the same position as
- * the previous cursor.
- *
- * @param markerManager The currently active marker manager.
- */
-export function installCursor(markerManager: Blockly.MarkerManager) {
-  const oldCurNode = markerManager.getCursor()?.getCurNode();
-  const lineCursor = new LineCursor();
-  markerManager.setCursor(lineCursor);
-  if (oldCurNode) {
-    markerManager.getCursor()?.setCurNode(oldCurNode);
-  }
-}