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 missing import on paste #2795

Closed

Conversation

CsCherrYY
Copy link
Contributor

@CsCherrYY CsCherrYY commented Nov 11, 2022

Signed-off-by: Shi Chen chenshi@microsoft.com

paste.mp4

related to eclipse-jdtls/eclipse.jdt.ls#2320
this PR requests VS Code proposed API (documentPaste).

Signed-off-by: Shi Chen <chenshi@microsoft.com>
@CsCherrYY
Copy link
Contributor Author

cc @fbricon @rgrunber , since we also have another PR using proposed paste API: #2703, will it be OK if we publish with the proposed API? (to be specific, for Theia compatibility)

@CsCherrYY CsCherrYY mentioned this pull request Nov 11, 2022
4 tasks
@rgrunber rgrunber requested a review from datho7561 November 11, 2022 22:06
@rgrunber
Copy link
Member

I think @datho7561 confirmed the extension will install without issue, so there should be no compatibility problems. I think we just need to confirm that enableProposedApi also doesn't affect anything on Theia.

I've added David as reviewer since he recently worked with the API. It would be great if both changes took a similar approach.

@datho7561
Copy link
Contributor

One thing that I want to mention is that, at least from my testing, the copy/paste handlers only work on Insiders. I tried setting the launch flag to allow vscode-java to use them in regular VS Code, but this just silenced the warning about using the API and didn't allow the handler to work. Please let me know if you have found a way to use the copy/paste handler in regular VS Code.

@rgrunber
Copy link
Member

rgrunber commented Nov 14, 2022

One thing that I want to mention is that, at least from my testing, the copy/paste handlers only work on Insiders. I tried setting the launch flag to allow vscode-java to use them in regular VS Code, but this just silenced the warning about using the API and didn't allow the handler to work. Please let me know if you have found a way to use the copy/paste handler in regular VS Code.

@CsCherrYY , so I think we should be fine in Theia. Before we request an exception, is there a way to test the proposed API in a released VS Code instance ? How does one simulate the exception on a local instance and test with a locally created vsix ? We can test in VS Code Insiders, but according to @datho7561 , there are issues getting it to run in a released version.

I would have thought if it can work in a released version, then the process to test should be the same as Insiders.

@rgrunber
Copy link
Member

rgrunber commented Nov 25, 2022

I think there's going to be a bit of overlap between this change and #2703 . What about cases where you have a snippet that has both missing imports and a string literal to be escaped ? (can't happen, pasting into strings would never overlap with imports). If both your providers are triggered independently, I'm not sure how they would combine that change. Do they all run in sequence or are they only given the original paste text ?

Seems like we should look to register a single provider and have the (currently 2) features be called on the resulting text. If both changes end up calling the language server, it might also reduce the number of requests/responses.

@CsCherrYY CsCherrYY marked this pull request as ready for review December 20, 2022 01:52
@CsCherrYY
Copy link
Contributor Author

since #2703, nothing we need to do for adding imports on paste at the client side, the server side implementation can be found at eclipse-jdtls/eclipse.jdt.ls#2320.

@CsCherrYY CsCherrYY closed this Jan 6, 2023
@CsCherrYY CsCherrYY deleted the cs-paste-imports branch January 6, 2023 06:48
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.

3 participants