Oni: "Find All References" produces wrong line number for jumps

[hoschi]

For every entry in the list the line number is one line above the right line number. E.g. the first should be line 3 and not 2:

Should be a zero index issue somewhere :wink:

[CrossR]

https://github.com/onivim/oni/blob/105c707d156438deb85e95724a0df3365845f72b/vim/core/oni-plugin-typescript/src/FindAllReferences.ts#L22-L40

I'm not fully sure what that code does, but it looks to be the culprit.
Add 1 to line and col, do the look up and then remove 1 from both.

Getting rid of all the minus 1s seems to fix it for me though:
Left is with the fix and the cursor is on the very start of Configuration, right is without and its one line above and the col is off.

If I'm not missing something, I can submit the PR for it, but I assume I probably am?

[bryphe]

Thanks for the detailed investigation, @CrossR!

This is confusing because there are 3 entities involved, and each has a different indexing behavior (in other words, uses either _zero-based_ line/columns, or _one-based_ line columns):

• __Language Server Protocol__ - uses _zero-based_ lines/columns
• __TypeScript Standalone Server__ - uses _one-based_ lines/columns
• __Neovim (quickfix)__ - uses _one-based_ lines/columns

So in the code above - what's happening is the request comes in via the _language server protocol_, so we have to convert it to a one-based index for the _typescript server_, and then the _typescript server_ sends us back one-based indices, so then we have to re-map it back to the protocol that the language client expects.

However, once we get those zero-based indices, we hand them off to Neovim in the quickfix window, without adjusting back to one-based indices (which Neovim expects). So it looks like the root issue is here:
https://github.com/onivim/oni/blob/6501a2b50d690ffe31e66aca71a5972c79e3e4eb/browser/src/Services/Language/FindAllReferences.ts#L49

When we're converting from types.Location to the quick-fix item format, we should be converting from zero-based to one-based indices.

And it looks like it is impacting other languages too (like if I 'Find all references' in a .less file):

[CrossR]

Just added a PR for this now finally.
As a side note, is there a second bug in that screenshot?

Oni is highlighting and shows a search for container, but the LSP is doing a search for overlay-container as far as I can tell. Its especially noticeable if you do it on background-image a few lines down whilst highlighting background. Every instance of the word background is highlighted on the line, but the LSP is only searching for background-image. The LSP functionality makes sense, I think the highlighting and search term just need fixing to reflect it too?

It looks to be as simple as updating the getTokenRegex to include -, but that could potentially cause issues with other languages. Is it worth adding language specific options, like how trigger characters work?

[CrossR]

Forgot to stick an update in this issue!
Bug should be fixed now, and the latest release should have this update.

Have you noticed if its working @hoschi ?

[hoschi]

@CrossR still sitting on an old version because of some bugs/missing features, but I should be able to update with the solarized theme now. I put this on my list to check it, when I updated. You can close it in the meantime, if you want.

[bryphe]

Looks like it's fixed now - I'll close it, but feel free to reopen if it's still around. Thanks @CrossR for the fix!

[hoschi]

Ah sorry, forgot that one. It works fine for me, too :+1: