feat: Text document selection range support (issue 651) #669

SCWells72 · 2024-12-06T19:12:57Z

This PR integrates the LSP textDocument/selectionRange feature into the IDE's extendWordSelectionHandler EP that provides the Extend/Shrink Selection actions (which are among my absolute favorites in the IDE). Note that it does not yet include unit tests, but I do plan to add them once I know that there's interest in moving forward with it.

Here's an example of it working in TypeScript:

and here it is in CSS:

I'll add a few notes on things that I know will likely warrant further discussion.

…red into the IDE's extendWordSelectionHandler EP.

SCWells72 · 2024-12-06T19:14:41Z

docs/LSPApi.md

@@ -14,6 +14,7 @@ The [LSPClientFeatures](https://github.com/redhat-developer/lsp4ij/blob/main/src
 - [LSP documentSymbol feature](#lsp-documentSymbol-feature)


I recommend that you hide whitespace differences because I ran several of the existing files through the Java formatter against the project's code style and it resulted in minor whitespace changes.

SCWells72 · 2024-12-06T19:17:04Z

src/main/java/com/redhat/devtools/lsp4ij/client/features/LSPClientFeatures.java

@@ -1105,6 +1140,7 @@ public void setServerCapabilities(@NotNull ServerCapabilities serverCapabilities

    @Nullable
    public TextDocumentServerCapabilityRegistry<? extends TextDocumentRegistrationOptions> getCapabilityRegistry(String method) {
+        // TODO: Can these be changed to else/if? Or can this be changed to switch?


I don't know how often this methods gets called, but it looks like it can be changed to either if ... else if ... else if ... or ideally switch (method) to make it more efficient. I'm happy to do that as part of these changes if you'd like.

This method is called when language server is starting and receive some registerCapability (you can see those LSP trace in the LSP console (do a search with registerCapability). If you want update it, let's go.

Thanks so much to have done this horrible change!

src/main/java/com/redhat/devtools/lsp4ij/features/selectionRange/LSPSelectionRangeSupport.java

...ain/java/com/redhat/devtools/lsp4ij/features/selectionRange/LSPBasicWordSelectionFilter.java

src/main/java/com/redhat/devtools/lsp4ij/features/selectionRange/LSPSelectionRangeSupport.java

angelozerr · 2024-12-06T20:00:05Z

Wow again an impressive PR. Let me some times to give yiu detailled review.

angelozerr · 2024-12-06T20:04:47Z

@SCWells72 it would be nice also to update the LSPSupport markdown to enable the selectiinRange and add a detailled info about implementation and a little demo with some expl1nation line shortcut to use.

I could do that if you wish.

SCWells72 · 2024-12-06T20:11:15Z

I'll update LSPSupport.md now. I'll let you add a demo if that's okay.

…ported.

…fset.

SCWells72 · 2024-12-06T21:06:27Z

I've added a demo to LSPSupport.md as well. I used Presentation Assistant to show the default keybindings to make it clear what's happening. I can re-record without those if you'd prefer.

…_support

angelozerr · 2024-12-08T08:24:05Z

...n/java/com/redhat/devtools/lsp4ij/features/selectionRange/LSPExtendWordSelectionHandler.java

+        }
+
+        // Only if textDocument/selectionRange is supported for the file
+        return LSPFileSupport.getSupport(file).getSelectionRangeSupport() != null;


This check is every time not null. You need to check if there is a language server which support selectionrange:

// Check if the file can support the feature return LanguageServiceAccessor.getInstance(project) .hasAny(file.getVirtualFile(), ls -> ls.getClientFeatures().getSelectionRangeFeature().isSelectionRangeSupported(file)));

Please see what I've done in the latest commit. I looked for something that does what the new method isSupported(PsiFile, Predicate<LSPClientFeatures>) is doing but couldn't find anything quite like it. If something does exist, please let me know and I'll migrate to it; if not, perhaps that method should be moved somewhere more common for future needs? If so, where? LanguageServerAccessor? LSP4IJUtils? ???

angelozerr · 2024-12-08T08:25:24Z

...n/java/com/redhat/devtools/lsp4ij/features/selectionRange/LSPExtendWordSelectionHandler.java

+        }
+
+        // textDocument/selectionRanges has been collected correctly, create list of IJ SelectionDescriptor from LSP SelectionRange list
+        return selectionRangesFuture.getNow(null);


It seems method should never return null, replace with:

return selectionRangesFuture.getNow(Colletions.emptyList());

angelozerr · 2024-12-08T19:56:21Z

docs/LSPSupport.md

+### Selection range
+
+[textDocument/selectionRange](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_selectionRange) is implemented with
+the `extendWordSelectionHandler` extension point and used via the IDE's **Extend Selection** and **Shrink Selection** actions.


Please add shortcut sample in () to execute those actions.

Sorry, but what does "in ()" mean? Is there already a list of supported keyboard shortcuts for LSP4IJ-integrated actions?

I wanted to say add (Ctrl+W) / (Ctrl+Shift+W)

…ment.

…sting/better way to perform dry run enablement like I'm doing here. If so, I'll migrate to it; if not, perhaps isSupported() should be moved to a common location.

SCWells72 · 2024-12-09T23:19:18Z

src/main/java/com/redhat/devtools/lsp4ij/client/features/LSPClientFeatures.java

-        if (LSPRequestConstants.TEXT_DOCUMENT_CODE_ACTION.equals(method)) {
-            // register 'textDocument/codeAction' capability
-            return getCodeActionFeature().getCodeActionCapabilityRegistry();
+        if (method == null) {


This is the result of changing this to use switch which should use a jump table instead of having to evaluate each and every condition. Note that the project formatter is responsible for deciding which case labels are single-line vs. multi-line here.

SCWells72 · 2024-12-09T23:20:50Z

...n/java/com/redhat/devtools/lsp4ij/features/selectionRange/LSPExtendWordSelectionHandler.java

+    }
+
+    // TODO: This seems so incredibly boiler-plate that it should already exist somewhere common
+    private static boolean isSupported(@NotNull PsiFile file,


Is there not already something that does this in an encapsulated manner like this? If not, should this be moved into a common location?

The short answer is https://github.com/redhat-developer/lsp4ij/pull/669/files#r1877498632

Here a detailled explanation: when you need to check that an IJ feature is enabled (ex : to enable/disable Implementations menu item, selectionrange like you need for your PR) it MUST be very fast otherwise IJ will freeze.

You must never wait for starting language servers which can takes some times (some language servers can take 15sec to start it), so IJ could freeze during 15 sec.

Using document matcher requires to open a read action. In Ij Quarkus we use that to associate our Quarkus Language server to Java files only when the project have some Java class in the classpath.

Using hasAny is very fast. It checks:

if there is one or more language server which are associated to the file

if there is one language server has thecapability but ONLY when language server is started.

It means that if you execute Ctrl+W and language servers is starting, the action will be disable but I think it is better to loose the Ctrl+W feature and avoid freezing IJ.

… it's only a single token.

angelozerr · 2024-12-10T07:37:56Z

...n/java/com/redhat/devtools/lsp4ij/features/selectionRange/LSPExtendWordSelectionHandler.java

+        }
+
+        // Only if textDocument/selectionRange is supported for the file
+        return isSupported(file, LSPClientFeatures::getSelectionRangeFeature);


Remove your isSupported method with

Project project = file.getProject(); if (project.isDisposed()) { return false; } // Only if textDocument/selectionRange is supported for the file return LanguageServiceAccessor.getInstance(project) .hasAny(file.getVirtualFile(), ls -> ls.getClientFeatures().getSelectionRangeFeature().isSelectionRangeSupported(file));

angelozerr · 2024-12-10T07:45:13Z

...n/java/com/redhat/devtools/lsp4ij/features/selectionRange/LSPExtendWordSelectionHandler.java

+    }
+
+    // TODO: This seems so incredibly boiler-plate that it should already exist somewhere common
+    private static boolean isSupported(@NotNull PsiFile file,


Please remove this method

angelozerr · 2024-12-10T07:46:05Z

...st/java/com/redhat/devtools/lsp4ij/features/selectionRange/TypeScriptSelectionRangeTest.java

+/**
+ * Selection range tests by emulating LSP 'textDocument/selectionRange' responses from the typescript-language-server.
+ */
+public class TypeScriptSelectionRangeTest extends LSPSelectionRangeFixtureTestCase {


Thanks so much to have written tests, that's great!

SCWells72 · 2024-12-10T15:27:38Z

New PR for this work opened as #678.

SCWells72 added 3 commits December 6, 2024 11:44

This is a first working cut at textDocument/selectionRange support wi…

80bc343

…red into the IDE's extendWordSelectionHandler EP.

Improved and nearly complete implementation aside from unit tests.

47c7fe9

Doc updates.

887dbeb

SCWells72 commented Dec 6, 2024

View reviewed changes

src/main/java/com/redhat/devtools/lsp4ij/features/selectionRange/LSPSelectionRangeSupport.java Outdated Show resolved Hide resolved

SCWells72 commented Dec 6, 2024

View reviewed changes

...ain/java/com/redhat/devtools/lsp4ij/features/selectionRange/LSPBasicWordSelectionFilter.java Outdated Show resolved Hide resolved

Turns out this is not needed after all.

34fd879

SCWells72 mentioned this pull request Dec 6, 2024

Add support for textDocument/selectionRange and integrate the resulting parse tree information via the declarationRangeHandler and extendWordSelectionHandler EPs. #651

Closed

angelozerr reviewed Dec 6, 2024

View reviewed changes

src/main/java/com/redhat/devtools/lsp4ij/features/selectionRange/LSPSelectionRangeSupport.java Outdated Show resolved Hide resolved

SCWells72 added 4 commits December 6, 2024 14:13

Updated LSPSupport.md to show that textDocument/selectionRange is sup…

68d5739

…ported.

Added a bit more detail about the implementation.

0dfa8e3

Better implementation of cache retention/read-through based on the of…

0bfbaf5

…fset.

Added a demo for textDocument/selectionRange.

156c785

SCWells72 added 2 commits December 7, 2024 11:51

Merge branch 'redhat-developer:main' into textDocument_selectionRange…

ed2f01c

…_support

Merge branch 'redhat-developer:main' into textDocument_selectionRange…

b797675

…_support

angelozerr mentioned this pull request Dec 8, 2024

Create tests for LSP selection range #675

Closed

angelozerr reviewed Dec 8, 2024

View reviewed changes

angelozerr requested changes Dec 8, 2024

View reviewed changes

SCWells72 added 4 commits December 9, 2024 14:22

Changed this set of mutually-exclusive conditions into a switch state…

5c0ad77

…ment.

PR 669 - Implemented code review feedback. Not sure if there's an exi…

09dcbb4

…sting/better way to perform dry run enablement like I'm doing here. If so, I'll migrate to it; if not, perhaps isSupported() should be moved to a common location.

Improved isSupported().

beaeaa8

Added unit tests for textDocument/selectionRange support.

7effc18

SCWells72 commented Dec 9, 2024

View reviewed changes

SCWells72 mentioned this pull request Dec 9, 2024

feat: Update external references on successful rename via LSP (issue 670) #674

Merged

SCWells72 and others added 2 commits December 9, 2024 19:35

Added one final assertion at the end for shrinking the selection when…

45a16b5

… it's only a single token.

Merge branch 'main' into textDocument_selectionRange_support

b6e89b6

angelozerr reviewed Dec 10, 2024

View reviewed changes

Implemented PR review feedback.

330a858

SCWells72 mentioned this pull request Dec 10, 2024

feat: textDocument/selectionRange support via extendWordSelectionHandler (issue 651) #678

Merged

SCWells72 closed this Dec 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Text document selection range support (issue 651) #669

feat: Text document selection range support (issue 651) #669

SCWells72 commented Dec 6, 2024 •

edited

Loading

SCWells72 Dec 6, 2024 •

edited

Loading

SCWells72 Dec 6, 2024

angelozerr Dec 8, 2024

angelozerr Dec 10, 2024

angelozerr commented Dec 6, 2024

angelozerr commented Dec 6, 2024

SCWells72 commented Dec 6, 2024

SCWells72 commented Dec 6, 2024

angelozerr Dec 8, 2024

SCWells72 Dec 9, 2024

angelozerr Dec 8, 2024

angelozerr Dec 8, 2024

SCWells72 Dec 9, 2024

angelozerr Dec 10, 2024

SCWells72 Dec 9, 2024 •

edited

Loading

SCWells72 Dec 9, 2024

angelozerr Dec 10, 2024

angelozerr Dec 10, 2024

angelozerr Dec 10, 2024

angelozerr Dec 10, 2024

SCWells72 commented Dec 10, 2024

		@@ -14,6 +14,7 @@ The [LSPClientFeatures](https://github.com/redhat-developer/lsp4ij/blob/main/src
		- [LSP documentSymbol feature](#lsp-documentSymbol-feature)

feat: Text document selection range support (issue 651) #669

feat: Text document selection range support (issue 651) #669

Conversation

SCWells72 commented Dec 6, 2024 • edited Loading

SCWells72 Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

angelozerr commented Dec 6, 2024

angelozerr commented Dec 6, 2024

SCWells72 commented Dec 6, 2024

SCWells72 commented Dec 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SCWells72 Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SCWells72 commented Dec 10, 2024

SCWells72 commented Dec 6, 2024 •

edited

Loading

SCWells72 Dec 6, 2024 •

edited

Loading

SCWells72 Dec 9, 2024 •

edited

Loading