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

feat(lsp): Implement textDocument/foldingRange #9900

Merged
merged 8 commits into from
Apr 2, 2021

Conversation

jeanp413
Copy link
Contributor

Related #8643

@kitsonk kitsonk self-requested a review March 26, 2021 04:48
cli/lsp/tsc.rs Outdated
Comment on lines 1283 to 1307
fn adjust_folding_end_line(
&self,
range: &lsp::Range,
line_index: &LineIndex,
text_content: &str,
line_folding_only: bool,
) -> u32 {
if line_folding_only && range.end.character > 0 {
let offset_start: usize = line_index
.offset(lsp::Position {
character: range.end.character - 1,
..range.end
})
.unwrap()
.into();
let offset_end: usize = line_index.offset(range.end).unwrap().into();
let fold_end_char: &str =
unsafe { text_content.get_unchecked(offset_start..offset_end) };
if FOLD_END_PAIR_CHARACTERS.contains(&fold_end_char) {
return cmp::max(range.end.line - 1, range.start.line);
}
}

range.end.line
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This was a really ugly hack in TypeScript to work around an issue, and it is even uglier in Rust. The original issue this was added for: microsoft/vscode#47240 appears to be a hack to workaround some folding issues with tsc. Personally I would like to try to remove this and see if the example mentioned in the original issue still persists in recent versions of tsc.

If it is still a problem, I would really like to find a safe way to basically get a single character at a position within a string and then compare that to a vector of static chars, instead of this. All we want is the end offset - 1 and the end offset.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is still needed as vscode only supports line folding (lineFoldingOnly is set to true) and typescript returns line and column

/**
 * If set, the client signals that it only supports folding complete lines.
 * If set, client will ignore specified `startCharacter` and `endCharacter`
 * properties in a FoldingRange.
 */
lineFoldingOnly?: boolean;

I would really like to find a safe way to basically get a single character at a position within a string and then compare that to a vector of static chars, instead of this. All we want is the end offset - 1 and the end offset.

This is what I'm trying to do here extracting a valid utf8 code point but I'll try using as_bytes as we are just looking for ASCII characters. I'm just learning rust so if there is a better way let me know.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you don't mind, I will take an attempt at it. I'm still learning Rust too, but might have been learning it a bit longer. 😁

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure go ahead 👍
In my last commit I managed to reduce it to

let offset_end: usize = line_index.offset(range.end).unwrap().into();
let fold_end_char = text_content.as_bytes()[offset_end - 1];

@kitsonk kitsonk mentioned this pull request Apr 2, 2021
43 tasks
Copy link
Contributor

@kitsonk kitsonk left a comment

Choose a reason for hiding this comment

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

LGTM, thank you again for a great contribution.

@kitsonk kitsonk merged commit 035f7b0 into denoland:main Apr 2, 2021
@jeanp413 jeanp413 deleted the lsp-folding-range branch April 2, 2021 14:59
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.

2 participants