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

adds hover support from the engine #3

Merged
merged 1 commit into from
Mar 26, 2023

Conversation

astrale-sharp
Copy link
Collaborator

adds hovering support, some repetition with my last pull request that I can factor out once it is merged, I can also rebase after you pulled

@astrale-sharp
Copy link
Collaborator Author

this is now rebased on the other pull request and shouldn't be merged first

src/main.rs Outdated
Comment on lines 11 to 20
use typst::ide::{tooltip, Tooltip};
use typst::syntax::LinkedNode;
use typst::World;
Copy link
Owner

Choose a reason for hiding this comment

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

Move all use above mod

async fn hover(&self, params: HoverParams) -> Result<Option<Hover>> {
let world = self.world.read().await;
let world = world.as_ref().unwrap();
let source = world.main();
Copy link
Owner

Choose a reason for hiding this comment

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

We should get the current document from params; the current main thing is a hack and will eventually be removed.

Copy link
Owner

Choose a reason for hiding this comment

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

I think I forgot how my own hack works; this being hard to do is why main exists. This is actually fine.

@astrale-sharp astrale-sharp force-pushed the hovering branch 4 times, most recently from dadbbdf to bce8f98 Compare March 23, 2023 15:40
@astrale-sharp astrale-sharp marked this pull request as draft March 23, 2023 15:42
@astrale-sharp astrale-sharp marked this pull request as ready for review March 23, 2023 15:46
@astrale-sharp
Copy link
Collaborator Author

Okay, could be ready to go, let me know!

src/main.rs Outdated

let cursor = get_cursor_for_position(params.text_document_position_params.position, source);

if let Some(cursor) = cursor {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm just dreaming of chain if let tbh

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I could use if_chain! if you'd like !

Copy link
Contributor

Choose a reason for hiding this comment

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

alternatively you could do

let Some(cursor) = cursor else { return Ok(None) };
let Some(tooltip) = tooltip(world, &[], source, cursor) else { return Ok(None) };

let Some(lk) = lk else { return Ok(None) };

I personally don't really like if_chain because they sometimes mess with rust-analyzers ability to go to definition and autocomplete.

Copy link
Owner

Choose a reason for hiding this comment

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

@astrale-sharp can you implement something like @jakobhellermann's idea here? It will keep the code from getting too deeply nested.

@astrale-sharp
Copy link
Collaborator Author

this works pretty well, I tried rebased on your branch but it shows little moves, that's annoying. I'm a bit stuck on what to rebase on what.
Apart from that it works pretty good, shows doc for builtins, shows values for variables.

Copy link
Owner

@nvarner nvarner left a comment

Choose a reason for hiding this comment

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

I'm planning to merge this ASAP; just one change and it looks good to go (plus a rebase, but I can probably do that unless we get conflicts)

src/main.rs Outdated

let cursor = get_cursor_for_position(params.text_document_position_params.position, source);

if let Some(cursor) = cursor {
Copy link
Owner

Choose a reason for hiding this comment

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

@astrale-sharp can you implement something like @jakobhellermann's idea here? It will keep the code from getting too deeply nested.

@astrale-sharp astrale-sharp force-pushed the hovering branch 4 times, most recently from 9b806d9 to 9defdfc Compare March 26, 2023 15:14
@astrale-sharp
Copy link
Collaborator Author

This was such a pain to rebase and I have no idea why but it's finally done (I ended up copy pasting from your master branch)

I love that last change you made me do, I didn't know we could do that !

It should be ready to go,

@astrale-sharp astrale-sharp requested a review from nvarner March 26, 2023 15:15
Copy link
Owner

@nvarner nvarner left a comment

Choose a reason for hiding this comment

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

Looks great now, and this should be a useful feature; thank you!

The difficulty rebasing is probably because a) there have been a lot of changes from all the PRs recently, and b) I mangled the git history in an attempt to merge them all quickly (so sorry about that). Future rebases will hopefully be easier.

@nvarner nvarner merged commit 0700028 into nvarner:master Mar 26, 2023
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.

3 participants