bug: hlsearch is persistent in places text no longer exists, or never was

[ollien]

Check the following:

• I have checked the behavior after setting "vscode-neovim.neovimClean" in VSCode settings.
• I have read the vscode-neovim docs.
• I have searched existing issues.

Neovim version (nvim -v)

v0.9.5

Operating system/version

Fedora 39

Describe the bug

When I use hlsearch, sometimes the "highlights" stick around in places the text no longer exists, similar to #1555. I'm not sure when it originated, it's been a problem for as long as I've used the plugin, but I've never quite pinned it down until now (it felt "random")

I've attached a video of a repro, but it's definitely not consistent in how it fails. In this case, you'll notice that once I hit J to join the lines, the I from Insert is duplicated. You'll also notice that on line 69, an extra Insert is overlaid the await.

simplescreenrecorder-2024-04-14_14.49.49.mp4

Steps To Reproduce

1. Search for some text
2. Edit text around the vicinity of the highlight, such that the highlighted portion moves,
3. After some number of tries, the highlighted portion is placed in a strange location

Expected Behavior

The highlight should move with the remaining text, if the text is still a match, just like in the commandline nvim

[ollien]

FWIW, I am interested in working to solve this bug, so if you have any pointers as to where I could look, it would be appreciated! Otherwise, I'm going to do some digging of my own.

[justinmk]

#1555 looks related. It was closed by #1575 , so looking at that code would be a place to start.

[ollien]

I've been staring at this for a little while, and I'm having trouble capturing the precise nature of what's going on, but I do think I have some kind of idea, and I wanted to share my findings.

In order to calculate offsets, the highlight manager requests details about the current viewport, which implies that these grid_line events are dependent on other viewport events.

vscode-neovim/src/highlight_manager.ts

Line 51 in f7f8938

const gridOffset = this.main.viewportManager.getGridOffset(grid);

When a batch of redraws is received, the extension broadcasts all the redraw events to the various Manager listeners

vscode-neovim/src/main_controller.ts

Lines 301 to 315 in f7f8938

	const batch = [...this.currentRedrawBatch.splice(0), ...redrawEvents];
	const eventData = batch.map(
	(b) =>
	({
	name: b[0],
	args: b.slice(1),
	get firstArg() {
	return this.args[0];
	},
	get lastArg() {
	return this.args[this.args.length - 1];
	},
	}) as any,
	);
	eventBus.fire("redraw", eventData);

The nvim docs state that the events must be handled in order.

Events must be handled in-order. Nvim sends a "flush" event when it has completed a redraw of the entire screen (so all windows have a consistent view of buffer state, options, etc.).

Unfortunately, there's no guarantee this will happen here for a given batch of redraws. Consider a redraw event payload which contains a win_viewport, followed by grid_line. If HighlightManager receives this event first, it will handle the grid_line (since it ignores the win_viewport), and later ViewportManager will handle the win_viewport. However, if ViewportManager receives this event first, it will handle the win_viewport, allowing the events to process in order.

I think that VSCode's EventEmitters provide us a guarantee that a fire call will only return once all handlers have returned https://github.com/microsoft/vscode/blob/6f91cdc47ae284c0a6fec5aa597b9cbcf3cd185c/src/vs/base/common/event.ts#L1163-L1195

With this in mind, I applied this patch to fire the redraw events one by one, in order, but it didn't seem to fix the issue.

diff --git a/src/main_controller.ts b/src/main_controller.ts
index 5ed5504..fdec736 100644
--- a/src/main_controller.ts
+++ b/src/main_controller.ts
@@ -299,6 +299,7 @@ export class MainController implements vscode.Disposable {
                 const hasFlush = findLastEvent("flush", events);
                 if (hasFlush) {
                     const batch = [...this.currentRedrawBatch.splice(0), ...redrawEvents];
+                    batch.reverse()
                     const eventData = batch.map(
                         (b) =>
                             ({
@@ -312,7 +313,9 @@ export class MainController implements vscode.Disposable {
                                 },
                             }) as any,
                     );
-                    eventBus.fire("redraw", eventData);
+                    eventData.forEach((item) => {
+                        eventBus.fire("redraw", [item]);
+                    })
                 } else {
                     this.currentRedrawBatch.push(...redrawEvents);
                 }

I'll keep playing with this.

[ollien]

Another interesting case of this: if you press tab after a double-wide character, the highlights are offset. It's weirdly inconsistent, too. Here, you'll notice that as I press tab after searching for he, the he is in the wrong place after the rocket emoji, only. Once I clear it (and force a redraw to make the phantom highlights go away), searching for he results in only the e being highlighted. Part of me wants to believe this is a bug in the highlight calculation code, but the fact that it happens when editing text only implies to me it's another race.

simplescreenrecorder-2024-04-27_22.15.47.mp4

[xiyaowong]

Yes, this is a legacy bug #1449 (comment).

The main challenge now is that we need to convert the drawing information from nvim into vscode decorations, rather than directly rendering(and there are also differences between JavaScript character rules and CSS character rules).

Nvim only sends data for the changes that occur in a line. For example, if there's a multi-width character that occupies 2 cells, we might only receive data for one cell. However, in vscode, we store highlights based on characters, which makes it difficult to calculate.

@justinmk If nvim could provide an option to always send complete highlight information for each line, this problem would become simpler. Of course, we can definitely solve this, but it will require consideration of many edge cases.

[justinmk]

@justinmk If nvim could provide an option to always send complete highlight information for each line, this problem would become simpler.

Are you sure there isn't a way to enable that in :help ui? Or check if it's discussed on https://github.com/neovim/neovim/issues/ , else create a feature request.

[xiyaowong]

#1976 may helps this

[xiyaowong]

Can this issue be reproduced in v1.12.0?

[ollien]

@xiyaowong It's definitely not fully resolved. I need to find a consistent reproducer, but while doing some other work, I managed to make this happen. I had searched Stmt in another tab, tabbed back, and came back to this.

[xiyaowong]

an old issue, the viewport changes while dealing with the highlight.

[ollien]

This has definitely been a lot better as of late. I've seen a few instances where whitespace is highlighted when it shouldn't, but ctrl+L seems to clear them.