Skip to content

Commit

Permalink
fix: redraw events are processed in order (#1940)
Browse files Browse the repository at this point in the history 
* fix: redraw events are processed in order

* refactor: send redraw events to screen after flush

* refactor: use PendingUpdates in CursorManager

* fix: highlights would not flush to screen

* chore: remove legacy hack for easymotion

This doesn't seem to be reproducible with normal easymotion, and the fork it was built for in #372 doesn't work anymore

* chore: add docblocks to WaitGroup

* chore: clarify usage of WaitGroup in comparison to Promise.all

* chore: replace WaitGroup with off-the-shelf impl

* chore: remove dead code after removal of statusline hack

* docs: clarify async shenanigans in redraw event handling

* fix: don't iterate over redraw events backwards

* chore: fix review comments
  • Loading branch information
@ollien
ollien committed May 9, 2024
1 parent 12e0d8b commit 122eb56
Show file tree
Hide file tree
Showing 13 changed files with 327 additions and 255 deletions.
6 changes: 6 additions & 0 deletions package-lock.json
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2150,6 +2150,7 @@
"webpack-cli": "^5.1.4"
},
"dependencies": {
"@jpwilliams/waitgroup": "^2.1.1",
"async-mutex": "^0.5.0",
"fast-myers-diff": "^3.2.0",
"grapheme-splitter": "^1.0.4",
Expand Down
26 changes: 12 additions & 14 deletions src/buffer_manager.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -222,22 +222,20 @@ export class BufferManager implements Disposable {
return this.externalTextDocuments.has(textDoc);
}

private handleRedraw(data: EventBusData<"redraw">) {
for (const { name, args } of data) {
switch (name) {
case "win_external_pos":
case "win_pos": {
for (const [grid, win] of args) {
this.grids.set(grid, { winId: win.id });
}
break;
private handleRedraw({ name, args }: EventBusData<"redraw">) {
switch (name) {
case "win_external_pos":
case "win_pos": {
for (const [grid, win] of args) {
this.grids.set(grid, { winId: win.id });
}
case "win_close": {
for (const [grid] of args) {
this.grids.delete(grid);
}
break;
break;
}
case "win_close": {
for (const [grid] of args) {
this.grids.delete(grid);
}
break;
}
}
}
Expand Down
84 changes: 41 additions & 43 deletions src/command_line_manager.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,53 +30,51 @@ export class CommandLineManager implements Disposable {
disposeAll(this.disposables);
}

private handleRedraw(data: EventBusData<"redraw">) {
for (const { name, args } of data) {
switch (name) {
case "cmdline_show": {
const [content, _pos, firstc, prompt, _indent, _level] = args[0];
const allContent = content.map(([, str]) => str).join("");
// !note: neovim can send cmdline_hide followed by cmdline_show events
// !since quickpick can be destroyed slightly at later time after handling cmdline_hide we want to create new command line
// !controller and input for every visible cmdline_show event
// !otherwise we may hit cmdline_show when it's being hidden
// as alternative, it's possible to process batch and determine if we need show/hide or just redraw the command_line
// but this won't handle the case when cmdline_show comes in next flush batch (is it possible?)
// btw, easier to just recreate whole command line (and quickpick inside)
if (this.cmdlineTimer) {
clearTimeout(this.cmdlineTimer);
this.cmdlineTimer = undefined;
this.showCmd(allContent, firstc, prompt);
private handleRedraw({ name, args }: EventBusData<"redraw">) {
switch (name) {
case "cmdline_show": {
const [content, _pos, firstc, prompt, _indent, _level] = args[0];
const allContent = content.map(([, str]) => str).join("");
// !note: neovim can send cmdline_hide followed by cmdline_show events
// !since quickpick can be destroyed slightly at later time after handling cmdline_hide we want to create new command line
// !controller and input for every visible cmdline_show event
// !otherwise we may hit cmdline_show when it's being hidden
// as alternative, it's possible to process batch and determine if we need show/hide or just redraw the command_line
// but this won't handle the case when cmdline_show comes in next flush batch (is it possible?)
// btw, easier to just recreate whole command line (and quickpick inside)
if (this.cmdlineTimer) {
clearTimeout(this.cmdlineTimer);
this.cmdlineTimer = undefined;
this.showCmd(allContent, firstc, prompt);
} else {
// if there is initial content and it's not currently displayed then it may come
// from some mapping. to prevent bad UI commandline transition we delay cmdline appearing here
if (allContent !== "" && allContent !== "'<,'>" && !this.commandLine) {
this.cmdlineTimer = setTimeout(() => this.showCmdOnTimer(allContent, firstc, prompt), 200);
} else {
// if there is initial content and it's not currently displayed then it may come
// from some mapping. to prevent bad UI commandline transition we delay cmdline appearing here
if (allContent !== "" && allContent !== "'<,'>" && !this.commandLine) {
this.cmdlineTimer = setTimeout(() => this.showCmdOnTimer(allContent, firstc, prompt), 200);
} else {
this.showCmd(allContent, firstc, prompt);
}
this.showCmd(allContent, firstc, prompt);
}
break;
}
case "wildmenu_show": {
this.commandLine?.setCompletionItems(args[0][0]);
break;
}
case "wildmenu_hide": {
this.commandLine?.setCompletionItems([]);
break;
}
case "cmdline_hide": {
if (this.cmdlineTimer) {
clearTimeout(this.cmdlineTimer);
this.cmdlineTimer = undefined;
} else if (this.commandLine) {
this.commandLine.cancel(true);
this.commandLine.dispose();
this.commandLine = undefined;
}
break;
break;
}
case "wildmenu_show": {
this.commandLine?.setCompletionItems(args[0][0]);
break;
}
case "wildmenu_hide": {
this.commandLine?.setCompletionItems([]);
break;
}
case "cmdline_hide": {
if (this.cmdlineTimer) {
clearTimeout(this.cmdlineTimer);
this.cmdlineTimer = undefined;
} else if (this.commandLine) {
this.commandLine.cancel(true);
this.commandLine.dispose();
this.commandLine = undefined;
}
break;
}
}
}
Expand Down
73 changes: 40 additions & 33 deletions src/cursor_manager.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import {
ManualPromise,
rangesToSelections,
} from "./utils";
import { PendingUpdates } from "./pending_updates";

const logger = createLogger("CursorManager", false);

Expand Down Expand Up @@ -63,9 +64,9 @@ export class CursorManager implements Disposable {
*/
public wantInsertCursorUpdate = true;
/**
* Set of grid that needs to undergo cursor update
* Set of grids that needs to undergo cursor update
*/
private gridCursorUpdates: Set<number> = new Set();
private gridCursorUpdates: PendingUpdates<number> = new PendingUpdates();

private debouncedCursorUpdates: WeakMap<TextEditor, DebouncedFunc<CursorManager["updateCursorPosInEditor"]>> =
new WeakMap();
Expand All @@ -92,9 +93,10 @@ export class CursorManager implements Disposable {
window.onDidChangeVisibleTextEditors(updateCursorStyle),
window.onDidChangeActiveTextEditor(updateCursorStyle),
eventBus.on("redraw", this.handleRedraw, this),
eventBus.on("flush-redraw", this.handleRedrawFlush, this),
eventBus.on("visual-changed", ([winId]) => {
const gridId = this.main.bufferManager.getGridIdForWinId(winId);
if (gridId) this.gridCursorUpdates.add(gridId);
if (gridId) this.gridCursorUpdates.addForceUpdate(gridId);
}),
{
dispose() {
Expand All @@ -112,37 +114,39 @@ export class CursorManager implements Disposable {
);
}

private handleRedraw(data: EventBusData<"redraw">): void {
for (const { name, args } of data) {
switch (name) {
case "grid_cursor_goto": {
args.forEach((arg) => this.gridCursorUpdates.add(arg[0]));
break;
}
// nvim may not send grid_cursor_goto and instead uses grid_scroll along with grid_line
// If we received it we must shift current cursor position by given rows
case "grid_scroll": {
args.forEach((arg) => this.gridCursorUpdates.add(arg[0]));
break;
}
case "mode_info_set": {
args.forEach((arg) =>
arg[1].forEach((mode) => {
if (mode.name && mode.cursor_shape) {
this.cursorModes.set(mode.name, { cursorShape: mode.cursor_shape });
}
}),
);
break;
}
case "mode_change": {
if (this.main.modeManager.isInsertMode) this.wantInsertCursorUpdate = true;
args.forEach((arg) => this.updateCursorStyle(arg[0]));
break;
}
private handleRedraw({ name, args }: EventBusData<"redraw">): void {
switch (name) {
case "grid_cursor_goto": {
args.forEach((arg) => this.gridCursorUpdates.addForceUpdate(arg[0]));
break;
}
// nvim may not send grid_cursor_goto and instead uses grid_scroll along with grid_line
// If we received it we must shift current cursor position by given rows
case "grid_scroll": {
args.forEach((arg) => this.gridCursorUpdates.addForceUpdate(arg[0]));
break;
}
case "mode_info_set": {
args.forEach((arg) =>
arg[1].forEach((mode) => {
if (mode.name && mode.cursor_shape) {
this.cursorModes.set(mode.name, { cursorShape: mode.cursor_shape });
}
}),
);
break;
}
case "mode_change": {
if (this.main.modeManager.isInsertMode) this.wantInsertCursorUpdate = true;
args.forEach((arg) => this.updateCursorStyle(arg[0]));
break;
}
}
}

private handleRedrawFlush(): void {
this.processCursorMoved();
this.gridCursorUpdates.clear();
}

public async waitForCursorUpdate(editor: TextEditor): Promise<unknown> {
Expand Down Expand Up @@ -177,7 +181,11 @@ export class CursorManager implements Disposable {
* Called when cursor update received. Waits for document changes to complete and then updates cursor position in editor.
*/
private processCursorMoved(): void {
for (const gridId of this.gridCursorUpdates) {
for (const [gridId, shouldUpdate] of this.gridCursorUpdates.entries()) {
if (!shouldUpdate()) {
continue;
}

logger.debug(`Received cursor update from neovim, gridId: ${gridId}`);
const editor = this.main.bufferManager.getEditorFromGridId(gridId);
if (!editor) {
Expand All @@ -188,7 +196,6 @@ export class CursorManager implements Disposable {
if (!this.cursorUpdatePromise.has(editor)) this.cursorUpdatePromise.set(editor, new ManualPromise());
this.getDebouncedUpdateCursorPos(editor)(editor, gridId);
}
this.gridCursorUpdates.clear();
}

// !Often, especially with complex multi-command operations, neovim sends multiple cursor updates in multiple batches
Expand Down
8 changes: 3 additions & 5 deletions src/eventBus.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,9 @@ import { Disposable, EventEmitter } from "vscode";
interface IRedrawEventArg<N, A extends unknown[] = []> {
name: N;
args: A["length"] extends 0 ? undefined : A[];
get firstArg(): A;
get lastArg(): A;
}

type RedrawEventArgs = (
type RedrawEventArgs =
| IRedrawEventArg<"win_close", [number]> // ["win_close", grid]
// ["win_external_pos", grid, win]
| IRedrawEventArg<"win_external_pos", [number, neovim.Window]>
Expand Down Expand Up @@ -98,8 +96,7 @@ type RedrawEventArgs = (
// ["mouse_off"]
| IRedrawEventArg<"mouse_off">
| IRedrawEventArg<"wildmenu_show", [string[]]>
| IRedrawEventArg<"wildmenu_hide">
)[];
| IRedrawEventArg<"wildmenu_hide">;
// #endregion

interface BufferInfo {
Expand All @@ -112,6 +109,7 @@ type EventsMapping = {
// nvim
redraw: RedrawEventArgs;
// custom
["flush-redraw"]: [];
["open-file"]: [string, 1 | 0 | "all"];
["external-buffer"]: [BufferInfo, 1 | 0, number];
["window-changed"]: [number];
Expand Down

0 comments on commit 122eb56

Please sign in to comment.