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

Resolving bounty version 0.1 #2

Merged
merged 15 commits into from
Nov 15, 2023
Merged

Conversation

digital-phoenix
Copy link
Contributor

This should provide everything you need to resolve issue #1

/claim #1

@algora-pbc algora-pbc bot mentioned this pull request Oct 27, 2023
@olegklimov
Copy link
Contributor

I looked at it 🎉 First, it needs an explanation how it works. Not many people I'm assuming are familiar VS plugins structure.

What MultilineGreyTextProvider does exactly? Who calls it, when and why?

It requires .NET 4.8 or some previous version that it doesn't show, where is the specification of what it requires?

MultilineGreyTextTagger.cs is too big with 500 lines, maybe divide it into several, or explain what's going on.

Why Settings.settings is almost empty?

What m_provider.CompletionBroker.CreateCompletionSession does exactly, why we need it?

The handling of VSConstants.VSStd2KCmdID.RETURN and TAB is not clear. Why it has anything to do with selection?

Second, there has to be a way to see the state of the plugin. For debugging, I didn't find a console that I can print messages to. For user, it's not clear if the plugin is connected correctly or not, or when it requests a completion.

Connection error:

image

Working:

image

@olegklimov
Copy link
Contributor

I've never seen this message, or any other message, even though I never managed to make the plugin actually work:

IMAGE 2023-10-29 09:46:51

Popups are a "heavier" way to inform user of any bad things happening. As a rule of thumb, it should give a popup on a fatal error, for example when it can't start the LSP part correctly. All the less significant errors like lost Wi-Fi signal should just show in the status bar as unplugged socket and contrast (depending on color theme) background.

@olegklimov
Copy link
Contributor

Formatting problems, please @digital-phoenix spend time of cleaning the code! In many cases it can be shorter, cleaner, better formatted.

image

For example here, it's not tabs or spaces problem, as I first thought. It's all spaces and the formatting is just messy like that. Put spaces before "{", decide if you want "{" on a new line in functions, in ifs, and follow that consistently throughout the code!

@olegklimov
Copy link
Contributor

refact-lsp.exe -- well of course it shouldn't be there.

The right way to do it is to create Github Actions script, that will take the latest binary from there https://github.com/smallcloudai/refact-lsp/actions and build an extension that can be actually installed on user computer. It's not hard to do, we already have it in VS Code, and you can talk to @reymondzzzz if you have any difficulties settings this up.

@digital-phoenix
Copy link
Contributor Author

I looked at it 🎉 First, it needs an explanation how it works. Not many people I'm assuming are familiar VS plugins structure.

What MultilineGreyTextProvider does exactly? Who calls it, when and why?

When you say it needs an explanation on how it works are you looking for a document that provides a general outline or comments in the code? A large proportion of the code in this extension is boiler plate code that is better explained by the vs extension documentation. But I can give a general outline of how everything fits together if that's what you want.

MultilineGreyTextProvider is an IViewTaggerProvider which is required to create a tagger and a tagger provides tags. Tags work similarly to html tags like and

by telling the system how to display certain information. In this case it tells the system when to insert empty space after a text line to hold the grey text.

It requires .NET 4.8 or some previous version that it doesn't show, where is the specification of what it requires?

It does require .NET 4.8. The easiest way to find this information would be to load the solution in Visual Studio and double click on Properties and look at the Application section.

MultilineGreyTextTagger.cs is too big with 500 lines, maybe divide it into several, or explain what's going on.

MultilineGreyTextTagger has quite a few comments explaining what is going on but I can add more if there is anything specific that you are confused about.

Why Settings.settings is almost empty?

Settings.settings and the other files under Properties are auto-generated files. The reason the file is almost empty is because the extension stores the user settings using the options page to store settings (instead of settings.settings). Specifically the options page titled "Refact Assistant".

What m_provider.CompletionBroker.CreateCompletionSession does exactly, why we need it?
The handling of VSConstants.VSStd2KCmdID.RETURN and TAB is not clear. Why it has anything to do with selection?

The RefactCompletionCommandHandler handles keyboard input. A large proportion of this code is boiler plate that is required to maintain the functionality of the popups while also triggering the LSP at the appropriate moments. m_provider.CompletionBroker.CreateCompletionSession creates a popup session and displays completion suggestions to the user in the popup. The Return and Tab keys are used to accept a suggestion when a user clicks return/enter or tab the grey text is accepted and added to the file.

Second, there has to be a way to see the state of the plugin. For debugging, I didn't find a console that I can print messages to. For user, it's not clear if the plugin is connected correctly or not, or when it requests a completion.

If you are running the code through visual studio the standard output window in Visual Studio will display all the debug console messages from the extension. If you want to see a more detailed view of the communication between the extension and the lsp server the easiest way to do that would be to comment out the code on line 82 of RefactLanguageClient.cs that says "info.CreateNoWindow = true;" this will cause the lsp to create a new console window with the lsp executable (and all its print messages) whenever the lsp is initiated.

Connection error:
image

Working:
image

I can add an image to display when the server is connected but this is entering scope creep territory for this issue as it is outside of the originally stated requirements. This may be better handled through opening a new issue.

@digital-phoenix
Copy link
Contributor Author

Popups are a "heavier" way to inform user of any bad things happening. As a rule of thumb, it should give a popup on a fatal error, for example when it can't start the LSP part correctly.

This is exactly what this code does. It only displays a popup when the lsp fails to launch.

@digital-phoenix
Copy link
Contributor Author

refact-lsp.exe -- well of course it shouldn't be there.

The right way to do it is to create Github Actions script, that will take the latest binary from there https://github.com/smallcloudai/refact-lsp/actions

I am working on this change now

@digital-phoenix
Copy link
Contributor Author

I've never seen this message, or any other message, even though I never managed to make the plugin actually work:

The two most likely reasons for this are either: the options page does not have a valid API key set and therefore the lsp is failing to accept completion requests. Or none of the code is being triggered. In order to start the lsp you need to run the extension and open a code file to start editing. Can you let me know the file type you are using to test this extension so I can test it on my end?

@digital-phoenix
Copy link
Contributor Author

Formatting problems, please @digital-phoenix spend time of cleaning the code! In many cases it can be shorter, cleaner, better formatted.
image

For example here, it's not tabs or spaces problem, as I first thought. It's all spaces and the formatting is just messy like that. Put spaces before "{", decide if you want "{" on a new line in functions, in ifs, and follow that consistently throughout the code!

Is there a specific style guide you'd like me to follow or is any style fine as long as it's consistent?

@digital-phoenix
Copy link
Contributor Author

digital-phoenix commented Oct 30, 2023

I added a workflow that uploads the refact-lsp.exe so you can manually download the exe from that workflow and replace the existing exe in ./MultilineGreyText/RefactLSP with the new one.

@olegklimov olegklimov merged commit f493dd3 into smallcloudai:main Nov 15, 2023
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.

None yet

2 participants