Skip to content

Commit b8c9e62

Browse files
alythobanialythobani
authored
Refactor: add InternalSelectionsTracker class (followup to #9644) (#9655)
1 parent 2a3a7c1 commit b8c9e62

File tree

5 files changed

+322
-133
lines changed

5 files changed

+322
-133
lines changed

‎extensionBase.ts

Lines changed: 7 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@ import { Jump } from './src/jumps/jump';
88
import { Mode } from './src/mode/mode';
99
import { ModeHandler } from './src/mode/modeHandler';
1010
import { ModeHandlerMap } from './src/mode/modeHandlerMap';
11-
import { cleanupOurSelectionsUpdates, hashSelections } from './src/mode/ourSelectionsUpdates';
1211
import { Register } from './src/register/register';
1312
import { CompositionState } from './src/state/compositionState';
1413
import { globalState } from './src/state/globalState';
@@ -284,34 +283,11 @@ export async function activate(context: vscode.ExtensionContext, handleLocal: bo
284283
return;
285284
}
286285

287-
// Clean up stale `ourSelectionUpdates` that never triggered a selectionChange event, before
288-
// we use them in our logic below
289-
mh.selectionsChanged.ourSelectionsUpdates = cleanupOurSelectionsUpdates(
290-
mh.selectionsChanged.ourSelectionsUpdates,
291-
);
292-
293-
if (e.kind !== vscode.TextEditorSelectionChangeKind.Mouse) {
294-
const selectionsHash = hashSelections(e.selections);
295-
const idx = mh.selectionsChanged.ourSelectionsUpdates.findIndex(
296-
(entry) => entry.selectionsHash === selectionsHash,
297-
);
298-
if (idx > -1) {
299-
mh.selectionsChanged.ourSelectionsUpdates.splice(idx, 1);
300-
Logger.trace(
301-
`Ignoring selection: ${selectionsHash}. ${mh.selectionsChanged.ourSelectionsUpdates.length} left`,
302-
);
303-
return;
304-
} else if (mh.selectionsChanged.ignoreIntermediateSelections) {
305-
Logger.trace(`Ignoring intermediate selection change: ${selectionsHash}`);
306-
return;
307-
} else if (mh.selectionsChanged.ourSelectionsUpdates.length > 0) {
308-
// Some intermediate selection must have slipped in after setting the
309-
// 'ignoreIntermediateSelections' to false. Which means we didn't count
310-
// for it yet, but since we have selections to be ignored then we probably
311-
// wanted this one to be ignored as well.
312-
Logger.warn(`Ignoring slipped selection: ${selectionsHash}`);
313-
return;
314-
}
286+
if (
287+
e.kind !== vscode.TextEditorSelectionChangeKind.Mouse &&
288+
mh.internalSelectionsTracker.shouldIgnoreAsInternalSelectionChangeEvent(e)
289+
) {
290+
return;
315291
}
316292

317293
// We may receive changes from other panels when, having selections in them containing the same file
@@ -419,14 +395,14 @@ export async function activate(context: vscode.ExtensionContext, handleLocal: bo
419395
const mh = await getAndUpdateModeHandler();
420396
if (mh) {
421397
if (compositionState.insertedText) {
422-
mh.selectionsChanged.ignoreIntermediateSelections = true;
398+
mh.internalSelectionsTracker.startIgnoringIntermediateSelections();
423399
await vscode.commands.executeCommand('default:replacePreviousChar', {
424400
text: '',
425401
replaceCharCnt: compositionState.composingText.length,
426402
});
427403
mh.vimState.cursorStopPosition = mh.vimState.editor.selection.active;
428404
mh.vimState.cursorStartPosition = mh.vimState.editor.selection.active;
429-
mh.selectionsChanged.ignoreIntermediateSelections = false;
405+
mh.internalSelectionsTracker.stopIgnoringIntermediateSelections();
430406
}
431407
const text = compositionState.composingText;
432408
await mh.handleMultipleKeyEvents(text.split(''));

‎src/mode/internalSelectionsTracker.ts

Lines changed: 262 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,262 @@
1+
import * as vscode from 'vscode';
2+
3+
import { Logger } from '../util/logger';
4+
import { areSelectionArraysEqual, hashSelectionsArray } from '../util/selections';
5+
6+
/**
7+
* Expiry time for our internal selections updates tracking, after which we can reasonably expect an
8+
* associated VSCode selection change event to have been fired for a given selections update, if one
9+
* ever will be.
10+
*
11+
* We want to be generous enough to allow for delayed VSCode selection change event firings, to
12+
* avoid treating the delayed event as an external update and re-updating our own selections state
13+
* based on an internal update we've already accounted for.
14+
*
15+
* We also want to discard stale internal updates before too long. Until we untrack them, they will
16+
* block incoming selection change events (including external ones) from being handled.
17+
*/
18+
const SELECTIONS_UPDATE_TO_IGNORE_EXPIRY_MS = 1000;
19+
20+
/**
21+
* An internal selections update that we want to track.
22+
*/
23+
export type InternalSelectionsUpdate = {
24+
/** Hash for the updated selections, based on each selection's anchor and active position. */
25+
selectionsHash: string;
26+
/** Timestamp (in ms since Unix epoch) of when this selections update was added to the tracker. */
27+
trackedAt: number;
28+
};
29+
30+
/**
31+
* Class that helps with tracking internal selections updates and controlling / determining whether
32+
* to treat incoming {@link vscode.TextEditorSelectionChangeEvent}s as internal/intermediate
33+
* selections changes to ignore, or as external selections updates that should update our internal
34+
* selections state.
35+
*
36+
* Methods exposed:
37+
*
38+
* - {@link shouldIgnoreAsInternalSelectionChangeEvent} - Determines whether to ignore a given
39+
* VSCode selection change event as an internal/intermediate selections update's change event
40+
*
41+
* - {@link startIgnoringIntermediateSelections} - Start ignoring selection change events while
42+
* running an action
43+
*
44+
* - {@link stopIgnoringIntermediateSelections} - Resume handling selection change events after
45+
* running an action
46+
*
47+
* - {@link maybeTrackSelectionsUpdateToIgnore} - Predicts whether the given selections update will
48+
* trigger a selection change event, and if so, tracks it
49+
*/
50+
export class InternalSelectionsTracker {
51+
// #region Determining whether to ignore a selection change event
52+
53+
/**
54+
* Determines whether or not to view the given VSCode selection change event as being from an
55+
* internal or intermediate selections update, and thus ignore it. Removes its associated
56+
* selections update entry from our tracking, if found.
57+
*
58+
* @returns `true` if the event should be ignored, else `false`.
59+
*/
60+
public shouldIgnoreAsInternalSelectionChangeEvent(
61+
event: vscode.TextEditorSelectionChangeEvent,
62+
): boolean {
63+
// First remove stale tracked updates that we shouldn't worry about anymore
64+
this.cleanupStaleSelectionsUpdatesToIgnore();
65+
66+
const selectionsHash = hashSelectionsArray(event.selections);
67+
return (
68+
this.maybeUntrackSelectionsUpdateToIgnore(selectionsHash) ||
69+
this.shouldIgnoreAsIntermediateSelection(selectionsHash)
70+
);
71+
}
72+
73+
/**
74+
* Looks for an equivalent selections update in our tracked selections updates to ignore, and
75+
* removes it if found.
76+
*
77+
* @returns `true` if the selection was found and removed, else `false`.
78+
*/
79+
private maybeUntrackSelectionsUpdateToIgnore(selectionsHash: string): boolean {
80+
const index = this.selectionsUpdatesToIgnore.findIndex(
81+
(update) => update.selectionsHash === selectionsHash,
82+
);
83+
if (index !== -1) {
84+
this.selectionsUpdatesToIgnore.splice(index, 1);
85+
this.logTrace(
86+
`Ignoring and un-tracking internal selection update's change event ${
87+
selectionsHash
88+
}. Remaining internal selections updates to ignore: ${
89+
this.selectionsUpdatesToIgnore.length
90+
}`,
91+
);
92+
return true;
93+
}
94+
return false;
95+
}
96+
97+
/**
98+
* Determines whether or not to view a VSCode selection change event (whose selections hash wasn't
99+
* found in our tracked selections updates to ignore) as being from an internal action's
100+
* intermediate selection, and thus ignore it.
101+
*
102+
* If `shouldIgnoreIntermediateSelections` is `true`, the answer is an easy yes.
103+
*
104+
* If not, but we are still tracking internal selections updates whose change events haven't fired
105+
* yet, we assume we're just processing intermediate selections' change events that have slipped
106+
* past the end of an action we just finished, and we return `true`. This may sometimes result in
107+
* unintentionally ignoring an external selection made during or closely after a series of
108+
* internal updates, especially if any of those internal updates were misguidedly added to our
109+
* tracking by {@link maybeTrackSelectionsUpdateToIgnore} even though they don't actually trigger
110+
* selection change events. For now, we err on the side of allowing false negatives (missing an
111+
* external update) over false positives (re-applying an internal update), but we may want to
112+
* revisit this tradeoff in the future.
113+
*
114+
* If neither condition is met, we return `false`.
115+
*
116+
* Logic originally implemented in: [VSCodeVim/Vim#5015](https://github.com/VSCodeVim/Vim/pull/5015)
117+
*
118+
* @param untrackedEventSelectionsHash Hashed selections array from a selection change event, that
119+
* wasn't found in our tracked selections updates to ignore
120+
* @returns `true` if we should ignore the event as an intermediate selection, else `false`
121+
*/
122+
private shouldIgnoreAsIntermediateSelection(untrackedEventSelectionsHash: string): boolean {
123+
if (this.shouldIgnoreIntermediateSelections) {
124+
this.logTrace(
125+
`Ignoring intermediate selection change event ${untrackedEventSelectionsHash} while running action`,
126+
);
127+
return true;
128+
}
129+
130+
if (this.selectionsUpdatesToIgnore.length > 0) {
131+
this.logWarn(
132+
`Treating untracked selection change event ${
133+
untrackedEventSelectionsHash
134+
} as an intermediate selection to ignore; assuming it slipped past the end of an action we just ran, since there are still ${
135+
this.selectionsUpdatesToIgnore.length
136+
} tracked internal selections updates to ignore`,
137+
);
138+
return true;
139+
}
140+
141+
return false;
142+
}
143+
144+
// #endregion
145+
146+
// #region Ignoring intermediate selections during an action
147+
148+
/**
149+
* Whether or not we should be ignoring all incoming VSCode selection change events.
150+
*/
151+
private shouldIgnoreIntermediateSelections: boolean = false;
152+
153+
/**
154+
* To be called when starting an action, to flag that we should start ignoring all incoming
155+
* VSCode selection change events until we call `stopIgnoringIntermediateSelections`.
156+
*/
157+
public startIgnoringIntermediateSelections(): void {
158+
this.shouldIgnoreIntermediateSelections = true;
159+
this.logDebug('Now ignoring intermediate selection change events while running action');
160+
}
161+
162+
/**
163+
* To be called after running an action, so we can start handling selection change events again.
164+
*/
165+
public stopIgnoringIntermediateSelections(): void {
166+
this.shouldIgnoreIntermediateSelections = false;
167+
this.logDebug('Resuming handling of selection change events after running action');
168+
}
169+
170+
// #endregion
171+
172+
// #region Tracking internal selections updates
173+
174+
/**
175+
* Array of hashed and timestamped internal selections updates, whose incoming VSCode selection
176+
* change events should be ignored. Each tracked update should be removed when we receive its
177+
* associated selection change event, or cleaned up after an expiry time if its change event never
178+
* gets fired, so that we don't indefinitely block incoming selection change events from updating
179+
* our internal state.
180+
*
181+
* Note that we intentionally use an array, rather than a set or map, to account for validly
182+
* identical selection updates which will each trigger their own vscode selection change events;
183+
* e.g. if we update selections to [1,1][1,1] then [1,2][1,2] and then [1,1][1,1] again.
184+
*/
185+
private selectionsUpdatesToIgnore: InternalSelectionsUpdate[] = [];
186+
187+
/**
188+
* Checks if the selections update will trigger a {@link vscode.TextEditorSelectionChangeEvent},
189+
* and if so, tracks it so we know to ignore its incoming selection change event. Note that VSCode
190+
* only fires a selection change event if the editor's selections actually change in value.
191+
*
192+
* However, it should be noted that our check isn't perfect here, and we might misguidedly track
193+
* an update even if it won't actually trigger a selection change event; hence the need to clean
194+
* up lingering updates from `selectionsUpdatesToIgnore` after an expiry time.
195+
*
196+
* This is because behind the scenes, VSCode seems to quickly reset `editor.selections` after it's
197+
* been set, before properly processing the update and firing a change event. So it's possible for
198+
* `editor.selections` to lag behind what we've set it to in the previous `updateView` call,
199+
* leading us to mistakenly think that the current update will trigger a selection change event,
200+
* even if it won't actually differ from `editor.selections` when VSCode processes this update.
201+
* See: [VSCodeVim/Vim#9644](https://github.com/VSCodeVim/Vim/pull/9644)
202+
*/
203+
public maybeTrackSelectionsUpdateToIgnore({
204+
updatedSelections,
205+
currentEditorSelections,
206+
}: {
207+
updatedSelections: readonly vscode.Selection[];
208+
currentEditorSelections: readonly vscode.Selection[];
209+
}): void {
210+
if (areSelectionArraysEqual(updatedSelections, currentEditorSelections)) {
211+
// VSCode won't fire a selection change event for this update, so there's no need to track it
212+
return;
213+
}
214+
this.trackSelectionsUpdateToIgnore(updatedSelections);
215+
}
216+
217+
private trackSelectionsUpdateToIgnore(updatedSelections: readonly vscode.Selection[]): void {
218+
const selectionsHash = hashSelectionsArray(updatedSelections);
219+
this.selectionsUpdatesToIgnore.push({ selectionsHash, trackedAt: Date.now() });
220+
this.logTrace(
221+
`Tracking ${selectionsHash} as an internal selections update to ignore. Total tracked now: ${this.selectionsUpdatesToIgnore.length}`,
222+
);
223+
}
224+
225+
/**
226+
* Removes stale entries from `selectionsUpdatesToIgnore`. Ideally there shouldn't be any stale
227+
* updates present, and instead every tracked update is removed before expiration when we handle
228+
* its associated selection change event. But if there are, we remove them so that we don't
229+
* indefinitely ignore incoming selection change events under the incorrect assumption that they
230+
* were triggered by internal updates we've already accounted for.
231+
*/
232+
private cleanupStaleSelectionsUpdatesToIgnore(): void {
233+
const now = Date.now();
234+
this.selectionsUpdatesToIgnore = this.selectionsUpdatesToIgnore.filter((entry) => {
235+
const { trackedAt, selectionsHash } = entry;
236+
const age = now - trackedAt;
237+
const hasExpired = age > SELECTIONS_UPDATE_TO_IGNORE_EXPIRY_MS;
238+
if (hasExpired) {
239+
this.logWarn(
240+
`Un-tracking stale internal selections update ${selectionsHash} after ${
241+
age
242+
}ms without a matching selection change event (expiry_ms: ${SELECTIONS_UPDATE_TO_IGNORE_EXPIRY_MS}ms)`,
243+
);
244+
}
245+
return !hasExpired;
246+
});
247+
}
248+
249+
// #endregion
250+
251+
// #region Logging helpers
252+
private logTrace(message: string): void {
253+
Logger.trace(`[InternalSelectionsTracker] ${message}`);
254+
}
255+
private logDebug(message: string): void {
256+
Logger.debug(`[InternalSelectionsTracker] ${message}`);
257+
}
258+
private logWarn(message: string): void {
259+
Logger.warn(`[InternalSelectionsTracker] ${message}`);
260+
}
261+
// #endregion
262+
}

0 commit comments

Comments
 (0)