Skip to content
This repository has been archived by the owner on Nov 5, 2024. It is now read-only.

Added folding on header lines #399

Closed
wants to merge 5 commits into from

Conversation

WalrusGumboot
Copy link

When I find myself working on longer documents, I often enjoy maintaining a sense of structure by collapsing big blocks of the text at once. This PR adds some very basic functionality of doing so with folding ranges; it detects headings solely based on the '=' character. While this isn't ideal (something based on a syntactic analysis to fold all nodes that are a child of a certain heading would be more robust) it suits my personal needs well enough and might be worth considering.

A quick note on the behaviour

In a document like

= this is a heading

some text

== subheading a

some more text

== subheading b

yet more text

folding ranges will be added as such:

┌──> │ = this is a heading
│    │ 
│    │ some text
└──< │ 
┌──> │ == subheading a
│    │ 
│    │ some more text
└──< │ 
┌──> │ == subheading b
│    │ 
└──< │ yet more text

instead of the hierarchical

┌──> │ = this is a heading
│    │ 
│    │ some text
│    │ 
│ ┌> │ == subheading a
│ │  │ 
│ │  │ some more text
│ └< │ 
│ ┌> │ == subheading b
│ │  │ 
│ └< │ yet more text
└──< │ 

@Myriad-Dreamin
Copy link
Collaborator

related #165

@Myriad-Dreamin
Copy link
Collaborator

@WalrusGumboot we are reaching a nice implementation, but unluckily I guess there is a bug when we simply use document_symbol. The result of document_symbol contains all symbols in a document. Therefore, in addition to headings, labels <label> and let binding #let a = 1 and also separate a folding range here.

SyntaxKind::Label => {
let ast_node = node
.cast::<ast::Label>()
.ok_or_else(|| anyhow!("cast to ast node failed: {:?}", node))?;
let name = ast_node.get().to_string();
if let Some(query) = query_string {
if !name.contains(query) {
return Ok(None);
}
}
let symbol = SymbolInformation {
name,
kind: SymbolKind::CONSTANT,
tags: None,
deprecated: None, // do not use, deprecated, use `tags` instead
location: Location {
uri: uri.clone(),
range: typst_to_lsp::range(node.range(), source, position_encoding).raw_range,
},
container_name: None,
};
Ok(Some(symbol))
}

imo we should exclude them.

Could you exclude these unrelated symbols?

image

@WalrusGumboot
Copy link
Author

Oops, didn't test extensively enough 😅

I guess filtering for SymbolKind::NAMESPACE works, and semantically it makes sense to fold on namespace-type symbols anyway.

@Myriad-Dreamin
Copy link
Collaborator

Myriad-Dreamin commented Jan 4, 2024

We have correct heading to folding now. But I find another bug, unfortunately. header foldings should be closed by their current parent, but now we don't and cannot do it simply. Accurately, a correct folding range identification should also regard braces. Would you like to continue solving this inperfect? I personally think it doesn't bother much.

image image

Oops, the default folding range support on braces is disabled since this PR.

image

@WalrusGumboot
Copy link
Author

Hmm, yeah, that seems like more work than I've got time for at the moment. Maybe after the exam season.

@Myriad-Dreamin
Copy link
Collaborator

well, we may reserve this feature PR for you, @WalrusGumboot. You're free to reopen it later.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants