Skip to content

Add document symbols for #region - #1911

Closed
Frode Flaten (fflaten) wants to merge 10 commits into
PowerShell:mainfrom
fflaten:region-symbol
Closed

Add document symbols for #region#1911
Frode Flaten (fflaten) wants to merge 10 commits into
PowerShell:mainfrom
fflaten:region-symbol

Conversation

@fflaten

@fflatenFrode Flaten (fflaten) commented Aug 26, 2022•
edited
Loading

Copy link
Copy Markdown
Contributor

PR Summary

Adds symbols for regions for easier navigation. Currently available in outline + breadcrumbs and "go to symbol in current editor".

image

PR Context

FixPowerShell/vscode-powershell#3604
FixPowerShell/vscode-powershell#3210

@fflaten

Frode Flaten (fflaten) commented Aug 26, 2022•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Waiting for #1886 as I'd rather get merge conflicts here 🙂

@andyleejordan

Andy Jordan (andyleejordan) commented Feb 2, 2023

Copy link
Copy Markdown
Member

Heh, ok Frode Flaten (@fflaten) things are so cleaned up now I think you can do this again (much more easily!)

@fflatenFrode Flaten (fflaten) mentioned this pull request Feb 7, 2023
Closed
@fflaten
Reuse regex-patterns for regions
27e80e0
For consistent behavior and easier maintenance
@fflaten

Frode Flaten (fflaten) commented Feb 14, 2023•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Rewrote to use ReferenceTable for caching, but still a WIP.
Andy Jordan (@andschwa): Does it make sense to use ReferenceTable here? It'll make it available for handlers incl. find all references. And it isn't something we'd need to track in the workspace.

Todo:

  • Test with full scriptrange (track endregion)
    • How does outline behave when endregion occurs in the middle of a function/class etc?
  • Validate regex behavior against PowerShell ISE
  • Move out of ReferenceTable?
  • Ignore symbol in scenarios like highlight and hover?

@andyleejordan

Andy Jordan (andyleejordan) commented Feb 14, 2023

Copy link
Copy Markdown
Member

Does it make sense to use ReferenceTable here?

I'd keep using it, it'll make everything easier. We can add "region" as a symbol type and just exclude them where appropriate.

@andyleejordan

Andy Jordan (andyleejordan) commented Feb 14, 2023

Copy link
Copy Markdown
Member

Does it make sense to use ReferenceTable here?

I'd keep using it, it'll make everything easier. We can add "region" as a symbol type and just exclude them where appropriate.

Hm I see what you mean now, since it's not done via AST but simply RegEx, and is only useful for the current document...I'll ponder, I'm sure there's a good way to handle that.

@fflaten
Track only regions with matching endregion
851384f
Adds support for outline nesting and breadcrumbs
@fflaten
Never scan for region symbols
6468b56
Regions aren't a reference.
Only useful as cosmetic/navigation symbol in current document.
@fflaten
Exclude hover for Region symbols
907d481
@fflaten
Exclude highlight for Region symbols
4aed74c
@fflaten

Frode Flaten (fflaten) commented Feb 18, 2023•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

  • Test with full scriptrange (track endregion)

    • How does outline behave when endregion occurs in the middle of a function/class etc?

It didn't break as I feared, but only wraps symbols with both start and end position inside. Ex.
image

Updated to include it and returning only regions with matching end just like folding.
I've disabled hover, highlight (on click) and reference scanning.

Hopefully we'll get some feedback if it becomes too noisy in go to symbol or the outline-nesting. Breadcrumbs require the latter to work, so hopefully worth it.

@fflaten
Frode Flaten (fflaten) marked this pull request as ready for review February 18, 2023 12:57
@fflaten
Frode Flaten (fflaten) requested a review from a teamFebruary 18, 2023 12:57
andyleejordan
Andy Jordan (andyleejordan) requested changes Feb 23, 2023
View reviewed changes

@andyleejordanAndy Jordan (andyleejordan) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If you want to take a crack at what I'm contemplating, go for it! Otherwise I'll try to get to it later today.

Comment threadsrc/PowerShellEditorServices/Services/Symbols/ReferenceTable.cs Outdated
Comment threadsrc/PowerShellEditorServices/Services/Symbols/SymbolsService.cs Outdated
Comment threadsrc/PowerShellEditorServices/Services/Symbols/Visitors/RegionVisitor.cs Outdated
{
internal static class RegionVisitor
{
internal static void GetRegionsInDocument(ScriptFile file, Func<SymbolReference, AstVisitAction> action)

@andyleejordanAndy Jordan (andyleejordan)Feb 23, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I like this a lot, makes reworking it much easier!

@fflatenFrode Flaten (fflaten)Feb 24, 2023•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Andy Jordan (@andschwa): I assume you meant the separate class.
Because the Func-parameter won't make much sense when moving out of ReferenceTable, would it? Unless you'd prefer Action<SymbolReference>-parameter over IEnumerable return type.

Comment threadsrc/PowerShellEditorServices/Services/Symbols/Visitors/RegionVisitor.cs
Comment threadsrc/PowerShellEditorServices/Services/TextDocument/Handlers/DocumentHighlightHandler.cs Outdated
Comment threadsrc/PowerShellEditorServices/Services/TextDocument/Handlers/HoverHandler.cs Outdated
Comment threadtest/PowerShellEditorServices.Test/Language/SymbolsServiceTests.cs Outdated
@andyleejordan

Andy Jordan (andyleejordan) commented Feb 23, 2023

Copy link
Copy Markdown
Member

Does it make sense to use ReferenceTable here?

I'd keep using it, it'll make everything easier. We can add "region" as a symbol type and just exclude them where appropriate.

I was wrong here 😆

fflaten
Frode Flaten (fflaten) commented Feb 24, 2023
View reviewed changes
Comment threadsrc/PowerShellEditorServices/Services/TextDocument/Handlers/DocumentSymbolHandler.cs
IEnumerable<SymbolReference> foundSymbols = ProvideDocumentSymbols(scriptFile);
if (foundSymbols is null)
{
return null;
}
foundSymbols = foundSymbols.Concat(RegionVisitor.GetRegionsInDocument(scriptFile));

@fflatenFrode Flaten (fflaten)Feb 24, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I didn't see a reason to keep the null-check. The local method ProvideDocumentSymbols() always returns a list, right?

fflaten
Frode Flaten (fflaten) commented Feb 24, 2023
View reviewed changes
Comment threadtest/PowerShellEditorServices.Test/Language/SymbolsServiceTests.cs
}

[Fact]
public void FindsRegionSymbolsInFile()

@fflatenFrode Flaten (fflaten)Feb 24, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

This technically doesn't belong here as SymbolsService isn't used for DocumentSymbolHandler. Move to own test-file? Suggestion?

@andyleejordanAndy Jordan (andyleejordan) mentioned this pull request Feb 24, 2023
Merged
@andyleejordan

Andy Jordan (andyleejordan) commented Feb 28, 2023

Copy link
Copy Markdown
Member

Closing since I integrated this, thanks again so much for your work!

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@andyleejordanAndy Jordan (andyleejordan)andyleejordan requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Outline should also support #Region Include regions in symbol list/outline/breadcrumbs

2 participants

@fflaten@andyleejordan