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

Folding end pattern for C based languages #9605

Closed
wants to merge 6 commits into from
Closed

Folding end pattern for C based languages #9605

wants to merge 6 commits into from

Conversation

aminroosta
Copy link

@aminroosta aminroosta commented Jul 22, 2016

Atom editor uses the indentation based folding but also includes the line after a fold-range with a foldEndPattern regex check. The regex pattern is different for each language.

For css: 'foldEndPattern': '(?<!_)__/|^\s_}|/\s@EnD\s__/'
For js : 'foldEndPattern': '^\s_}|^\s_]|^\s*)'

Some language extensions in vs code already have this pattern (like php) but not all of them.
With this PR i'm supporting C based languages only (because supporting all languages demands every language extension to be updated).

With this PR the folding behavior will change to match other editors like atom or visual studio or vim or eclipse or ...

Original text:
screen shot 2016-07-22 at 7 23 52 am
With this fix:
screen shot 2016-07-22 at 7 24 41 am
Without this fix:
screen shot 2016-07-22 at 7 24 03 am

related discussion #2971 @aeschli

@msftclas
Copy link

Hi @aminroosta, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

@msftclas
Copy link

@aminroosta, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, MSBOT;

@aeschli
Copy link
Contributor

aeschli commented Jul 22, 2016

Thanks @aminroosta! That's the right direction. What's missing is getting the folding pattern per language and not hardcode it.
If you want to go deeper have a look at LanguageConfiguration that we have for every language. Extensions can define them in code or by a language configuration file. It defines for example the brackets used by every language.
To get a hold of a language configuration use the LanguageConfigurationRegistry. Look here for a use case.
I'd suggest that we add a property folding to the language configuration file. foldEndPattern would be a subproperty.

@aeschli aeschli self-assigned this Jul 22, 2016
@aminroosta
Copy link
Author

@aeschli
Thanks for your reply, i've updated the PR, now it supports individual languages.
could you please take another look at tell me what needs to change ;-)

@aminroosta
Copy link
Author

@aeschli I have two requests / wishes!

1- Could you please add support for C based languages in next vscode release 🙏
If this PR is no good then could you please add it yourself 😉
Only a simple regex like /^\s*\}|^\s*\]|^\s*\)/ will work for js,ts,java,csharp,cpp,c,css,.... and a lot of other languages.
And it makes the folding behavior consistent with other editors.

2- Is it possible to provide information on folded regions? This will really help vim extensions.
If that's not vscode priority could please point me in the right direction and i will send a PR.

@aeschli
Copy link
Contributor

aeschli commented Jul 25, 2016

@aminroosta Thanks a lot for your work. I'm sorry, I won't be able to already push this for the July release (last development day is today). This still needs some discussion and thinking. Not everyone likes the closing bracket to be included in the folded section, so we need to a way to let the user configure which folding strategy is applied. I didn't find the time to think about this, input is highly welcome.

API for folding regions is a separate request, please open a seperate issue so we can discuss it there.

@aminroosta
Copy link
Author

@aeschli Thanks for your response. Sorry for pushing this issue.
Please feel free to read and comment about the following issues when get free :-)

Not everyone likes the closing bracket to be included in the folded section.

I just checked visual studio settings, even visual studio does not have that option.
I think almost all editors include the closing bracket by default and i think it's a better choice.

we need to a way to let the user configure which folding strategy is applied.

I don't think this should be included in user or workspace settings.
Instead language extensions should be able to change folding behavior and users can choose different language extensions. And by folding behavior i mean foldEndPattern, that's the way atom editor or visual studio implements it and everyone seems to be happy about it.

API for folding regions is a separate request, please open a separate issue so we can discuss it there.

Sure! i think @rebornix agreed to open an issue in our discussion at VSCodeVim/Vim#493

@rebornix
Copy link
Member

@aminroosta I'm going through all Vim's Fold commands and see what's the exact scope of Folding info we need. Will send a PR in VSCodeVim and open an issue here as well.

@aeschli
Copy link
Contributor

aeschli commented Jul 26, 2016

From what I know, Sublime does not fold the closing bracket. With Atom there are discussions:
https://discuss.atom.io/t/code-folding-close-bracket/9997 and atom/atom#11179

@rebornix
Copy link
Member

rebornix commented Jul 26, 2016

@aeschli @aminroosta I've opened an issue #9786 for Folding area management, look forward to your ideas.

@aminroosta
Copy link
Author

@rebornix Thanks you very much, Great summary of vim folding features ;)

@aeschli Thank you for the atom link 👍
Here is what i was absolutely missing with this PR

if (someTest) {
    // do some code here
} else {
    // and some other code here
}

///------------ Should collapse to -------------------

if (someText) { ... } else {
    // and some other code here
}

Maybe we should continue discussion on #9786

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.005%) to 60.974% when pulling 7b10fe6 on aminroosta:master into e87f1d9 on Microsoft:master.

@aminroosta
Copy link
Author

@sandy081 @aeschli Any chance you can add folding end patterns for the Sept release?

The first commit of this PR will fix the issue for java, c, c++, js, ts, css, c#, ...
The rest of the PR is not probably the best solution, just include something like the first commit for this Sept release. It's really a 10 minute fix. Don't make us wait another month 😉

@aminroosta
Copy link
Author

@sandy081 @aeschli Any chance you can fix this in Oct release?

@aminroosta
Copy link
Author

Closing because no one cares :-)

@aminroosta aminroosta closed this Oct 27, 2016
@aeschli
Copy link
Contributor

aeschli commented Oct 27, 2016

@aminroosta The problem is that this is a bigger issue and needs more thinking to be done right.
As said there are different folding strategies and users have different preferences.

@aminroosta
Copy link
Author

@aeschli Thanks for your reply.

there are different folding strategies and users have different preferences.

No, Not actually! atom, visual studio, intelli j ... all have the default folding end patterns.
And even if what you are saying is true, shouldn't the default strategy be the most popular one?

this is a bigger issue and needs more thinking to be done right.

For C based languages this can be fixed easily. right? So why would you wait a couple of months (or a year) when you can support 90% of languages with a 10 minute fix? Why?

Atom provides FoldingEndPattern option for extensions, Vscode should do the same, right?

Thanks again and apologies for being impatient 😩

@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.

None yet

5 participants