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

Proof of concept: Diagrams plugin #3250

Closed
wants to merge 1 commit into from

Conversation

edsko
Copy link

@edsko edsko commented Oct 5, 2022

This PR adds a proof-of-concept plugin that can render diagrams when hovering over a (top-level) binding of the appropriate type.

Perhaps it would be nicer to generalize this over some kind of custom Render type class, but that might get a bit messy dependency wise (where would that Render type class be defined)?

It would also be much nicer if the diagram would be shown inline as a code lens rather than on hover, but it seems this is not possible with LSP?

With many thanks to @wz1000 and @mpickering !

image

Copy link
Collaborator

@michaelpj michaelpj left a comment

Choose a reason for hiding this comment

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

Looks plausible! How's the compatibility story for diagrams?

I think hovers are the best place to put this, sadly.

Could do with some tests.

return Nothing
where
looksLikeDiagram :: IDE.Type -> Bool
looksLikeDiagram = ("Diagram" `isPrefixOf`) . IDE.printWithoutUniques
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we not at least have the qualified name here?

aux :: GHC.NoCompat.Module -> UTCTime -> GHC.Linkable
aux mod time = GHC.LM time mod []

initialiseSessionForEval ::
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can these bits be shared with the eval plugin? It looks like you already depend on it. GHC session management is fiddly at the best of times, it would be great if we can deduplicate it!

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was going to ask the same thing. The Eval plugin already contains code to do interactive evaluation, so I think it would be best if hls-diagrams-plugin reused more of it!

Example diagram
-------------------------------------------------------------------------------}

_myCircle :: Diagram B
Copy link
Collaborator

Choose a reason for hiding this comment

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

dead?

deriving stock (Show)
deriving anyclass (Exception)

newtype CatchErrors c m a = CatchErrors {
Copy link
Collaborator

Choose a reason for hiding this comment

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

would be nice to have some explanation of what this is for, etc. It looks like you're halfway to writing a generic plugin handler monad, which would be no bad thing!

Copy link
Collaborator

@pepeiborra pepeiborra left a comment

Choose a reason for hiding this comment

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

This looks good to me, modulo the code sharing with the Eval plugin. I'm happy to merge it as soon as you update the docs to list the new plugin. In particular:

  • update docs/features.md
  • update docs/supported-versions.md
  • add yourself to the CODEOWNERS file,

Thankyou!

aux :: GHC.NoCompat.Module -> UTCTime -> GHC.Linkable
aux mod time = GHC.LM time mod []

initialiseSessionForEval ::
Copy link
Collaborator

Choose a reason for hiding this comment

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

I was going to ask the same thing. The Eval plugin already contains code to do interactive evaluation, so I think it would be best if hls-diagrams-plugin reused more of it!

@edsko
Copy link
Author

edsko commented Oct 10, 2022

I'm happy to make those changes, but then we should wait for the dust to settle on #3249 first I guess.

Also, if we are going to merge this,perhaps I should make a minor modification: instead of recognizing diagrams specifically, we could instead recognize something like

-- hls:renderimage
myImage :: FilePath -> IO ()
myImage = ... rendering code ...

or some such, so that we generalize away from diagrams and can render anything that can write itself to a file? In this case of course it would be more important for people to opt-in to this; but perhaps this is too open to security exploits? (I'm happy to leave it specific to diagrams too, but some people were asking me about other kinds of rendering).

I also need to add a handle to clean up the temporary files on exit, I will do that too, along with the other suggestions.

But yes, let's wait for Matt's eval simplification PR first (this was written in parallel to that last week).

@edsko
Copy link
Author

edsko commented Oct 10, 2022

Sorry, I just realized that there is an obvious better way to do have the generalization without causing security exploits. The above signature is too general; we should just have

-- hls:renderimage
myImage :: ByteString
myImage = .. rendering code ..

or something like that.

@edsko
Copy link
Author

edsko commented Jun 5, 2023

I'll close this PR for now, since I don't really have time to update it. If someone feels inspired by it and wants to take it over, please feel free :)

@edsko edsko closed this Jun 5, 2023
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.

None yet

3 participants