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 mix of Attributes and single-line comments #733

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

apupier
Copy link
Member

@apupier apupier commented Jun 23, 2023

It removes the possibility to have Folding range for the first group of attributes or first group of single-line comments. This is due to a limitation of VS Code which doesn't handle several folding ranges starting on the same line. This limitation lead me to add some non negligeable code complexity.

resolves #719

FolginGroupOfAttributesAndSingleLineComments.mp4

@apupier apupier force-pushed the 719-FoldMixOfAttributesAndComments branch from a12d3cf to f95b2fe Compare June 23, 2023 00:28
@apupier apupier requested a review from ggrossetie June 23, 2023 00:29
@apupier apupier marked this pull request as ready for review June 23, 2023 00:29
It removes the possibility to have Folding range for the first group of
attributes or first group of single-line comments. This is due to a
limitation of VS Code which doesn't handle several folding ranges
starting on the same line.

resolves asciidoctor#719

Signed-off-by: Aurélien Pupier <[email protected]>
@apupier apupier force-pushed the 719-FoldMixOfAttributesAndComments branch from f95b2fe to d122d27 Compare June 23, 2023 12:01
@ggrossetie
Copy link
Member

Arguably, we could collapse the document header when we click on the document title. The downside is that you cannot collapse the entire file but is that really useful?

Would it simplify the logic?

@apupier
Copy link
Member Author

apupier commented Jun 27, 2023

Arguably, we could collapse the document header when we click on the document title. The downside is that you cannot collapse the entire file but is that really useful?

I do not understand why it would interfere with collapsing the whole file.

I feel like what would be interesting would be to implement the folding for each section delimited by = and the various levels of ==...=.

@ggrossetie
Copy link
Member

I believe that, currently, if you fold on the document title, it will fold the whole document, correct?

What I'm proposing is that if you fold on the document title, it will only fold the document header (i.e. document attributes, author line, etc...).

By doing that, my guess is that you can simplify the logic for attributes folding? You can fold a group of attributes until a comment or an empty line is found.

@apupier
Copy link
Member Author

apupier commented Jun 29, 2023

I believe that, currently, if you fold on the document title, it will fold the whole document, correct?

noFoldOnDocumentTitle.mp4

no, there is no fold on the document title

@apupier
Copy link
Member Author

apupier commented Jun 29, 2023

What I'm proposing is that if you fold on the document title, it will only fold the document header (i.e. document attributes, author line, etc...).

By doing that, my guess is that you can simplify the logic for attributes folding? You can fold a group of attributes until a comment or an empty line is found.

So implementing it this way will effectively simplify code but I think having the fold of a whole "titled section" is more interesting.

Another side thoughts, if a folding section is missing, end users are nto block and can create their own custom folding section https://code.visualstudio.com/docs/editor/codebasics#_fold-selection

@ggrossetie
Copy link
Member

but I think having the fold of a whole "titled section" is more interesting.

What do you mean? My idea was to have a special fold on the document title (single =) but keep the existing behavior for section titles (two or more =).

Another side thoughts, if a folding section is missing, end users are nto block and can create their own custom folding section https://code.visualstudio.com/docs/editor/codebasics#_fold-selection

Oh I didn't know that! We should probably mention that in the documentation!

@ggrossetie
Copy link
Member

I think I understand what's bothering you with my proposal.
In the following case, we won't be able to fold the whole block of attributes/comments since my proposal is basically to fold until a comment is found.

this is a paragraph

:an-attribute1: value of attribute
:an-attribute2: value of attribute
// A single-line comment.
:an-attribute: value of attribute
// A second single-line comment.
:a-second-attribute: value of second attribute

this is another paragraph.

Having said that, I don't think this is a common use case. The document header is where we usually have a mix of comments and attributes. Attributes defined in the body of the document are "rare". It's also extremely uncommon to have a block of comments/attributes in the body.

So I think we should fold attributes until we found a comment or something that's not an attribute.

Does it make sense?

@ggrossetie
Copy link
Member

I think it's difficult to decide because it depends 😄

In the example below, we most likely want to fold all the attribute entries + comment:

this is a paragraph

:an-attribute1: value of attribute
:an-attribute2: value of attribute
//:an-attribute3: value of attribute
:an-attribute4: value of attribute

this is a paragraph

However in the following example I would argue that we want to fold per "group":

= Embracing a new thing
// localization
:example-caption: Exemple
:tip-caption: Astuce
// reveal.js options
:revealjs_fragmentInURL: true
:revealjs_hash: true
:revealjs_controls: true
:revealjs_controlsLayout: edges
:revealjs_controlsTutorial: true
:revealjs_slideNumber: c/t
:revealjs_showSlideNumber: speaker
:revealjs_autoPlayMedia: true
:revealjs_totalTime: 2700
// style
:icons: font
:source-highlighter: highlight.js

content...

In other words, comments are used as headers/delimiters so it makes sense to fold the localization group, the reveal.js options group and the style group independently.

For reference, the Intellij IDEA extension does not seem to provide this feature.

@mojavelinux @ahus1 I would love your input on this issue 🥺

@mojavelinux
Copy link
Member

My intuition says that groups of attribute entries would be folded behind a leading line comment (the line comment being the fold line). (I'm not sure if that's feasible to implement, but it would make sense to me as a user).

@ggrossetie
Copy link
Member

My intuition says that groups of attribute entries would be folded behind a leading line comment (the line comment being the fold line).

I agree 👍🏻

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.

folding of properties/attributes
3 participants