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

Cabal plugin outline view #4323

Merged
merged 25 commits into from
Jul 30, 2024
Merged

Conversation

VenInf
Copy link
Contributor

@VenInf VenInf commented Jun 15, 2024

This is an attempt to solve an issue #4282.
It adds an Outline for the cabal files.

Implementation details

Using runIdeAction we get parsed cabal fields with positions of the keywords.

This can be transformed with a series of functions to the DocumentSymbol. It is returned, and the moduleOutline can be plugged in to the cabal descriptor.

Copy link
Collaborator

@michaelpj michaelpj left a comment

Choose a reason for hiding this comment

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

Needs some tests. I'll let the others comment on the symbol kind choices, I'm not sure what those constructors actually correspond to.

@VeryMilkyJoe
Copy link
Collaborator

Thank you for working on this feature, it seems like a great addition to the plugin!
Could you add a screenshot of what the feature looks like in action?

@VenInf
Copy link
Contributor Author

VenInf commented Jun 17, 2024

Thank you for working on this feature, it seems like a great addition to the plugin! Could you add a screenshot of what the feature looks like in action?

Yes, sure.
There is how the outline looks for haskell-language-server.cabal file and simple.cabal file from cabal plugin tests.
hls_cabal_outline_screenshot
simple_cabal_outline_sway-screenshot

@VeryMilkyJoe
Copy link
Collaborator

I was trying out the feature, it looks like a great start!
But, I think there is some unwanted behaviour here.
Firstly, I think it would be less overwhelming to only see the parents in the outline by default, with everything else collapsed.
Secondly, I think it would be easier to use the outline if the names of stanzas were part of the respective elements instead of being a child.
Also, I think the if conditions are not grouped correctly as can be seen below.
image

@VeryMilkyJoe
Copy link
Collaborator

I think, it could also make sense to not show leaf values in the outline at all?
Specifically for long descriptions or exposed-modules content, the text becomes rather long.

@VenInf
Copy link
Contributor Author

VenInf commented Jun 17, 2024

I was trying out the feature, it looks like a great start! But, I think there is some unwanted behaviour here. Firstly, I think it would be less overwhelming to only see the parents in the outline by default, with everything else collapsed.

I thought that this will be an easy fix, but it looks like that it's unclear, who can provide such settings. It certanly can be done manualy, but I can't tell if it's possible to do by default. I will look into it, but I will be glad to hear any ideas for a soltion.

Secondly, I think it would be easier to use the outline if the names of stanzas were part of the respective elements instead of being a child. Also, I think the if conditions are not grouped correctly as can be seen below.

Right now the outline displays the cabal's AST without any modifications. I think it will be nice to see the if statements in one line, and looks like it can be done by jamming all SectionArgs on one level in one DocumentSymbol. I will test this.

@VenInf
Copy link
Contributor Author

VenInf commented Jun 17, 2024

I think, it could also make sense to not show leaf values in the outline at all? Specifically for long descriptions or exposed-modules content, the text becomes rather long.

At least for me it's not obvious, what data should be displayed, and what omitted. As far as I can tell, it can be independent with the depth in the cabal's AST.

I think a collapsed outline would solve this issue as well.

@michaelpj
Copy link
Collaborator

I don't think we have any control over whether or not the outline is expanded or not.

At least for me it's not obvious, what data should be displayed, and what omitted.

As I understand it, the outline view is generally used for navigation between named entities in the file. So I definitely don't think we should be including the values of fields, that's quite odd.

I think having the top-level field names is probably useful, in that it allows jumping to them, but I think it would be nice to group them in a "top-level stanza" element, perhaps?

Just
(defDocumentSymbol range)
{ LSP._name = decodeASCII fieldName,
LSP._kind = LSP.SymbolKind_Object,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
LSP._kind = LSP.SymbolKind_Object,
LSP._kind = LSP.SymbolKind_Field,

where
range = cabalPositionToLSPRange pos `addNameLengthToLSPRange` decodeASCII string

documentSymbolForFieldLine :: FieldLine Position -> Maybe LSP.DocumentSymbol
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think what I'm suggesting is that we get rid of this.

where
range = cabalPositionToLSPRange pos `addNameLengthToLSPRange` decodeASCII fieldName

documentSymbolForSectionArgs :: SectionArg Position -> Maybe LSP.DocumentSymbol
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm unsure we want to include these either, what do they correspond to? It looks like this is responsible for e.g. the weirdness with conditionals.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think they can also include the names of stanzas so we definitely want to process them somehow but probably not into their own DocumentSymbol element

@VenInf
Copy link
Contributor Author

VenInf commented Jun 18, 2024

Fixed some minor issues, and hopefully solved the problem with displaying values of the fields.
Also now the Section and SectionArgs are displayed in one line.
For example source-repository head and if impl(ghc >= 9.8)

cabal_outline_ifs_oneline_sway-screenshot

@VeryMilkyJoe
Copy link
Collaborator

Fixed some minor issues, and hopefully solved the problem with displaying values of the fields. Also now the Section and SectionArgs are displayed in one line. For example source-repository head and if impl(ghc >= 9.8)

cabal_outline_ifs_oneline_sway-screenshot

This looks great!

@VeryMilkyJoe
Copy link
Collaborator

To fix the pre-commit failing, you will want to run the stylish haskell formatter on your code, especially the imports.

@VenInf
Copy link
Contributor Author

VenInf commented Jun 18, 2024

Once the functionality will be set I will implement the tests.

@VenInf
Copy link
Contributor Author

VenInf commented Jun 21, 2024

The tests are now implemented as well.

@VenInf VenInf force-pushed the cabal-plugin-outline-view branch from 75ee342 to f4d57a5 Compare July 2, 2024 03:04
Copy link
Collaborator

@fendor fendor left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! Just some documentation nitpicks, code looks good otherwise!

plugins/hls-cabal-plugin/test/Outline.hs Outdated Show resolved Hide resolved
plugins/hls-cabal-plugin/src/Ide/Plugin/Cabal/Outline.hs Outdated Show resolved Hide resolved
Copy link
Collaborator

@fendor fendor left a comment

Choose a reason for hiding this comment

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

One more thing

plugins/hls-cabal-plugin/src/Ide/Plugin/Cabal/Outline.hs Outdated Show resolved Hide resolved
@VeryMilkyJoe
Copy link
Collaborator

@VenInf You need to format some imports, but afterwards I think we're good to go to merge!

@VeryMilkyJoe VeryMilkyJoe enabled auto-merge (squash) July 15, 2024 18:02
@fendor
Copy link
Collaborator

fendor commented Jul 15, 2024

I don't understand why this isn't merging... Or rather, why CircleCI seems to be hating @VenInf in particular.

@VenInf
Copy link
Contributor Author

VenInf commented Jul 15, 2024

I don't understand why this isn't merging... Or rather, why CircleCI seems to be hating @VenInf in particular.

Maybe I have to register somewhere? Or provide some sort of credentials?

@fendor fendor disabled auto-merge July 30, 2024 12:29
@fendor fendor enabled auto-merge (squash) July 30, 2024 12:54
@fendor fendor merged commit 0bf3348 into haskell:master Jul 30, 2024
21 checks passed
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.

4 participants