tiny change to Reformat , so that it will be the same user experience as Java in Intellij IDEA

[sonicning]

1. add reformat selection to LSPReformatAction
2. do not pop out show reformat dialog twice

Purpose

1. If user has selection in editor when using lsp reformat, our plugin still reformat the whole file for the user. But in Java, it will only reformat selection code. In order to have the same user experience, I suggest this pull request
2. If user clicked "cancel" after "Show Reformat File Dialog", our plugin will still pop up the dialog again, as in this line

Goals

1. If user has selection in editor when using lsp reformat, our plugin will only reformat selection
2. If user clicked "cancel" after "Show Reformat File Dialog" , the dialog will "cancel" directly.

Approach

Describe how you are implementing the solutions. Include an animated GIF or screenshot if the change affects the UI (email documentation@wso2.com to review all UI text). Include a link to a Markdown file or Google doc if the feature write-up is too long to paste here.

User stories

Summary of user stories addressed by this change>

Release note

Brief description of the new feature or bug fix as it will appear in the release notes

Documentation

Link(s) to product documentation that addresses the changes of this PR. If no doc impact, enter “N/A” plus brief explanation of why there’s no doc impact

Training

Link to the PR for changes to the training content in https://github.com/wso2/WSO2-Training, if applicable

Certification

Type “Sent” when you have provided new/updated certification questions, plus four answers for each question (correct answer highlighted in bold), based on this change. Certification questions/answers should be sent to certification@wso2.com and NOT pasted in this PR. If there is no impact on certification exams, type “N/A” and explain why.

Marketing

Link to drafts of marketing content that will describe and promote this feature, including product page changes, technical articles, blog posts, videos, etc., if applicable

Automation tests

• Unit tests

  Code coverage information

• Integration tests

  Details about the test cases and coverage

Security checks

• Followed secure coding standards in http://wso2.com/technical-reports/wso2-secure-engineering-guidelines? yes/no
• Ran FindSecurityBugs plugin and verified report? yes/no
• Confirmed that this PR doesn't commit any keys, passwords, tokens, usernames, or other secrets? yes/no

Samples

Provide high-level details about the samples related to this feature

Related PRs

List any other related PRs

Migrations (if applicable)

Describe migration steps and platforms on which migration has been tested

Test environment

openjdk version "11.0.3" 2019-04-16
OpenJDK Runtime Environment (build 11.0.3+12-b304.10)
OpenJDK 64-Bit Server VM (build 11.0.3+12-b304.10, mixed mode)

$ ./clangd --version
clangd version 8.0.0 (tags/RELEASE_800/final)

Learning

Describe the research phase and any blog posts, patterns, libraries, or add-ons you used to solve the problem.

[NipunaRanasinghe]

@sonicning Thank you so much for working on this! Your PR looks all good and the there are two things that I'm worrying about

1. IMO if a particular language server does not support partial(range) formatting (we can check this using the received server capabilities), the client should falls back to full file formatting to preserve the user experience.
2. There might be cases where there are "unintentional" selections in the file that the user is not aware of.

So I would like to suggest that its better to show the reformat dialog if any there are any selections in the file so that the user can choose their preferred action. WDYT?

[sonicning]

@NipunaRanasinghe

1. Yes, I haven't thought about that for point 1. I am using clangd as language server, so it is functional. What kind of error will it be when the language server does not support partial formatting ? Will this line of code throw execption ? or just do nothing ?

2. I think we can treat "unintentional" selections as "intentional" selections. As in Intellij IDEA, that is the case for Java. If there is selection, no matter "unintentional" or "intentional", IDEA will reformat selection code.

[rasika]

1. IMO if a particular language server does not support partial(range) formatting (we can check this using the received server capabilities), the client should falls back to full file formatting to preserve the user experience.

If we look at Java experience in IntelliJ, it does nothing when it fails to format. I suppose we can do the same. Here, if the server does not support partial formatting it will be communicated at the initial stage of the LSP protocol via server capabilities. Even after that, if the client sends a partial formatting request we can simply ignore it. As a user, falling back into the full file formatting is not the accepted behavior for me. Rather I would expect do nothing when if fails to format.

2. There might be cases where there are "unintentional" selections in the file that the user is not aware of.

Agree with @sonicning IMO We don't need to add extra steps into formatting. This should be the same as Java partial formatting.

[NipunaRanasinghe]

Yes, I haven't thought about that for point 1. I am using clangd as language server, so it is functional. What kind of error will it be when the language server does not support partial formatting ? Will this line of code throw execption ? or just do nothing ?

If you could have a look at the DefaultRequestManager you can see we are checking whether the connected language server has the particular capability before sending the request. So clangd lang-server won't receive the range formatting request if it doesn't support the partial formatting.

I think we can treat "unintentional" selections as "intentional" selections. As in Intellij IDEA, that is the case for Java. If there is selection, no matter "unintentional" or "intentional", IDEA will reformat selection code.

Yeah that is the default behavior for java in IDEA. What I was saying is that we can add an improvement where if the attached language server does not support partial formatting we can simply notify it to user and ask to format the file only if the user wants. But I think we can think about it after merging this since its not a blocker for now :)

[sonicning]

@NipunaRanasinghe Yes, Good idea, I will look into that when I have time, thanks