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

Still having problems with Regions folding on 1.8.2 #1458

Closed
MayPLaY opened this issue Aug 1, 2018 · 18 comments · Fixed by #1467
Closed

Still having problems with Regions folding on 1.8.2 #1458

MayPLaY opened this issue Aug 1, 2018 · 18 comments · Fixed by #1467

Comments

@MayPLaY
Copy link

MayPLaY commented Aug 1, 2018

Issue Description

Hi,
I saw with 1.8.0/1.8.1 you guys had some problems with regions folding, and 1.8.2 was supposed to fix it. But as you see in this gif, I'm still having problems with everything getting folded instead of my designed regions.

I'm using "ctrl+k ctrl+8" and "ctrl+k ctrl+9" to fold/unfold. With the extentsion installed, I'm having trouble, but without it, I got no problem at all. I can't recall I moment where it didn't work before 1.8.x.

Thanks !!

powershellextension_regionsproblem

Environment Information

Visual Studio Code

Name Version
Operating System Windows_NT x64 10.0.17134
VSCode 1.25.1
PowerShell Extension Version 1.8.2

PowerShell Information

Name Value
PSVersion 5.1.17134.165
PSEdition Desktop
PSCompatibleVersions 1.0 2.0 3.0 4.0 5.0 5.1.17134.165
BuildVersion 10.0.17134.165
CLRVersion 4.0.30319.42000
WSManStackVersion 3.0
PSRemotingProtocolVersion 2.3
SerializationVersion 1.1.0.1

Visual Studio Code Extensions

Visual Studio Code Extensions(Click to Expand)
Extension Author Version
PowerShell ms-vscode 1.8.2
@rjmholt
Copy link
Collaborator

rjmholt commented Aug 1, 2018

Hi @MayPLaY, I've watched through the GIF a couple of times, but just want to be clear about the behaviour you're seeing vs the behaviour you're expecting.

Can you restate, in words, (1) the behaviour you're expecting and (2) the behaviour you're experiencing that differs from that expectation?

Also worth noting that this folding feature can be disabled with:

"powershell.codeFolding.enable": false

@MayPLaY
Copy link
Author

MayPLaY commented Aug 1, 2018

Sorry for the confusion.

Expected

  • Calling command "Un/Fold All Regions" : Get only "#region" and "#endregion" tag folding.
  • Calling command "Un/Fold All" : Get everything fold (regions, statements, functions, etc.)
  • This is what happen when the folding feature is disable (as you suggested), or extension is uninstalled.

What it does

  • "Un/Fold All Regions" and "Un/Fold All" commands act exactly the same, they both fold everything.

I hope it's a bit more clear :).

Thanks.

@TylerLeonhardt
Copy link
Member

@glennsarti for the awareness.

I think we currently mark a lot of these folds as Region ... Except for <# #>

As a test... Does the "Un/Fold All Regions" fold block comments <# #>?

@TylerLeonhardt
Copy link
Member

@MayPLaY for the notification just in case

@MayPLaY
Copy link
Author

MayPLaY commented Aug 1, 2018

Your guess is right ,"Un/Fold All Regions" doesn't fold block comments, but "Un/Fold All" does.

powershellextension_commandproblemfold

@glennsarti
Copy link
Contributor

glennsarti commented Aug 2, 2018

This seems like expected behaviour.

  • Line comments (# line comment) and region comments (<# ..... #>) are Comments (Unfold/Fold all Block Comments)

  • Everything else is a Region (Unfold/Fold all Regions)

There are only 3 types of folding types (https://code.visualstudio.com/docs/extensionAPI/vscode-api#FoldingRangeKind)

@TylerLeonhardt
Copy link
Member

Yeah, Glenn's right. I think we should probably open an issue on vscode to add another item to the enum... Maybe like a "Code" FoldingRangeKind

@glennsarti
Copy link
Contributor

I haven't tried using an invalid enum integer e.g. -1

@rjmholt
Copy link
Collaborator

rjmholt commented Aug 2, 2018

Actually, that's not a bad idea. Does assigning 4 work?

Wait, we're depending on VSCode to work out what to do based on the value, so we need a new enum value...

@glennsarti
Copy link
Contributor

glennsarti commented Aug 2, 2018

Okay...so quick test, it does work. With an ID that is not in the enum, it doesn't respond to Unfold/Fold all Regions or Block Comments, BUT it still responsed to Unfold/Fold All.

Can't say I'm happy about it though. It's definitely a hack


Changed the following

From:

        // Find matching comment regions   #region -> #endregion
        this.matchScopeElements(
            this.extractRegionScopeElements(tokens, document),
            "custom.start.region",
            "custom.end.region",
            vscode.FoldingRangeKind.Region, document)
            .forEach((match) => { matchedTokens.push(match); });

To

        // Find matching comment regions   #region -> #endregion
        this.matchScopeElements(
            this.extractRegionScopeElements(tokens, document),
            "custom.start.region",
            "custom.end.region",
            56, document)
            .forEach((match) => { matchedTokens.push(match); });

@TylerLeonhardt
Copy link
Member

Ok. Let's maybe have a const static variable called like FoldingRangeKindCode? That's set to some arbitrary int?

Then we can have the expected behavior at least.

@rjmholt
Copy link
Collaborator

rjmholt commented Aug 2, 2018

Wow, issue 55,686!

@rjmholt
Copy link
Collaborator

rjmholt commented Aug 4, 2018

To quote from the VSCode issue:

Region is only to be used for ranges originating from folding range marked by #region and #endregion

For syntax ranges, don't set kind. kind is optional.

@TylerLeonhardt
Copy link
Member

Nice find, Rob!

@glennsarti
Copy link
Contributor

OKay, I sense another PR on the way

@glennsarti
Copy link
Contributor

glennsarti commented Aug 6, 2018

Might have to PR a docs edit to the VS Code API. As it doesn't mention that bit. The only thing to indicate it's optional is the ? in the interface definition, which is pretty obtuse.

... And it's already done. microsoft/vscode@dea3960

glennsarti added a commit to glennsarti/vscode-powershell that referenced this issue Aug 6, 2018
Previously all of the folding ranges were either Region or Comment, however this
is confusing e.g. a Here String is neither a region or a comment.  The VS Code
API does state that the range kind property is optional.  This commit changes
the range kind for braces, parentheses and here strings to be null, that is,
neither a region or comment range. This makes editor commands like
'Fold All Regions' behave as a user expects.
glennsarti added a commit to glennsarti/vscode-powershell that referenced this issue Aug 6, 2018
Previously all of the folding ranges were either Region or Comment, however this
is confusing e.g. a Here String is neither a region or a comment.  The VS Code
API does state that the range kind property is optional.  This commit changes
the range kind for braces, parentheses and here strings to be null, that is,
neither a region or comment range. This makes editor commands like
'Fold All Regions' behave as a user expects.
@glennsarti
Copy link
Contributor

Fix PR is up - #1467

@MayPLaY
Copy link
Author

MayPLaY commented Aug 6, 2018

Thanks @glennsarti

TylerLeonhardt pushed a commit that referenced this issue Aug 6, 2018
Previously all of the folding ranges were either Region or Comment, however this
is confusing e.g. a Here String is neither a region or a comment.  The VS Code
API does state that the range kind property is optional.  This commit changes
the range kind for braces, parentheses and here strings to be null, that is,
neither a region or comment range. This makes editor commands like
'Fold All Regions' behave as a user expects.
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 a pull request may close this issue.

4 participants