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

Feature/nested-outline #593

Closed
wants to merge 19 commits into from
Closed

Conversation

kv9898
Copy link
Contributor

@kv9898 kv9898 commented Oct 17, 2024

Fixes posit-dev/positron#3822.

With the following code:

# first level ####
a <- 1 # missing symbol
###########################
## second level ####
### third level####
some_function <- function(x){
  b <- 2
  # inside first level ####
  ## inside second level####
}

The outline is now displayed as:

image

@kv9898
Copy link
Contributor Author

kv9898 commented Oct 17, 2024

We're still missing is the foldable comment feature - I guess changes to multiple files are needed for this functionality and I'm afraid of writing something difficult for you to maintain in that case.

Copy link
Contributor

@lionel- lionel- left a comment

Choose a reason for hiding this comment

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

Good first stab! This will be such a nice improvement.

Unfortunately we can't use unsafe code in this way, and especially not if safety is not established from a thorough analysis.

It seems it should be possible to move the comment section handling inside the loop over the children of a node. And swap out the current parent there depending on the level. Then you shouldn't need to pass this stack around and deal with the ensuing reference lifetime issues.

WDYT?

// Check if the comment starts with one or more '#' followed by any text and ends with 4+ punctuations
if let Some((_level, title)) = parse_comment_as_section(&comment_text) {
// Create a symbol based on the parsed comment
if let Some((level, title)) = parse_comment_as_section(&comment_text) {
Copy link
Contributor

Choose a reason for hiding this comment

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

At this point it seems like this large handling block should become its own method, to keep index_node() readable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. I'm not sure what parameters we should pass to the new method. It seems that passing things around in Rust is not easy.

crates/ark/src/lsp/symbols.rs Outdated Show resolved Hide resolved
crates/ark/src/lsp/symbols.rs Outdated Show resolved Hide resolved

// Push the new symbol to the current parent
if let Some((_, current_parent_ptr)) = section_stack.last() {
let current_parent = unsafe { &mut **current_parent_ptr };
Copy link
Contributor

Choose a reason for hiding this comment

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

We're going to have to find another way. I don't think we can afford unsafe sections in this code, and we haven't established safety here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right! Just didn't work out how to do it in a safe way.

@kv9898
Copy link
Contributor Author

kv9898 commented Oct 17, 2024

Good first stab! This will be such a nice improvement.

Unfortunately we can't use unsafe code in this way, and especially not if safety is not established from a thorough analysis.

It seems it should be possible to move the comment section handling inside the loop over the children of a node. And swap out the current parent there depending on the level. Then you shouldn't need to pass this stack around and deal with the ensuing reference lifetime issues.

WDYT?

I have moved the comment handling to the loop. Now it should look closer to our main version now. However, I'm still struggling to get around the "unsafe" problem. There remain 2 unsafe references. @lionel- any suggestions, feel free to overwrite my code?

return Ok(true);
// Push the new symbol to the current parent
if let Some((_, current_parent_ptr)) = section_stack.last() {
let current_parent = unsafe { &mut **current_parent_ptr };
Copy link
Contributor Author

Choose a reason for hiding this comment

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

1st unsafe reference

}
} else {
if let Some((_, current_parent_ptr)) = section_stack.last() {
let current_parent = unsafe { &mut **current_parent_ptr };
Copy link
Contributor Author

Choose a reason for hiding this comment

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

2nd unsafe reference

@kv9898 kv9898 requested a review from lionel- October 18, 2024 17:44
@lionel- lionel- mentioned this pull request Oct 21, 2024
@kv9898
Copy link
Contributor Author

kv9898 commented Oct 28, 2024

Closed as refactored to #611.

@kv9898 kv9898 closed this Oct 28, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Oct 28, 2024
@kv9898 kv9898 deleted the feature/nested-outline branch October 30, 2024 11:04
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.

Outline view: Support nested sections in R scripts
2 participants