feat: `codeBlockProvider` improvements #716

SCWells72 · 2024-12-20T19:54:27Z

Opening this as a draft PR initially to drive discussion, but I'm hoping it will be promoted to a working PR pretty quickly.

This work was driven by some inconsistent behavior and shortcomings I saw with the LSP formatting range-based code block provider, and I figured that by also using the information from the LSP selection range capability, it should be possible to address most if not all of those issues. In practice, that does actually seem to be the case.

This set of changes also includes an update to derive actual language-specific brace pairs from TextMate (textmate language) or the IDE's abstract file type (TEXT language), but due to a breaking change in the TextMate API in the oldest supported IDE versions, the TextMate aspect is currently commented out. Once support for those oldest versions (the breaking change was made in September, 2023 and it stable in all subsequent IDE versions) has been removed, TextMate support can be restored. Alternatively I could use reflection to support both until there's no longer a need for the older API.

I'll add other details alongside the PR changes themselves.

…selection range and folding block information.

…race pair derivation because of a backward incompatible change in the plugin API in September, 2023. Once LSP4IJ no longer supports that older version, this can be restored. Until this, it will degrade gracefully to use default brace pairs.

SCWells72 · 2024-12-20T19:57:45Z

src/main/java/com/redhat/devtools/lsp4ij/LSPIJTextMateUtils.java

+ * Utilities for working with TextMate files in LSP4IJ.
+ */
+@ApiStatus.Internal
+public final class LSPIJTextMateUtils {


Added a new class for working with TextMate-specific stuff. Right now it's primarily for deriving the language's brace pairs, but as stated in the PR description, I can't do that in a way that works across all supported IDE versions due to a breaking change in TextMateBracePair back in September, 2023:

JetBrains/intellij-community@8df3d04#diff-08fc4fd41510ee4662c41d3f2a671ae2f654d1a2f6ff7608765f427c26eaeae7

Well, I can make it work via reflection, but I'm not sure if it's worth going that far. I guess the options here are:

Leave it like this until the older IDE versions are no longer supported, then uncomment this and it will just work.

Remove this commented code and TextMate support can be reimplemented when incompatible older versions are no longer supported.

Use reflection to support both older and newer versions until older verions are no longer supported.

Thoughts?

SCWells72 · 2024-12-20T19:58:58Z

src/main/java/com/redhat/devtools/lsp4ij/features/codeBlockProvider/LSPCodeBlockProvider.java

+/**
+ * Code block provider that uses information from {@link LSPSelectionRangeSupport} and {@link LSPFoldingRangeBuilder}.
+ */
+public class LSPCodeBlockProvider implements CodeBlockProvider {


Now that this is no longer strictly tied to the folding range feature -- which it still uses, but alongside the selection range feature now -- so I moved it into its own package. Unfortunately there are enough other changes that Git doesn't see it as a move so there's not a good before/after diff here.

SCWells72 · 2024-12-20T19:59:41Z

src/main/java/com/redhat/devtools/lsp4ij/features/codeBlockProvider/LSPCodeBlockProvider.java

+
+        // Try to find it first using the selection ranges which tend to be more accurate; we must use the effective
+        // offset for selection ranges to act as if we're in the adjusted braced block
+        TextRange codeBlockRange = getUsingSelectionRanges(


Selection ranges from the current offset generally provide more detailed code block information, so try finding the closest containing block using that feature first.

SCWells72 · 2024-12-20T20:00:09Z

src/main/java/com/redhat/devtools/lsp4ij/features/codeBlockProvider/LSPCodeBlockProvider.java

+        }
+
+        // Failing that, try to find it using the folding ranges
+        return getUsingFoldingRanges(


If we couldn't find a good code block from the selection ranges, try to find one using the folding ranges.

SCWells72 · 2024-12-20T20:02:49Z

src/main/java/com/redhat/devtools/lsp4ij/features/codeBlockProvider/LSPCodeBlockProvider.java

+                    char endChar = documentChars.charAt(endOffset - 1);
+
+                    // If aligned on an open brace and this ends with the expected close brace, use it
+                    if ((startOffset == openBraceOffset)) {


The first two conditions here are for when the caret starts exactly on an open or close brace(/bracket/paren) and the evaluated range begins/ends on that offset and ends/begins with the appropriate paired brace. Basically these are an exact brace match.

SCWells72 · 2024-12-20T20:03:30Z

src/main/java/com/redhat/devtools/lsp4ij/features/codeBlockProvider/LSPCodeBlockProvider.java

+                        }
+                    }
+                    // Otherwise see if it starts and ends with a known brace pair and we'll find the "closest" below
+                    else if ((openBraceOffset == -1) && (closeBraceOffset == -1) && LSPCodeBlockUtils.isCodeBlockStartChar(file, startChar)) {


Otherwise we see whether or not the selection range starts and ends with a supported brace pair and, if so, we add it to the list of containing code blocks.

SCWells72 · 2024-12-20T20:03:49Z

src/main/java/com/redhat/devtools/lsp4ij/features/codeBlockProvider/LSPCodeBlockProvider.java

+            }
+
+            // Return the closest (i.e., smallest) containing text range
+            if (!ContainerUtil.isEmpty(containingTextRanges)) {


And then we return the smallest containing code block if at least one was found.

SCWells72 · 2024-12-20T20:04:18Z

src/main/java/com/redhat/devtools/lsp4ij/features/codeBlockProvider/LSPCodeBlockProvider.java

+                                                   int offset,
+                                                   @Nullable Character openBraceChar,
+                                                   @Nullable Character closeBraceChar) {
+        List<FoldingRange> foldingRanges = LSPFoldingRangeBuilder.getFoldingRanges(file);


This has been updated to behave similar to how we process selection ranges.

SCWells72 · 2024-12-20T20:06:37Z

src/main/java/com/redhat/devtools/lsp4ij/features/codeBlockProvider/LSPCodeBlockUtils.java

+ * Utilities for deriving information about code blocks.
+ */
+@ApiStatus.Internal
+public final class LSPCodeBlockUtils {


I derived another utility class specifically for code block-related utility methods such as isCodeBlockStart/EndChar() and getCodeBlockStart/EndChar(). This is also what tries to use the file type-specific information to derive supported brace pairs. It works for both TextMate files and plain text files backed by AbstractFileType information, but as stated a few times already, the TextMate aspect is currently commented out due to a breaking API change across the range of supported IDE versions.

SCWells72 · 2024-12-20T20:07:22Z

src/main/java/com/redhat/devtools/lsp4ij/features/codeBlockProvider/LSPCodeBlockUtils.java

+            Map<Character, Character> bracePairs = file instanceof TextMateFile ? LSPIJTextMateUtils.getBracePairs(file) :
+                    file.getFileType() instanceof AbstractFileType ? getAbstractFileTypeBracePairs(file) :
+                            null;
+            if (bracePairs == null) {


Degrade gracefully to the previous behavior where we assumed that the brace pairs were the defaults of curly braces, square brackets, and parens.

SCWells72 · 2024-12-20T20:08:11Z

src/main/java/com/redhat/devtools/lsp4ij/features/codeBlockProvider/LSPCodeBlockUtils.java

+
+    @NotNull
+    private static Map<Character, Character> getBracePairsFwd(@NotNull PsiFile file) {
+        return CachedValuesManager.getCachedValue(file, () -> {


I've confirmed that the caching here is properly evicted if cached files' file types are changed, e.g., from TextMate to plain text or vice-versa.

SCWells72 · 2024-12-20T20:08:42Z

src/main/java/com/redhat/devtools/lsp4ij/features/codeBlockProvider/LSPCodeBlockUtils.java

+    }
+
+    @Nullable
+    private static Map<Character, Character> getAbstractFileTypeBracePairs(@NotNull PsiFile file) {


This is how supported brace pairs are derived for plain text files backed by AbstractFileTypes.

SCWells72 · 2024-12-20T20:09:21Z

src/main/java/com/redhat/devtools/lsp4ij/features/foldingRange/LSPCodeBlockProvider.java

This was moved to the codeBlockProvider package and was already described above.

SCWells72 · 2024-12-20T20:09:38Z

src/main/java/com/redhat/devtools/lsp4ij/features/foldingRange/LSPFoldingRangeBuilder.java

@@ -49,20 +51,11 @@ public class LSPFoldingRangeBuilder extends CustomFoldingBuilder {

    private static final Logger LOGGER = LoggerFactory.getLogger(LSPFoldingRangeBuilder.class);

-    // NOTE: JetBrains has maintained a long assumption that these are the primary structural block delimiters via


Moved into LSPCodeBlockUtils.

SCWells72 · 2024-12-20T20:10:03Z

src/main/java/com/redhat/devtools/lsp4ij/features/foldingRange/LSPFoldingRangeBuilder.java

@@ -72,7 +65,7 @@ protected void buildLanguageFoldRegions(@NotNull List<FoldingDescriptor> descrip
        List<FoldingRange> foldingRanges = getFoldingRanges(file);
        if (!ContainerUtil.isEmpty(foldingRanges)) {
            for (FoldingRange foldingRange : foldingRanges) {
-                TextRange textRange = getTextRange(foldingRange, document);
+                TextRange textRange = getTextRange(foldingRange, file, document);


We need to pass the PSI file so that we can derive file type-specific brace pairs.

SCWells72 · 2024-12-20T20:10:58Z

src/main/java/com/redhat/devtools/lsp4ij/features/foldingRange/LSPFoldingRangeBuilder.java

@@ -86,7 +79,8 @@ protected void buildLanguageFoldRegions(@NotNull List<FoldingDescriptor> descrip
    }

    @NotNull
-    static List<FoldingRange> getFoldingRanges(@Nullable PsiFile file) {
+    @ApiStatus.Internal


I guess I haven't mentioned this already, but because things are now spread out more across different packages, I've added @ApiStatus.Internal to pretty much anything that has to be public but wasn't already part of the published plugin API. Please let me know if you see anything that's become public that doesn't have that annotation.

SCWells72 · 2024-12-20T20:11:20Z

src/main/java/com/redhat/devtools/lsp4ij/features/foldingRange/LSPFoldingRangeBuilder.java

@@ -176,24 +175,6 @@ static TextRange getTextRange(
        return textRange;
    }

-    static boolean isOpenBraceChar(@Nullable Character character) {


These were all moved into LSPCodeBlockUtils.

SCWells72 · 2024-12-20T20:11:53Z

src/main/java/com/redhat/devtools/lsp4ij/features/foldingRange/LSPFoldingRangeBuilder.java

@@ -153,9 +152,9 @@ static TextRange getTextRange(
        Character startChar = start > 0 ? documentChars.charAt(start - 1) : null;
        if ((startChar != null) && ((openBraceChar == null) || (startChar == openBraceChar))) {
            // If necessary, infer the braces for this block
-            if ((openBraceChar == null) && isOpenBraceChar(startChar)) {
+            if ((openBraceChar == null) && LSPCodeBlockUtils.isCodeBlockStartChar(file, startChar)) {


These were also renamed to have less "brace"-specific naming.

SCWells72 · 2024-12-20T20:12:29Z

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

                textRanges.addAll(expandToWholeLinesWithBlanks(editorText, parentSelectionTextRange));
            }
        }
        return new ArrayList<>(textRanges);
    }
-


Moved into LSPSelectionRangeSupport so that it can be used by LSPCodeBlockProvider.

SCWells72 · 2024-12-20T20:13:14Z

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

    private Integer previousOffset;

    public LSPSelectionRangeSupport(@NotNull PsiFile file) {
        super(file);
    }

+    @NotNull
+    @ApiStatus.Internal
+    public static List<SelectionRange> getSelectionRanges(@NotNull PsiFile file,


These were moved from LSPExtendWordSelectionHandler.

...t/java/com/redhat/devtools/lsp4ij/features/foldingRange/TypeScriptCodeBlockProviderTest.java

SCWells72 · 2024-12-20T20:15:00Z

src/test/java/com/redhat/devtools/lsp4ij/fixtures/LSPCodeBlockProviderFixtureTestCase.java

        List<FoldingRange> mockFoldingRanges = JSONUtils.getLsp4jGson().fromJson(mockFoldingRangesJson, new TypeToken<List<FoldingRange>>() {
        }.getType());
        MockLanguageServer.INSTANCE.setFoldingRanges(mockFoldingRanges);

+        List<SelectionRange> mockSelectionRanges = JSONUtils.getLsp4jGson().fromJson(mockSelectionRangesJson, new TypeToken<List<SelectionRange>>() {


Use the mock selection ranges.

…screte test cases.

SCWells72 · 2024-12-20T22:55:18Z

src/main/java/com/redhat/devtools/lsp4ij/features/codeBlockProvider/LSPCodeBlockProvider.java

+                    int startOffset = Math.max(0, textRange.getStartOffset() - 1);
+                    int endOffset = Math.min(documentLength - 1, textRange.getEndOffset());
+                    // These ranges can tend to add whitespace at the end, so trim that before looking for braces
+                    while ((endOffset > startOffset) && Character.isWhitespace(documentChars.charAt(endOffset))) {


We sometimes have to perform a pseudo-"trim" on the tail of the folding range to find the closing brace.

SCWells72 · 2024-12-21T14:38:41Z

...t/java/com/redhat/devtools/lsp4ij/features/foldingRange/TypeScriptCodeBlockProviderTest.java

This ends up being a big diff because I had to add mock selection range responses and I added considerably more test cases for more complex code blocks. I also broke each use case out into its own test method. I'd recommend just taking a look at the test class itself in the IDE instead of here.

…anges pretty much exactly as it does for languages with native support, at least with regard to the behavior when the action is activated while the caret is at the beginning of a line.

…native languages when starting for a full line.

SCWells72 added 4 commits December 20, 2024 11:13

This is a much more comprehensive code block provider that uses both …

b0aada5

…selection range and folding block information.

Brace pairs for TextMate files are now derived from TextMate info.

ed10074

And now it can derive supported brace pairs for abstract file types.

88d1830

openshift-ci bot added the do-not-merge/work-in-progress label Dec 20, 2024

SCWells72 commented Dec 20, 2024

View reviewed changes

...t/java/com/redhat/devtools/lsp4ij/features/foldingRange/TypeScriptCodeBlockProviderTest.java Outdated Show resolved Hide resolved

SCWells72 commented Dec 20, 2024

View reviewed changes

Fixed several boundary conditions and split the unit tests up into di…

7a32498

…screte test cases.

SCWells72 commented Dec 20, 2024

View reviewed changes

SCWells72 marked this pull request as ready for review December 21, 2024 14:29

openshift-ci bot removed the do-not-merge/work-in-progress label Dec 21, 2024

SCWells72 commented Dec 21, 2024

View reviewed changes

SCWells72 added 2 commits December 23, 2024 11:16

This makes the Extend Selection action work for LSP-based selection r…

4611f53

…anges pretty much exactly as it does for languages with native support, at least with regard to the behavior when the action is activated while the caret is at the beginning of a line.

This is perhaps a bit more faithful to how Extend Selection works in …

a8cf54c

…native languages when starting for a full line.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: `codeBlockProvider` improvements #716

feat: `codeBlockProvider` improvements #716

SCWells72 commented Dec 20, 2024

SCWells72 Dec 20, 2024

SCWells72 Dec 20, 2024 •

edited

Loading

SCWells72 Dec 20, 2024

SCWells72 Dec 20, 2024

SCWells72 Dec 20, 2024

SCWells72 Dec 20, 2024

SCWells72 Dec 20, 2024

SCWells72 Dec 20, 2024 •

edited

Loading

SCWells72 Dec 20, 2024 •

edited

Loading

SCWells72 Dec 20, 2024 •

edited

Loading

SCWells72 Dec 20, 2024

SCWells72 Dec 20, 2024

SCWells72 Dec 20, 2024

SCWells72 Dec 20, 2024

SCWells72 Dec 20, 2024

SCWells72 Dec 20, 2024 •

edited

Loading

SCWells72 Dec 20, 2024 •

edited

Loading

SCWells72 Dec 20, 2024 •

edited

Loading

SCWells72 Dec 20, 2024

SCWells72 Dec 20, 2024 •

edited

Loading

SCWells72 Dec 20, 2024

SCWells72 Dec 20, 2024

SCWells72 Dec 21, 2024 •

edited

Loading

		@@ -49,20 +51,11 @@ public class LSPFoldingRangeBuilder extends CustomFoldingBuilder {

		private static final Logger LOGGER = LoggerFactory.getLogger(LSPFoldingRangeBuilder.class);

		// NOTE: JetBrains has maintained a long assumption that these are the primary structural block delimiters via

feat: codeBlockProvider improvements #716

Are you sure you want to change the base?

feat: codeBlockProvider improvements #716

Conversation

SCWells72 commented Dec 20, 2024

Choose a reason for hiding this comment

SCWells72 Dec 20, 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 Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

SCWells72 Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

SCWells72 Dec 20, 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 Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

SCWells72 Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

SCWells72 Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SCWells72 Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SCWells72 Dec 21, 2024 • edited Loading

Choose a reason for hiding this comment

feat: `codeBlockProvider` improvements #716

feat: `codeBlockProvider` improvements #716

SCWells72 Dec 20, 2024 •

edited

Loading

SCWells72 Dec 20, 2024 •

edited

Loading

SCWells72 Dec 20, 2024 •

edited

Loading

SCWells72 Dec 20, 2024 •

edited

Loading

SCWells72 Dec 20, 2024 •

edited

Loading

SCWells72 Dec 20, 2024 •

edited

Loading

SCWells72 Dec 20, 2024 •

edited

Loading

SCWells72 Dec 20, 2024 •

edited

Loading

SCWells72 Dec 21, 2024 •

edited

Loading