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

Add "Open Type Documentation" context menu option #405

Merged
merged 5 commits into from
Aug 22, 2022

Conversation

DaelonSuzuka
Copy link
Collaborator

@DaelonSuzuka DaelonSuzuka commented Aug 11, 2022

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

Open questions:

  • is Open Symbol Documentation the right name?
  • is this the correct location in the context menu?
Code_62VWSKM8sC.mp4

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

@tomwyr
Copy link
Contributor

tomwyr commented Aug 12, 2022

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:

  • Open Type Documentation would be more descriptive, or
  • Open Symbol Documentation if it also opens the documentation panel when called on Godot class members

@DaelonSuzuka
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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
Copy link
Contributor

tomwyr commented Aug 13, 2022

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
Copy link
Contributor

RedMser commented Aug 15, 2022

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 DaelonSuzuka changed the title Add Open Symbol Documentation context menu option Add Open Type Documentation context menu option Aug 17, 2022
@DaelonSuzuka
Copy link
Collaborator Author

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
Copy link
Collaborator Author

DaelonSuzuka commented Aug 17, 2022

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 godotengine/godot#43188 that are relevant to future development of this feature.

src/godot-tools.ts Outdated Show resolved Hide resolved
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

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.

DaelonSuzuka and others added 2 commits August 21, 2022 16:41
Co-authored-by: Hugo Locurcio <hugo.locurcio@hugo.pro>
@DaelonSuzuka
Copy link
Collaborator Author

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.

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Thanks!

@Calinou Calinou changed the title Add Open Type Documentation context menu option Add "Open Type Documentation" context menu option Aug 22, 2022
@Calinou Calinou merged commit e7674c1 into godotengine:master Aug 22, 2022
@DaelonSuzuka DaelonSuzuka deleted the open-docs-menu-option branch August 22, 2022 22:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants