Skip to content
This repository has been archived by the owner on Nov 25, 2021. It is now read-only.

Fix off-by-one highlighting error #360

Closed
wants to merge 3 commits into from
+4 −4
Closed

Fix off-by-one highlighting error #360

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a6614b6
chore: fix comment reference
efritz Apr 1, 2021
edab340
fix: half-closed intervals
efritz Apr 1, 2021
1f4619b
fix: update test values
efritz Apr 1, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/hoverifier.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ describe('Hoverifier', () => {

const delayTime = 100
const hoverRange = { start: { line: 1, character: 2 }, end: { line: 3, character: 4 } }
const hoverRange1Indexed = { start: { line: 2, character: 3 }, end: { line: 4, character: 5 } }
const hoverRange1Indexed = { start: { line: 2, character: 3 }, end: { line: 4, character: 4 } }

scheduler.run(({ cold, expectObservable }) => {
const hoverifier = createHoverifier({
Expand Down
6 changes: 3 additions & 3 deletions src/hoverifier.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -667,8 +667,8 @@ export function createHoverifier<C extends object, D, A>({
*
* Returns `undefined` if the hover result is not successful.
*
* Uses the range specified by the hover result if present, or `position` oherwise,
* which will be expanded into a full token in getTokenAtPosition().
* Uses the range specified by the hover result if present, or `position` otherwise,
* which will be expanded into a full token in getTokenAtPositionOrRange().
*/
const getHighlightedRange = ({
hoverOrError,
Expand All @@ -687,7 +687,7 @@ export function createHoverifier<C extends object, D, A>({
},
end: {
line: hoverOrError.range.end.line + 1,
character: hoverOrError.range.end.character + 1,
character: hoverOrError.range.end.character,
Copy link
Contributor

Choose a reason for hiding this comment

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

It definitely feels wrong to remove the + 1 from the character calculation but not from the line. That can't be right, and it will probably make the code more confusing/harder to debug for the next issue.

The comment above indicates that this transformation is done because the input LSP range is 0-indexed, but other parts of the code are 1-indexed.

                // The result is 0-indexed; the code view is treated as 1-indexed.

The issue here is probably more of a problem with the logic that maps the range to nodes treating the end as inclusive as opposed to exclusive, and the fix would be in that logic, as opposed to here. So something involving getTokenAtPositionOrRange() or getTokenAtPositionOrRange() probably.

BIG DISCLAIMER THAT I'M JUST GUESSING 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@felixfbecker I agree, and I opened this PR to get exactly this type of advice. Who knows this code well enough to be able to formulate an actual solution?

Copy link
Contributor

Choose a reason for hiding this comment

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

@tjkandala when he's back from vacation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bump @tjkandala

Copy link
Contributor

Choose a reason for hiding this comment

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

He's still out of office today :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tjkandala have a good vacay today cuz ur gettin pinged tmr

Copy link
Contributor

Choose a reason for hiding this comment

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

@felixfbecker Looks like you're right (fix in PR #366)

},
}
}
Expand Down