godotengine / godot-vscode-plugin

Godot development tools for VSCode
MIT License
1.53k stars 151 forks source link

Add "Open Type Documentation" context menu option #405

Closed DaelonSuzuka closed 2 years ago

DaelonSuzuka commented 2 years ago

This adds an editor context menu option to open the symbol docs for the selected word, or the word under the cursor.

Open questions:

https://user-images.githubusercontent.com/18042232/184232546-99a089d5-ca62-4ece-a291-b4c87293a4bf.mp4

Special thanks to @tomwyr, since this builds directly on his PR that fixes the doc viewer behavior.

tomwyr commented 2 years ago

General question: isn't it what Go to Declaration context menu option already does via LSP?

If I saw Open Symbol Documentation command in an IDE, I'd expect it to open docs for any symbol (type, method, property) so maybe either:

DaelonSuzuka commented 2 years ago

isn't it what Go to Declaration context menu option already does

I have only seen Go to Declaration and Go to Definition jump to source code. I think I'd be very surprised if either one opened up a hyperlink-filled doc page.

I'd expect it to open docs for any symbol (type, method, property)

Very good point. This current implementation for types was very straightforward (once your PR helped me understand how the docs are triggered, so thanks again). Implementing this for methods and properties will be harder[1] because I don't fully understand the language server yet, and it's a very difficult system to just 'poke around' in.

I think the best course of action for now is to pick a narrower name, like Open Type Documentation, and expand the feature later.

[1]: I can expand on why I think this is, if anybody is interested.

DaelonSuzuka commented 2 years ago

I went ahead and renamed it Godot Tools: Open Type Documentation for M A X I M U M C L A R I T Y.

I also learned how to create custom when clause contexts, so now there's a godotTools.connectedToEditor context that can be used to automatically enable/disable various features simply by adding "when": "godotTools.connectedToEditor" to an item in the contributes property in package.json.

This feature could be used to control v3 or v4 specific menu options or views, if we end up with any of those.

tomwyr commented 2 years ago

I can expand on why I think this is, if anybody is interested.

If you don't mind sharing, I'd be interested. LSP is something I'd like to familiarize myself with better.

I also learned how to create custom when clause contexts

I didn't know you could that, sounds great!

RedMser commented 2 years ago

I have only seen Go to Declaration and Go to Definition jump to source code.

Ctrl+clicking on a symbol in Godot's internal script editor opens the documentation.

Also in VSCode, when ctrl+clicking on a symbol from a JS library that has typings, it opens the corresponding .d.ts which sort of acts like documentation, not the .js source.

I don't know how the ctrl+click logic is implemented on the LSP/extension-level though, or if opening this documentation page is even possible for this case.

DaelonSuzuka commented 2 years ago

Ctrl+clicking on a symbol in Godot's internal script editor opens the documentation.

Also in VSCode, when ctrl+clicking on a symbol from a JS library that has typings, it opens the corresponding .d.ts which sort of acts like documentation, not the .js source.

Thanks for the alternative perspective. Most of my experience is in C, where a Declaration and a Definition are both actual language features that are explicitly inside the source code.

I don't know how the ctrl+click logic is implemented on the LSP/extension-level though, or if opening this documentation page is even possible for this case.

It's definitely possible to register our own "provider" for this behavior, but it's yet another subsystem I haven't learned how to use yet.

Maybe in the future I'll make the declaration jump to the source code and the definition show the docs, or some similar combination.

DaelonSuzuka commented 2 years ago

If you don't mind sharing, I'd be interested. LSP is something I'd like to familiarize myself with better.

Okay, so it looks like opening the docs involves actively communicating with the language server by sending a message containing the native_class and the symbol_name. The language server responds with the text of the documentation, which then triggers the doc display behavior. The problem here is that you have to send the class name and the symbol name, which means you already have to know those things.

This is why it'll work for type names, but methods and variables are more complicated. If we want to look up the documentation for a variable, we'll first have to figure out what type the variable is, then ask for the docs for that type. A method is usually called from an instance of a class, IE on a variable, so for a method we have to identify the associated variable, then look up the variable, then pull the docs.

I would like to assume that the language server is actually capable of this, but I don't yet know the magic words to get it to return this information.

I just did a quick test of watching the messages used to populate the hover popups, and unfortunately, when you hover over a variable, the language server responds with {"id":xx,"jsonrpc":"2.0","result":{"contents":[]}}, as if it's not even trying. Adding type annotations doesn't seem to help.

@tomwyr if you wanted to investigate this together sometime, I'd be down. My discord is in my github profile, if you use that.

Edit: Found issues #205 and https://github.com/godotengine/godot/issues/43188 that are relevant to future development of this feature.

DaelonSuzuka commented 2 years ago

This pull request uses spaces for indentation, but it seems that the rest of the files use tabs for indentation. This should be harmonized before the PR is merged.

I have no idea how that happened... I don't think I changed any vscode settings and I'm pretty sure my previous stuff had the correct indentation.

I'm gonna see if I can configure prettier on a precommit hook or something, and maybe write a test for the CI so the machine will yell at me right away.

Thanks for catching my use of let instead of const, too.