python-language-server icon indicating copy to clipboard operation
python-language-server copied to clipboard

Extension breaks highlight selected word occurences on scrollbar

Open pohmelie opened this issue 5 years ago • 19 comments

Environment data

vscode

Version: 1.44.2 Commit: ff915844119ce9485abfe8aa9076ec76b5300ddd Date: 2020-04-16T17:50:03.709Z Electron: 7.1.11 Chrome: 78.0.3904.130 Node.js: 12.8.1 V8: 7.8.279.23-electron.0 OS: Linux x64 4.15.0-96-generic

Extension version: 2020.4.74986 OS and version: ubuntu 18.04

Steps to reproduce:

Enable extension, some clicks and scrolls and thing stop working. Disable extension and highlight works just fine.

More info and discussion about reasons is here https://github.com/microsoft/vscode/issues/16042 Looks like many years ago some other lang extensions did have problems with this.

More to say, if I open, lets say a .yml file, then scrollbar highlight works just fine, even when python extension enabled.

pohmelie avatar Apr 22 '20 16:04 pohmelie

Laggy a bit :/

Peek 2020-04-24 14-26

pohmelie avatar Apr 24 '20 11:04 pohmelie

Looks like the analysis in Language server might be blocking the highlighting.

karrtikr avatar Apr 24 '20 14:04 karrtikr

Might be side effect of https://github.com/microsoft/python-language-server/pull/1767

MikhailArkhipov avatar Apr 24 '20 17:04 MikhailArkhipov

Is the issue here that it's slow, or that it's not working at all? I can understand slow, given this now uses the analysis.

jakebailey avatar Apr 27 '20 17:04 jakebailey

It's not working at all.

pohmelie avatar Apr 27 '20 17:04 pohmelie

I guess I'm confused; the screencast shows it working for variables. Are you specifically referring to keywords? Those aren't expected to to work, and only worked before as it was VS Code "guessing" what to highlight without understanding the context.

jakebailey avatar Apr 27 '20 17:04 jakebailey

If that's it, then we can see if the call is returning empty or none; if it's empty than maybe returning a null result when doing an invalid hover will cause VS Code to fallback to its old behavior.

jakebailey avatar Apr 27 '20 17:04 jakebailey

the screencast shows it working for variables

I can film new screencast with restart of vscode after plugin enabled. And nothing will work. I guess it is something like an artifact from working time before.

pohmelie avatar Apr 27 '20 17:04 pohmelie

Peek 2020-04-27 20-45

pohmelie avatar Apr 27 '20 17:04 pohmelie

With diabled plugin:

Peek 2020-04-27 20-47

pohmelie avatar Apr 27 '20 17:04 pohmelie

Note that the lower screencast is just VS Code's builtin matching which doesn't care about context at all and will highlight any matching word. We declare that we support the feature, so it disables itself.

In the first screencast:

  • The keywords are not highlighted, as they are not variables. This is what I referenced before that may have a simple change to tweak.
  • The duplicate functions are arguably an edge case; I don't know what the "right" thing to do here is when we are internally merging function definitions as only one of them is "real". Most people are not declaring the same function over and over.
  • self is being scoped to each function; perhaps it should be highlighted for the entire class's references to self.

But none of these are it "not working".

jakebailey avatar Apr 27 '20 18:04 jakebailey

So, how can I bring back primitive builtin matching and have benefits from extension: autocomplete, linting, go to definition, etc.?

pohmelie avatar Apr 27 '20 18:04 pohmelie

I don't think there's any simple way that's not a code change in the extension or the LS. Feasibly you can disable the extension's LS downloader and manually use an old build, but that is a pretty significant workaround.

To confirm, it's the keywords not being highlighted which is the main issue?

jakebailey avatar Apr 27 '20 18:04 jakebailey

No, the main issue that is nothing highlighted. Take a look at screencasts: async, def, self, pass, start, stop, nothing highlights. I hope there will be a toggle to choose who provides highlights. So I can bring builtin behavior back.

pohmelie avatar Apr 27 '20 18:04 pohmelie

async, def, and pass are all keywords. This is what I said before. They are only highlighted in VS Code because VS Code does not understand Python. It's not clear to me that highlighting a keyword is a good idea.

self, start, and stop are not keywords.

self may be able to be fixed to highlight the same self in all functions, but we will likely want to look at other highlight implementations to see if they also do something similar (like with this) for consistency, but it is being highlighted in each specific function.

Your example of start and stop are special because you've declared the same function multiple times. Arguably we should be highlighting the correct region and not the prior declaration.

jakebailey avatar Apr 27 '20 18:04 jakebailey

I got your point, no need to explain each case exactly. My point is I want behavior as it was before (builtin mode) and I want all other features of extension. Is it possible to make some toggle in options to disable extension highlight rules?

pohmelie avatar Apr 27 '20 19:04 pohmelie

Is it possible to make some toggle in options to disable extension highlight rules?

Not in the language server itself. We can only declare at startup that we do or don't support this feature, we cannot do so dynamically. Any other changes would be a hack in the client (the VS Code extension) to skip certain calls, which is a bad idea.

It's better to improve the feature than to add toggles to disable it.

jakebailey avatar Apr 27 '20 19:04 jakebailey

Hm, I didn't see it in the spec initially, but there is way to dynamically declare support for this. Regardless, we should try and address the issues first, rather than adding toggles we don't plan to keep.

jakebailey avatar Apr 27 '20 19:04 jakebailey

This will need https://github.com/microsoft/python-language-server/pull/1960 since it handles stub references better. It does not seem to be possible to return null or something to get default behavior, so for non-references we'd need to get down to tokens.

MikhailArkhipov avatar May 01 '20 07:05 MikhailArkhipov