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

Support Collapsed Text #170447

Open
wants to merge 24 commits into
base: main
Choose a base branch
from
Open

Support Collapsed Text #170447

wants to merge 24 commits into from

Conversation

mtbaqer
Copy link

@mtbaqer mtbaqer commented Jan 3, 2023

Description

Adds VSCode implementation of LSP's collapsed text.
Allows for custom collapsed text instead of the default ..., and the ability to start folding anywhere in the start line not only at the very end. See the demo below.

Demo

Screenshot 2023-01-02 at 15 45 35

Screenshot 2023-01-02 at 15 45 53

Issues

Should make following issues trivial to solve:

Does a lot of heavy lifting for (but still requires some work):

Changes

  • Adds two fields to almost all inner folding interfaces/classes (FoldingRange, FoldingRegion, etc.):

    • collapsedText: text to display
    • startColumn: where the folding should start at the start line. More details here.
  • Changes FoldingDecorations to dynamically construct collapsed decorations to include collapsed text (currently little messy, will make a future refactor PR to reduce the size of this one).

  • Introduces new InlineFoldingRange class to support the custom startColumn. Currently only supports always folding to the end of the line.

  • Adds new hideContent field to ModelDecorationOptions to support InlineFoldingRange.

  • Modifies ModelLineProjection, ModelLineProjectionData and related classes to support inlineFoldingRange.

  • Adds new test cases and modifies current ones for most classes touched.

Note: The current implementation assumes InlineFoldingRange decorations are located in the same InjectedText decoration tree. This can be changed in future updates.

VSCode Language Server

A new PR against vscode-languageserver-node is needed to support providing ranges through the language server and to update the capability.

@mtbaqer
Copy link
Author

mtbaqer commented Jan 3, 2023

@microsoft-github-policy-service agree

@mtbaqer mtbaqer marked this pull request as ready for review January 8, 2023 10:24
@mtbaqer
Copy link
Author

mtbaqer commented Jan 11, 2023

@aeschli any updates on this?

@aeschli aeschli requested a review from alexdima January 12, 2023 10:48
@aeschli
Copy link
Contributor

aeschli commented Jan 12, 2023

Thanks @mtbaqer, this is an impressive change, that will take a while to process and review. We need @alexdima to look at the editor changes, but unfortunately he's out for the month. Or maybe @hediet can check it out?

@hediet hediet self-requested a review January 13, 2023 10:41
@hediet hediet self-assigned this Jan 13, 2023
@hediet hediet added this to the January 2023 milestone Jan 13, 2023
@aeschli aeschli modified the milestones: January 2023, February 2023 Jan 23, 2023
@aeschli aeschli assigned aeschli and unassigned aeschli Feb 20, 2023
@aeschli aeschli modified the milestones: February 2023, March 2023 Feb 20, 2023
@daiyam
Copy link
Contributor

daiyam commented Feb 26, 2023

Is there any updates? This PR is a must have. Thanks @mtbaqer

@kieferrm kieferrm mentioned this pull request Mar 4, 2023
91 tasks
@hediet
Copy link
Member

hediet commented Mar 9, 2023

I had a look at the PR now.

I am very impressed by the work and I can see that it took a lot of effort to implement this feature!

However, because this part of the editor is so complicated, we have to be very careful with how such features are implemented.
I think we should split up this feature into a proper implementation of the hideContent flag for decorations and then the folding feature on top of that.

The implementation of the hideContent flag in this PR misses some edge cases and doesn't properly implement the flag, as it only supports hiding content from a given offset until the end of the line.

I think the hideContent flag and injected text together should be implemented as view edits to simplify the implementation. A single view edit would replace a range with text. That range can be non-empty to hide content and the text can be non-empty to insert text.
That way there wouldn't be confusion around folding indices within injected text.

@mtbaqer
Copy link
Author

mtbaqer commented Mar 14, 2023

@hediet Thanks for taking the time to look over the PR.

I 100% agree that fully implementing the hideContent flag (which is basically #50840) then doing the custom collapsed text #70794 is the better approach. The reason that did not happen was to reduce the amount changes required to achieve the goal of the PR which was #70794.

Given your knowledge of the codebase, if you think a full implementation of inline folding is absolutely required, then I understand. However, as much as I would love to implement #50840, I'm afraid I would not be able to anytime soon (I'm graduating soon and still in the middle of a job search). Anyone is more than welcome to build on top my current InlineFoldingRange implementation to do it.

On the other hand, if there's anything else we can do to this current PR to provide a solution for #70794 that can be merged with much less work required, I would be more than happy to help with what I can.

@hediet hediet modified the milestones: March 2023, On Deck Mar 20, 2023
@kieferrm kieferrm mentioned this pull request Apr 2, 2023
90 tasks
@kieferrm kieferrm mentioned this pull request May 8, 2023
76 tasks
@kieferrm kieferrm mentioned this pull request Jun 11, 2023
74 tasks
@alexdima alexdima self-assigned this Mar 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[folding] custom folding text for folded ranges
5 participants