Skip to content

Fix code-folding for LSP clients that support line-folding only#7750

Merged
lahodaj merged 1 commit intoapache:masterfrom
sid-srini:code_folding_if_else_vscode_bug
Oct 29, 2024
Merged

Fix code-folding for LSP clients that support line-folding only#7750
lahodaj merged 1 commit intoapache:masterfrom
sid-srini:code_folding_if_else_vscode_bug

Conversation

@sid-srini
Copy link
Copy Markdown
Contributor

@sid-srini sid-srini commented Sep 13, 2024 •
edited
Loading

  1. Added TextDocumentServiceImpl.convertToLineOnlyFolds which:

    • accepts code-folds specified as fine-grained Position-based (line, column) ranges, and,
    • changes them to coarse-grained line-only ranges.
    • The line-only ranges computed uphold the code-folding invariant that: a fold does not end at the same point where another fold starts.

  2. This is used in TextDocumentServiceImpl.foldingRange() when the client capabilities have FoldingRangeClientCapabilities.lineFoldingOnly = true.

Symptoms of this issue maybe seen in oracle/javavscode#249.

@sid-srini sid-srini marked this pull request as draft September 13, 2024 05:42
@sid-srini sid-srini mentioned this pull request Sep 13, 2024
Closed
@lahodaj lahodaj added Java [ci] enable extra Java tests (java.completion, java.source.base, java.hints, refactoring.java, form) LSP [ci] enable Language Server Protocol tests VSCode Extension labels Sep 13, 2024
@lahodaj
Copy link
Copy Markdown
Contributor

lahodaj commented Sep 13, 2024

On a (relatively) quick review, this seems OK to me. But, I would say the convertToLineOnlyFolds method is quite complex, and some tests would be desirable. Round-trip test (like those in ServerTest) would not be bad, but this method could be also tested in a unit-test way. E.g. the method could be package-private and static, and called directly from the test(?)

@sid-srini
Copy link
Copy Markdown
Contributor Author

sid-srini commented Sep 13, 2024

On a (relatively) quick review, this seems OK to me. But, I would say the convertToLineOnlyFolds method is quite complex, and some tests would be desirable. Round-trip test (like those in ServerTest) would not be bad, but this method could be also tested in a unit-test way. E.g. the method could be package-private and static, and called directly from the test(?)

Thanks @lahodaj for taking a quick look at this draft PR. I intended to add the unit tests before making it ready-for-review. :)

@sid-srini
Fix code-folding for LSP clients that support line-folding only
d371a8c 
1. Added `TextDocumentServiceImpl.convertToLineOnlyFolds` which:
    - accepts code-folds specified as fine-grained Position-based (line, column) ranges, and,
    - changes them to coarse-grained line-only ranges.
    - The line-only ranges computed uphold the code-folding invariant that: *a fold **does not end** at the same point **where** another fold **starts**. *

2. This is used in `TextDocumentServiceImpl.foldingRange()` when the client capabilities have `FoldingRangeClientCapabilities.lineFoldingOnly = true`.

Signed-off-by: Siddharth Srinivasan <siddharth.srinivasan@oracle.com>
@sid-srini sid-srini force-pushed the code_folding_if_else_vscode_bug branch from e914d3a to d371a8c Compare September 17, 2024 03:12
@sid-srini sid-srini marked this pull request as ready for review September 17, 2024 03:12
@sid-srini
Copy link
Copy Markdown
Contributor Author

sid-srini commented Sep 23, 2024 •
edited
Loading

I added the unit test for this enhancement. @lahodaj - Please review whenever you get a chance. Thank you.

@sid-srini
Copy link
Copy Markdown
Contributor Author

sid-srini commented Oct 24, 2024

@lahodaj Request you to please facilitate in reviewing this PR. Thank you.

lahodaj
lahodaj approved these changes Oct 25, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@lahodaj lahodaj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks OK to me. Thanks!

@sid-srini
Copy link
Copy Markdown
Contributor Author

sid-srini commented Oct 25, 2024

Looks OK to me. Thanks!

Thank you very much.

@lahodaj
Copy link
Copy Markdown
Contributor

lahodaj commented Oct 25, 2024

Unless there are objections, I'll merge tonight.

@sid-srini sid-srini mentioned this pull request Oct 26, 2024
Merged
sid-srini added a commit to sid-srini/javavscode that referenced this pull request Oct 28, 2024
@sid-srini
[JVSC-249] Fix code folding in VS Code due to line-only folding limit…
077a2a6 
…ation

Added the backport of the Netbeans PR apache/netbeans#7750 patch which allows supporting LSP clients
which have line-only folding support.
- This is the case of VS Code as referenced in microsoft/vscode#50840.

Closes oracle#249

Signed-off-by: Siddharth Srinivasan <siddharth.srinivasan@oracle.com>
@lahodaj lahodaj merged commit 30d34e6 into apache:master Oct 29, 2024
@sid-srini sid-srini deleted the code_folding_if_else_vscode_bug branch October 29, 2024 07:16
@sid-srini sid-srini restored the code_folding_if_else_vscode_bug branch October 29, 2024 07:33
@sid-srini
Copy link
Copy Markdown
Contributor Author

sid-srini commented Oct 29, 2024 •
edited
Loading

The LSP ServerTest. testCodeFolding and ServerTest.testPullUp seem to be failing in the merge tests run due to an NPE since NbCodeClientCapabilities.getClientCapabilities() is null for the unit tests.

I was trying to look to this PR's github workflow actions but could not find it. I am not sure why the github actions didn't run on the PR. I am very sorry for this unit tests breakage.

I will need to add the following fix to TextDocumentServiceImpl

            lineFoldingOnly = client != null
                    && (nbCodeCapabilities = client.getNbCodeCapabilities()) != null
                    && (clientCapabilities = nbCodeCapabilities.getClientCapabilities()) != null
                    && (textDocument = clientCapabilities.getTextDocument()) != null
                    && (foldingRange = textDocument.getFoldingRange()) != null
                    && Objects.equals(foldingRange.getLineFoldingOnly(), Boolean.TRUE);

@lahodaj lahodaj mentioned this pull request Oct 29, 2024
Merged
@sid-srini sid-srini mentioned this pull request Oct 29, 2024
Closed
@lahodaj
Copy link
Copy Markdown
Contributor

lahodaj commented Oct 29, 2024

How about this:
#7921

@sid-srini
Copy link
Copy Markdown
Contributor Author

sid-srini commented Oct 29, 2024

Thank you very much @lahodaj.

@ebarboni ebarboni added this to the NB25 milestone Oct 29, 2024
@mbien
Copy link
Copy Markdown
Member

mbien commented Oct 29, 2024 •
edited
Loading

I am not sure why the github actions didn't run on the PR.

I am also not completely sure what happened here. But it looks like the last change to this branch was about a month ago, and github didn't add the green check mark there. This could mean that the CI-run was waiting to be approved for this PR and nobody pressed the button? But this is just a guess - could be something else.

image

here our PR review / merge check list as reminder:
https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=240884239#PRsandYouAreviewerGuide-PRapprovalchecklist

in particular:

did the right tests run? When did they run?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lahodaj lahodaj lahodaj approved these changes

Assignees

No one assigned

Labels

Java [ci] enable extra Java tests (java.completion, java.source.base, java.hints, refactoring.java, form) LSP [ci] enable Language Server Protocol tests VSCode Extension

Projects

None yet

Milestone

NB25

Development

Successfully merging this pull request may close these issues.

4 participants

@sid-srini @lahodaj @mbien @ebarboni