Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(highlight): fix eol highlight not displaying #1988

Merged
merged 3 commits into from
May 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
40 changes: 40 additions & 0 deletions src/test/unit/highlights/highlight_grid.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -630,6 +630,46 @@ describe("processHighlightCellsEvent", () => {
},
],
},
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for carrying this over. If you would be so kind, would you mind adding a case for where the Math.max will trigger return 0, when calculating eolCol?

{
testName: "virtual highlights can exceed line length",
events: [
{
row: 2,
vimCol: 6,
validCells: [{ hlId: 2, text: "!" }],
lineText: "hello",
tabSize: 4,
},
],
expectedRanges: [
{
textType: "virtual" as const,
highlights: [{ hlId: 2, text: "!", virtText: "!" }],
line: 12,
col: 6,
},
],
},
{
testName: "calculateEditorColFromVimScreenCol eolCol >= 0",
events: [
{
row: 2,
vimCol: 3,
validCells: [{ hlId: 2, text: "!" }],
lineText: "\t你",
tabSize: 2,
},
],
expectedRanges: [
{
textType: "virtual" as const,
highlights: [{ hlId: 2, text: "!", virtText: "!" }],
line: 12,
col: 1,
},
],
},
].forEach(
({
testName,
Expand Down
17 changes: 16 additions & 1 deletion src/utils/text.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,14 @@ export function expandTabs(line: string, tabWidth: number): string {
return expanded;
}

/**
* Note: This function is for use by the highlight module only.
*/
export function calculateEditorColFromVimScreenCol(line: string, screenCol: number, tabSize: number): number {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

off topic: this could be named getColFromNvimCol without any loss of clarity, while reducing verbosity.

if (screenCol === 0 || !line) {
return 0;
}

let currentCharIdx = 0;
let currentVimCol = 0;
while (currentVimCol < screenCol) {
Expand All @@ -33,7 +37,18 @@ export function calculateEditorColFromVimScreenCol(line: string, screenCol: numb
}

if (currentCharIdx >= line.length) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ollien Here we cannot use width; what we need is character index

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch. I had a test failure on one of the double-wide unit tests but must have fixed it otherwise

return currentCharIdx;
// The basic logic of highlighting is to store them according
// to the editor column. For EOL highlights, the end-of-line position
// is always used as the decoration position, use CSS
// position offset for display.

// Typically, the starting vim column in the grid_line event only exceeds the
// line text length when there are EOL highlights, so the offset
// needs to be added directly here. Otherwise, the parts exceeding the
// line text length will use the same editor column, causing EOL highlights
// to be incorrectly covered.
const eolCol = Math.max(screenCol - currentVimCol, 0);
return currentCharIdx + eolCol;
}
}
return currentCharIdx;
Expand Down