Clean up stale ourSelections to not indefinitely ignore external selection changes (#9644)

- Add updatedAt timestamps to ourSelections (now named ourSelectionsUpdates)
- During selection change handler, clean up ourSelectionsUpdates older than 1s, which
  should have already triggered selection change events
- This prevents external selection updates from being ignored for too long when internal
  updates have been misguidedly added to ourSelections (which is hard to fully avoid due
  to vscode.editor.selections unreliability)

Fixes #8903, fixes #5746, fixes #5555, fixes: #8765

Author: alythobani

extensionBase.ts

@@ -8,6 +8,7 @@ import { Jump } from './src/jumps/jump';
 import { Mode } from './src/mode/mode';
 import { ModeHandler } from './src/mode/modeHandler';
 import { ModeHandlerMap } from './src/mode/modeHandlerMap';
+import { cleanupOurSelectionsUpdates, hashSelections } from './src/mode/ourSelectionsUpdates';
 import { Register } from './src/register/register';
 import { CompositionState } from './src/state/compositionState';
 import { globalState } from './src/state/globalState';
@@ -283,24 +284,27 @@ export async function activate(context: vscode.ExtensionContext, handleLocal: bo
         return;
       }
 
+      // Clean up stale `ourSelectionUpdates` that never triggered a selectionChange event, before
+      // we use them in our logic below
+      mh.selectionsChanged.ourSelectionsUpdates = cleanupOurSelectionsUpdates(
+        mh.selectionsChanged.ourSelectionsUpdates,
+      );
+
       if (e.kind !== vscode.TextEditorSelectionChangeKind.Mouse) {
-        const selectionsHash = e.selections.reduce(
-          (hash, s) =>
-            hash +
-            `[${s.anchor.line}, ${s.anchor.character}; ${s.active.line}, ${s.active.character}]`,
-          '',
+        const selectionsHash = hashSelections(e.selections);
+        const idx = mh.selectionsChanged.ourSelectionsUpdates.findIndex(
+          (entry) => entry.selectionsHash === selectionsHash,
         );
-        const idx = mh.selectionsChanged.ourSelections.indexOf(selectionsHash);
         if (idx > -1) {
-          mh.selectionsChanged.ourSelections.splice(idx, 1);
+          mh.selectionsChanged.ourSelectionsUpdates.splice(idx, 1);
           Logger.trace(
-            `Ignoring selection: ${selectionsHash}. ${mh.selectionsChanged.ourSelections.length} left`,
+            `Ignoring selection: ${selectionsHash}. ${mh.selectionsChanged.ourSelectionsUpdates.length} left`,
           );
           return;
         } else if (mh.selectionsChanged.ignoreIntermediateSelections) {
           Logger.trace(`Ignoring intermediate selection change: ${selectionsHash}`);
           return;
-        } else if (mh.selectionsChanged.ourSelections.length > 0) {
+        } else if (mh.selectionsChanged.ourSelectionsUpdates.length > 0) {
           // Some intermediate selection must have slipped in after setting the
           // 'ignoreIntermediateSelections' to false. Which means we didn't count
           // for it yet, but since we have selections to be ignored then we probably

src/mode/modeHandler.ts

@@ -11,6 +11,7 @@ import { IBaseAction } from '../actions/types';
 import { Cursor } from '../common/motion/cursor';
 import { configuration } from '../configuration/configuration';
 import { decoration } from '../configuration/decoration';
+import { isLiteralMode, remapKey } from '../configuration/langmap';
 import { Notation } from '../configuration/notation';
 import { Remappers } from '../configuration/remapper';
 import { Jump } from '../jumps/jump';
@@ -58,7 +59,7 @@ import {
   isStatusBarMode,
   isVisualMode,
 } from './mode';
-import { isLiteralMode, remapKey } from '../configuration/langmap';
+import { OurSelectionsUpdate, hashSelections } from './ourSelectionsUpdates';
 
 interface IModeHandlerMap {
   get(editorId: Uri): ModeHandler | undefined;
@@ -88,7 +89,7 @@ export class ModeHandler implements vscode.Disposable, IModeHandler {
    * Used internally to ignore selection changes that were performed by us.
    * 'ignoreIntermediateSelections': set to true when running an action, during this time
    * all selections change events will be ignored.
-   * 'ourSelections': keeps track of our selections that will trigger a selection change event
+   * 'ourSelectionsUpdates': keeps track of our selections that will trigger a selection change event
    * so that we can ignore them.
    */
   public selectionsChanged = {
@@ -101,7 +102,7 @@ export class ModeHandler implements vscode.Disposable, IModeHandler {
      * keeps track of our selections that will trigger a selection change event
      * so that we can ignore them.
      */
-    ourSelections: Array<string>(),
+    ourSelectionsUpdates: Array<OurSelectionsUpdate>(),
   };
 
   /**
@@ -1456,16 +1457,11 @@ export class ModeHandler implements vscode.Disposable, IModeHandler {
         );
 
       if (willTriggerChange) {
-        const selectionsHash = selections.reduce(
-          (hash, s) =>
-            hash +
-            `[${s.anchor.line}, ${s.anchor.character}; ${s.active.line}, ${s.active.character}]`,
-          '',
-        );
-        this.selectionsChanged.ourSelections.push(selectionsHash);
+        const selectionsHash = hashSelections(selections);
+        this.selectionsChanged.ourSelectionsUpdates.push({ selectionsHash, updatedAt: Date.now() });
         Logger.trace(
           `Adding selection change to be ignored! (total: ${
-            this.selectionsChanged.ourSelections.length
+            this.selectionsChanged.ourSelectionsUpdates.length
           }) Hash: ${selectionsHash}, Selections: ${selections[0].anchor.toString()}, ${selections[0].active.toString()}`,
         );
       }

src/mode/ourSelectionsUpdates.ts

@@ -0,0 +1,59 @@
+import * as vscode from 'vscode';
+
+import { Logger } from '../util/logger';
+
+/** Expiry time for our selections updates queue, after which we can reasonably expect a selection
+ * update to have triggered a VSCode selection change event resulting in its dequeueing.
+ *
+ * We want the expiry time to be generous enough to allow for delayed VSCode event firings. If we
+ * remove an update too early, we might mistakenly treat its change event as an external change that
+ * we should be tracking, rather than an internal one we've already accounted for.
+ *
+ * We also want the expiry time to be short enough to discard stale internal updates, whose change
+ * events have likely already fired, or will never fire. Note that VSCode selectionChange events
+ * only fire if the editor's selections have actually changed in value, hence why we compare against
+ * `editor.selections` to choose whether to add an update to `ourSelectionsUpdates`. But because we
+ * receive VSCode's selection events at a different rate from our own updates, `editor.selections`
+ * can be outdated when we do that comparison, and we can mistakenly think an internal update will
+ * trigger a selectionChange event when it actually won't. Such an update will be added to
+ * `ourSelectionsUpdates`, and will only be removed when we clean it up after the expiry time. */
+const SELECTIONS_UPDATE_EXPIRY_MS = 1000;
+
+/**
+ * An internally-triggered selections update, which will trigger a VSCode selection change event
+ * that we can ignore as an already-tracked change.
+ */
+export type OurSelectionsUpdate = {
+  selectionsHash: string;
+  updatedAt: number;
+};
+
+/**
+ * Removes our selections updates that are older than the expiry time. Ideally there shouldn't be
+ * any stale updates present when this is called. But if there are, we remove them so that VSCode
+ * selectionChange events aren't being ignored indefinitely under the incorrect assumption that
+ * they were triggered by internal updates we've already tracked.
+ */
+export function cleanupOurSelectionsUpdates(
+  ourSelectionsUpdates: OurSelectionsUpdate[],
+): OurSelectionsUpdate[] {
+  const now = Date.now();
+  return ourSelectionsUpdates.filter((entry) => {
+    const age = now - entry.updatedAt;
+    const shouldKeep = age < SELECTIONS_UPDATE_EXPIRY_MS;
+    if (!shouldKeep) {
+      Logger.warn(
+        `OurSelectionsUpdate ${entry.selectionsHash} still around after ${age}ms; removing now`,
+      );
+    }
+    return shouldKeep;
+  });
+}
+
+export function hashSelections(selections: readonly vscode.Selection[]): string {
+  return selections.reduce(
+    (hash, s) =>
+      hash + `[${s.anchor.line}, ${s.anchor.character}; ${s.active.line}, ${s.active.character}]`,
+    '',
+  );
+}