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

{DO NOT MERGE} (GH-1609) Remove syntax folder provider feature #1610

Merged
merged 1 commit into from
Nov 28, 2018

Conversation

glennsarti
Copy link
Contributor

@glennsarti glennsarti commented Nov 13, 2018

THIS PR SHOULD NOT BE MERGED UNTIL PowerShell/PowerShellEditorServices#777 is merged

PR Summary

Now that the PowerShell Editor Services has a Folding Provider, this extension
based feature is no longer required. This commit removes the feature and tests.

The PSES Issue #793 [1] tracks the work to add it to PSES.

[1] PowerShell/PowerShellEditorServices#793

PR Checklist

Note: Tick the boxes below that apply to this pull request by putting an x between the square brackets.
Please mark anything not applicable to this PR NA.

  • PR has a meaningful title
  • Summarized changes
  • PR has tests
  • This PR is ready to merge and is not work in progress
    • If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready

Fixes #1609

Now that the PowerShell Editor Services has a Folding Provider, this extension
based feature is no longer required.  This commit removes the feature and tests.

The PSES Issue PowerShell#793 [1] tracks the work to add it to PSES.

[1] PowerShell/PowerShellEditorServices#793
@rjmholt
Copy link
Contributor

rjmholt commented Nov 28, 2018

Just to make sure, VSCode itself implements the client-side folding right? So we don't need to add any new handling for the server-side provider ourselves?

@rjmholt
Copy link
Contributor

rjmholt commented Nov 28, 2018

We should link to the relevant VSCode feature documentation in the issue

@glennsarti
Copy link
Contributor Author

glennsarti commented Nov 28, 2018

I haven't seen the source code but this what I've observed;

  1. If the language server says it provides a Folding Provider VSCode uses that.
  2. If the extension registers a FoldingProvider then VSCode uses that.
  3. Otherwise it defaults to the indentation default folder within VSCode

@rjmholt
Copy link
Contributor

rjmholt commented Nov 28, 2018

Oh right, so the registration is in the FoldingProvider.Enable (or whatever it is) part of the initialisation message response?

@glennsarti
Copy link
Contributor Author

We should link to the relevant VSCode feature documentation in the issue

AFAIK There is no explicit documentation on this

@glennsarti
Copy link
Contributor Author

glennsarti commented Nov 28, 2018

Oh right, so the registration is in the FoldingProvider.Enable (or whatever it is) part of the initialisation message response?

Yes. The logic in PSES is;

  1. The Server says it supports folding and the folder is enabled.
  2. The Client (VSCode) later sends the DidChangeConfiguration notification which then enables or disables the folder in PSES

@rjmholt
Copy link
Contributor

rjmholt commented Nov 28, 2018

AFAIK There is no explicit documentation on this

Bugger. I'll keep searching just so we have a few signposts on how it all works.

@glennsarti
Copy link
Contributor Author

glennsarti commented Nov 28, 2018

The deep internal reference documentation of VSCode isn't the greatest :-(

https://github.com/Microsoft/vscode/blob/957518fdffe5b62262e957e2f942e373f825312a/src/vs/editor/contrib/folding/folding.ts

https://github.com/Microsoft/vscode/blob/0ac0c45b29c9c138e39c4b12602302650d9e405c/src/vs/editor/common/modes.ts#L1343

This is probably no different that a Document Symbol provider or Hover provider or Completion provider.

Copy link
Member

@TylerLeonhardt TylerLeonhardt left a comment

Choose a reason for hiding this comment

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

If this is really as simple as it sounds and we just need to remove the feature client-side this looks good to me!

@glennsarti
Copy link
Contributor Author

Works on my machine. Would appreciate someone on Mac and/or Ubuntu to try it out.

@TylerLeonhardt
Copy link
Member

Checked out your code in both repos. Can confirm, works on macOS 😄

Folding a here string worked perfectly and settings updates were automatically applied.

Copy link
Contributor

@rjmholt rjmholt left a comment

Choose a reason for hiding this comment

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

👍

@rjmholt rjmholt merged commit 79848af into PowerShell:master Nov 28, 2018
@cpdohert
Copy link

The new syntax folding doesn't work correctly. Prior to this PR, "Fold All" did what I expect: function definitions collapsed into a single line, etc. Now, folding is randomly collapsing more than a single function/stanza, or not an entire function/stanza. Syntax folding is essentially completely broken for our PowerShell module authoring as of this PR. I can't easily provide examples witout violating company NDA, but if I am not the only one with this issue I can generate representative code that displays the problem.

@rjmholt
Copy link
Contributor

rjmholt commented Dec 10, 2018

Hi @cpdohert, thanks for the feedback. The issue tracking this is #1631. The fix is being worked on in PowerShell/PowerShellEditorServices#805.

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.

Move syntax aware folding provider to PowerShell Editor Services
4 participants