Skip to content

add an explicit based folding strategy#53910

Closed
daiyam wants to merge 1 commit into
microsoft:release/1.25from
daiyam:explicit-folding-strategy
Closed

add an explicit based folding strategy#53910
daiyam wants to merge 1 commit into
microsoft:release/1.25from
daiyam:explicit-folding-strategy

Conversation

@daiyam

@daiyam daiyam commented Jul 9, 2018

Copy link
Copy Markdown
Contributor

Hello,

I've added an explicit based folding strategy. It can resolve the followings issues: #41633 & #36002.

It can be easily configured like:

{
    "editor.foldingStrategy": "explicit",

    "editor.foldingExplicitMarkers": {
        "comment": {
          "start": "\\/\\*\\*",
          "end": "\\*\\/"
        },
        "region": {
          "start": "\\{\\{\\{",
          "end": "\\}\\}\\}"
        }
    },

    "[javascript]": {
        "editor.foldingStrategy": "auto"
    },

    "[json]": {
        "editor.foldingStrategy": "indentation"
    },

    "[jsonc]": {
        "editor.foldingStrategy": "indentation"
    },

    "[typescript]": {
        "editor.foldingStrategy": "auto"
    }
}

@msftclas

msftclas commented Jul 9, 2018

Copy link
Copy Markdown

CLA assistant check
All CLA requirements met.

@aeschli

aeschli commented Jul 11, 2018

Copy link
Copy Markdown
Contributor

The actual 'indentation' strategy is actually 'indentation & predefined markers'.
Instead of adding a new strategy, what about extending the 'indentation' strategy to 'indentation & predefined markers & user markers'.
IMO that would fit better with the current story (and would fix #36002).

@daiyam

daiyam commented Jul 11, 2018

Copy link
Copy Markdown
Contributor Author

@aeschli I agree, for #36002, the pull request need to be reworked.

But let's review the current folding strategies:

  • auto -> (block | indentation) & predefined markers
  • indentation -> indentation & predefined markers

If we add user markers, we get:

  • auto -> (block | indentation) & predefined markers & user markers
  • indentation -> indentation & predefined markers & user markers
    That's fine.

But, the explicit strategy is a predefined markers | user markers only strategy.
Why do I need that strategy?

It's about readability of code (even with correctly formatted and commented code).
When you open a file with more than 2000 lines, at first glance, it's difficult to know its structure, what's in it and what's going on.
So, you fold the code to see its structure.
But with auto and indentation strategies, the folding become tricky. For example, if you have a class in a namespace, the class disappears. You have to unfold the namespace, and then unfold the class to read its properties or methods.
With the explicit strategy, you can see immediately the structure of the code (if there are markers). You dan't have to unfold the if/for block in functions.

In vim, it's the marker method
In jEdit, it's the explicit mode

I hope I was good enough so you can see the advantage to add another strategy 😃

@aeschli

aeschli commented Jul 11, 2018

Copy link
Copy Markdown
Contributor

Just some food for thought (and clarification on what auto means):

Besides the indentation based folding strategy, we also have ranges coming from folding range providers. Folding range providers can be added by extensions through code. We have such providers for TypeScript, JSON, CSS...

'auto' means the best available strategy is chosen.
If a folding range provider is available, its ranges will be used.
If more than one folding provider is available, all ranges will be merged (best effort).

  • auto -> (folding range provider results) or (indentation & predefined markers)
  • indentation -> indentation & predefined markers

Note that folding range provider have full control of the ranges. Neither indention based ranges nor predefined markers are merged in. Instead, we expect the folding range provider to correctly add ranges for the predefined markers (he has a better understanding of the language and can better detect markers in comments or string literals.

You could implement your feature also as a folding range provider. But if another extension also provides folding provider for the language, both will be merged, resulting in the extra nesting that you described above.
The idea is to allow give a folding range provider a name which then could be referenced in the folding strategy.

@daiyam

daiyam commented Jul 11, 2018

Copy link
Copy Markdown
Contributor Author

I've tried with an extension (Zokugun Folding) which add user markers (in a quick&dirty way).

But with the auto strategy, the problems are:

  • multiple markers (markers defined in the language and by the extension)
  • folding ranges provided by the language (I only want markers)

I see three ways to resolve those:

  • add the explicit strategy
  • add a none strategy to bypass both auto and indentation. Then I rename my extension as Explicit Folding (with some fixes).
  • add settings for the folding range provider:
    • to allow user markers
    • to disable folding ranges of statements, block, declarations, ...

The third option would be the best because like you said, the folding range provider has a better understanding of the language. But it would need to be modified to support those new settings.

So the second one can be good solution until all folding range providers support the new settings.

What do you think?

@aeschli

aeschli commented Jul 12, 2018

Copy link
Copy Markdown
Contributor

I see two ways:
a.

  • new user setting for folding markers and new only-markers strategy
  • indentation strategy uses: indentation & predefined markers & user markers
  • only-markers strategy uses: predefined markers & user markers

b.

  • allow folding range provider to have an id
interface FoldingRangeProvider {
     id?: string;
     providerFoldingRanges(...) ...
}
  • allow the user to enter that id as a strategy
 "editor.foldingStrategy": "zukogun"

@daiyam

daiyam commented Jul 12, 2018

Copy link
Copy Markdown
Contributor Author

I would go with b. It's better than my none strategy.

But I have a question, should the auto strategy exclude FoldingRangeProviders which have an id?

@aeschli

aeschli commented Jul 12, 2018

Copy link
Copy Markdown
Contributor

I'd have auto merge all providers, like it is now. What do you think?

@daiyam

daiyam commented Jul 12, 2018

Copy link
Copy Markdown
Contributor Author

Yes, that's fine.

What is the next step?

@aeschli

aeschli commented Jul 12, 2018

Copy link
Copy Markdown
Contributor

If you have interest (and time) a PR would be great. Unfortunately, I don't know when I will have cycles to work on it.

daiyam added a commit to daiyam/vscode that referenced this pull request Jul 12, 2018
@daiyam

daiyam commented Jul 12, 2018

Copy link
Copy Markdown
Contributor Author

The PR has been submitted and the updated extension is available at zokugun/vscode-explicit-folding.

}
}

const length = startIndexes.length;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Can use destructures
const { length } = startIndexes;

@aeschli

aeschli commented Sep 17, 2018

Copy link
Copy Markdown
Contributor

closed in favor of #54200

@aeschli aeschli closed this Sep 17, 2018
@github-actions github-actions Bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants