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

Worksheets support for non-VS Code editors #1089

Merged
merged 3 commits into from Dec 7, 2019

Conversation

@alekseiAlefirov
Copy link
Contributor

alekseiAlefirov commented Nov 28, 2019

Current implementation of worksheets functionality (or, to be more accurate, publishing of worksheet evaluation) is VS-Code specific. This PR adds universal solution for other editors with basic LSP implementation.
worksheet-vim

The solution is using workspace/applyEdit method.

Additionally, providing package header functionality has been fixed (it also uses workspace/applyEdit). Before it failed in some editors (like Vim and Eclipse), because paths of files to edit were sent, not URIs. While regular paths are supported in, for instance, VS Code and Sublime (which is why, probably, the bug had not been spotted earlier), URI is the default parameter for WorkspaceEdit object.

@alekseiAlefirov alekseiAlefirov changed the title Worksheets fallback Worksheets support for non-VS Code editors Nov 28, 2019
@olafurpg

This comment has been minimized.

Copy link
Member

olafurpg commented Nov 28, 2019

This is exciting! You can rebase on top of master now to clean up the git commit history.

@alekseiAlefirov alekseiAlefirov force-pushed the alekseiAlefirov:worksheets-fallback branch from 83bbe4b to c157ebf Nov 28, 2019
@ayoub-benali

This comment has been minimized.

Copy link
Contributor

ayoub-benali commented Nov 28, 2019

@olafurpg what do you think about using textDocument/codeLens to display the result instead of code comment ?
According to the spec a codelens isn't required to have a Command associated with it.

@olafurpg

This comment has been minimized.

Copy link
Member

olafurpg commented Nov 28, 2019

@alekseiAlefirov how would you display multi-line output with code lenses?

Copy link
Collaborator

tgodzik left a comment

Some initial comments from me - I will have to take look a bit further, but looks great.

import org.eclipse.{lsp4j => l}
import mdoc.{interfaces => i}

object MdocToLspUtils {

This comment has been minimized.

Copy link
@tgodzik

tgodzik Nov 28, 2019

Collaborator

There might be some already existing helper method in MetalsExtension, which are brought out by:
import scala.meta.internal.metals.MetalsEnrichments._

This comment has been minimized.

Copy link
@alekseiAlefirov

alekseiAlefirov Nov 29, 2019

Author Contributor

this whole file is just moved code from WorksheetProvider.scala.
You suggest to make these methods a partd of MetalsEnrichments?

This comment has been minimized.

Copy link
@tgodzik

tgodzik Nov 29, 2019

Collaborator

Hmm... might not be that useful there since it will only be in worksheets.

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 6, 2019

Member

Let's rename this file to MdocEnrichments

This comment has been minimized.

Copy link
@alekseiAlefirov

alekseiAlefirov Dec 6, 2019

Author Contributor

Should it be moved from .../internal/worksheets/ to .../internal/metals?

@@ -177,7 +177,7 @@ class MetalsLanguageServer(
private var doctor: Doctor = _
var httpServer: Option[MetalsHttpServer] = None
var treeView: TreeViewProvider = NoopTreeViewProvider
var worksheetProvider: Option[WorksheetProvider] = None
var worksheetProvider: WorksheetProvider = _

This comment has been minimized.

Copy link
@tgodzik

tgodzik Nov 28, 2019

Collaborator

Let's go back to Option[WorksheetProvider]

This comment has been minimized.

Copy link
@alekseiAlefirov

alekseiAlefirov Nov 29, 2019

Author Contributor

why? Previously Option made sense, since there would not be a Provider always, and now we have fallback.

This comment has been minimized.

Copy link
@tgodzik

tgodzik Nov 29, 2019

Collaborator

Ok, right. Haven't thought about it!

@tgodzik

This comment has been minimized.

Copy link
Collaborator

tgodzik commented Nov 28, 2019

@alekseiAlefirov how would you display multi-line output with code lenses?

I think the issue was that lenses are not published - so one would need to reload the file to see the results.

We would need to do some additional work to send a refresh command to the client.

@marek1840 Can provide some more info on that.

@ayoub-benali

This comment has been minimized.

Copy link
Contributor

ayoub-benali commented Nov 28, 2019

Good point, the code lens need to be requested by the client on each save :/

@tgodzik

This comment has been minimized.

Copy link
Collaborator

tgodzik commented Nov 28, 2019

Issue with cross tests is known - I am fixing it in a separate PR.

@tgodzik

This comment has been minimized.

Copy link
Collaborator

tgodzik commented Nov 28, 2019

Good point, the need to be requested by the client on each save :/

We could possibly just wait until the results are ready and then publish lenses - but not sure about timeouts there.

@marek1840

This comment has been minimized.

Copy link
Collaborator

marek1840 commented Nov 29, 2019

I think the issue was that lenses are not published - so one would need to reload the file to see the results.
We would need to do some additional work to send a refresh command to the client.
@marek1840 Can provide some more info on that.

We are already forcing the client (currently VS code only) to request code lenses after each compilation.
You can do the same by sending the MetalsLanguageClient::refreshModel notification to the client whenever you feel it should refresh the code lenses.

@ckipp01

This comment has been minimized.

Copy link
Member

ckipp01 commented Nov 29, 2019

First of all, this is incredible. I'm so happy to see this supported in other editors. While testing it in Vim with coc.nvim there are a few things that I think are a bit odd behavior. For example if you have a long return that is printed out which puts text in ranging the next 50 lines, and then you delete the original snippet, you commented code that was entered remains.

2019-11-29 09 20 45

I also think this is a bit odd behavior when you for example run the same snippet as above, and then try to move down to the next line, you're still in the comment and the other commented out evaluated code remains. It makes sense why this is happening, but as a user without context, I wouldn't want this.

Screenshot 2019-11-29 at 09 22 37

This works fantastic when the evaluated code can be shown on a single line, but I feel like we need to address what happens on long spanning lines of evaluation.

@olafurpg

This comment has been minimized.

Copy link
Member

olafurpg commented Nov 29, 2019

@ckipp01 that's a good point! Do you think the UX would be more acceptable if comments were formatted like this instead?

val y = 1.to(100) //> y: Range.Inclusive = RangeInclusive
                  //>   1,
                  //>   2,
                  //> ...
@ckipp01

This comment has been minimized.

Copy link
Member

ckipp01 commented Nov 29, 2019

Do you think the UX would be more acceptable if comments were formatted like this instead?

Maybe? I think from a UX perspective I think the clearer we can make it that this is a comment and will remain the better. I do think you example makes that a bit clearer. It would also allow you to just a line below, hit save, and then everything will continue to work and be re-evaluated.

@tgodzik

This comment has been minimized.

Copy link
Collaborator

tgodzik commented Nov 29, 2019

I think the issue was that lenses are not published - so one would need to reload the file to see the results.
We would need to do some additional work to send a refresh command to the client.
@marek1840 Can provide some more info on that.

We are already forcing the client (currently VS code only) to request code lenses after each compilation.
You can do the same by sending the MetalsLanguageClient::refreshModel notification to the client whenever you feel it should refresh the code lenses.

I think we need first to support it in other clients and then we can switch to lenses. Also, workspace edit seems to be supported in more editors.

@olafurpg

This comment has been minimized.

Copy link
Member

olafurpg commented Nov 29, 2019

I am concerned about the following situation

// step 1
val x = 1.to(100) //> y: Range.Inclusive = RangeInclusive
                  //>   1,
                  //>   2,
                  //> ...

// step 2 (before save)
val x = 1.to(100) //> y: Range.Inclusive = RangeInclusive
val y = 1.to(100) // (unsaved)
                  //>   1,
                  //>   2,
                  //> ...

// step 3 (after save)
val x = 1.to(100) //> y: Range.Inclusive = RangeInclusive
                  //>   1,
                  //>   2,
                  //> ...
val y = 1.to(100) //> y: Range.Inclusive = RangeInclusive
                  //>   1,
                  //>   2,
                  //> ...

I wonder if it would be better to keep single line output and display the multiline output via textDocument/hover over the comment 🤔

@ayoub-benali

This comment has been minimized.

Copy link
Contributor

ayoub-benali commented Nov 29, 2019

Also, workspace edit seems to be supported in more editors.

Indeed, for example code lens isn't yet supported in sublime sublimelsp/LSP#781

@tgodzik

This comment has been minimized.

Copy link
Collaborator

tgodzik commented Nov 29, 2019

I wonder if it would be better to keep single line output and display the multiline output via textDocument/hover over the comment

I think it's a good idea - would limit the issues user might have with typing and should be a better UX overall.

@olafurpg

This comment has been minimized.

Copy link
Member

olafurpg commented Nov 29, 2019

To implement the textDocument/hover idea, we can keep a map in WorksheetProvider of hover messages for each worksheet. Something like this:

case class HoverMessage(range: l.Range, message: String)
case class HoverMessages(text: String, hovers: Seq[HoverMessage])
val hoverMessages: Map[Path, HoverMessages]

Then in the implementation of textDocument/hover, we could query the worksheet provider as a fallback when Compilers.hover returns an empty result. We would need to use token edit distance to make sure the positions of the hover message continue to cover the positions of the comments as the user makes changes in the worksheet.

@ckipp01

This comment has been minimized.

Copy link
Member

ckipp01 commented Nov 29, 2019

I wonder if it would be better to keep single line output and display the multiline output via textDocument/hover over the comment

What symbol would you execute the hover on in order to see the output? I'm confused about how that would work. Like if you had this:

val x = 1.to(100)

What would you hover on to see the evaluated result? I ask because I'd expect my hover on x would still show the Int information and I could do a hover on to, so then it might be odd as a vim user to know where to execute the hover.

@tgodzik

This comment has been minimized.

Copy link
Collaborator

tgodzik commented Nov 29, 2019

to see the evaluated result? I ask because I'd expect my hover on x would still show the Int information and I could do a hover on to, so then it might be odd as a vim user to know where to execute the hover.

Maybe add ellipsis at the end (...) ?

val x = 1.to(100) //> y: Range.Inclusive = RangeInclusive (...)

And wherever you hover on the comment you get the full output.

@olafurpg

This comment has been minimized.

Copy link
Member

olafurpg commented Nov 29, 2019

@ckipp01 you would need to hover on the //> Range.Inclusive comment

@tgodzik we already truncate the output with decorations in VS Code. There's EvaluatedWorksheetStatement.{summary,details}, see https://github.com/scalameta/mdoc/blob/ae7613dcad4b39855a69e2c7139ca8e39dba52e3/mdoc-interfaces/src/main/scala/mdoc/interfaces/EvaluatedWorksheetStatement.java#L6-L7

@ckipp01

This comment has been minimized.

Copy link
Member

ckipp01 commented Nov 29, 2019

@ckipp01 you would need to hover on the //> Range.Inclusive comment

duh, of course 😅

Maybe add ellipsis at the end (...) ?

I like this

@alekseiAlefirov

This comment has been minimized.

Copy link
Contributor Author

alekseiAlefirov commented Nov 29, 2019

First of all, this is incredible. I'm so happy to see this supported in other editors. While testing it in Vim with coc.nvim there are a few things that I think are a bit odd behavior. For example if you have a long return that is printed out which puts text in ranging the next 50 lines, and then you delete the original snippet, you commented code that was entered remains.

2019-11-29 09 20 45

I also think this is a bit odd behavior when you for example run the same snippet as above, and then try to move down to the next line, you're still in the comment and the other commented out evaluated code remains. It makes sense why this is happening, but as a user without context, I wouldn't want this.

Screenshot 2019-11-29 at 09 22 37

This works fantastic when the evaluated code can be shown on a single line, but I feel like we need to address what happens on long spanning lines of evaluation.

@ckipp01
yep, good point. thank you. I will try to do something with hover.

@olafurpg so I'm going to use short message from mdoc for the comment and detailed for the hover.
I suppose, for that I'd need to fix clean a bit output of mdoc ("//" at the beginning of decoration) as a PR to Mdoc; or I can just remove them on Metals side, like I actually do now with the newline of detailed message...
Also can be useful to fix Mdoc to let Metals know, if short message is actually not complete, hence ... should be put there 🤔

@olafurpg

This comment has been minimized.

Copy link
Member

olafurpg commented Nov 29, 2019

@alekseiAlefirov feel free to send a PR extending mdoc to include the missing information, it would be good to remove the "// " prefix and include whether the summary has been truncated. We can cut a new release of mdoc as soon as your PR is merged

@alekseiAlefirov

This comment has been minimized.

Copy link
Contributor Author

alekseiAlefirov commented Nov 29, 2019

@olafurpg , what's text: String here for?

case class HoverMessages(text: String, hovers: Seq[HoverMessage])

upd. ah, I get it, it must be a snapshot to evaluate token distance.

@olafurpg

This comment has been minimized.

Copy link
Member

olafurpg commented Nov 29, 2019

@alekseiAlefirov the text contents of the document after applying the worksheet decorations comments. This is needed to run token edit distance on hover to adjust the hover positions to the latest changes in the document

@olafurpg

This comment has been minimized.

Copy link
Member

olafurpg commented Dec 3, 2019

mdoc v2.0.3 has been released with your changes @alekseiAlefirov

@alekseiAlefirov

This comment has been minimized.

Copy link
Contributor Author

alekseiAlefirov commented Dec 5, 2019

Fixed to have one-line summary comments and hovers.
(*) Screen width might need to be thought about (same issue for VS Code and decorations)
(**) Not Vim (Sublime), because it seems like it does not pull textDocument/hover (but definition in appropriate cases).
hovers

@ckipp01

This comment has been minimized.

Copy link
Member

ckipp01 commented Dec 6, 2019

(**) Not Vim (Sublime), because it seems like it does not pull textDocument/hover (but definition in appropriate cases).

What wasn't working for you in vim? Just testing this out now, and it is working fantastically

2019-12-06 06 50 27

@alekseiAlefirov

This comment has been minimized.

Copy link
Contributor Author

alekseiAlefirov commented Dec 6, 2019

What wasn't working for you in vim?

I was trying :LspHover (not really experienced Vim user 😅 ). Thank you for checking this out!

Copy link
Member

olafurpg left a comment

Amazing job @alekseiAlefirov ! Just a few minor nitpick comments, otherwise I believe this PR is ready to be merged. Feel free to remove the "Draft"


object WorkspaceEditWorksheetPublisher {

case class HoverMessage(range: Range, message: String)

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 6, 2019

Member

let's move these to the toplevel, we're already in the worksheets package.

This comment has been minimized.

Copy link
@alekseiAlefirov

alekseiAlefirov Dec 6, 2019

Author Contributor

but RenderResult does not really make sense outside of the class, don't you think?

path: AbsolutePath,
worksheet: EvaluatedWorksheet
): Unit = {
(render(path) _ andThen publish(path, languageClient))(worksheet)

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 6, 2019

Member

nit: let's avoid infix operations a op b for non-symbol operators. I'm not sure what the correct refactoring is but something like this below would be easier to read

Suggested change
(render(path) _ andThen publish(path, languageClient))(worksheet)
publish(path, languageClient, render(path, worksheet))

This comment has been minimized.

Copy link
@alekseiAlefirov

alekseiAlefirov Dec 6, 2019

Author Contributor

done. Named rendered result and then passed it to publish

import org.eclipse.{lsp4j => l}
import mdoc.{interfaces => i}

object MdocToLspUtils {

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 6, 2019

Member

Let's rename this file to MdocEnrichments

@alekseiAlefirov alekseiAlefirov force-pushed the alekseiAlefirov:worksheets-fallback branch from c3eb69b to c2b3104 Dec 6, 2019
@alekseiAlefirov

This comment has been minimized.

Copy link
Contributor Author

alekseiAlefirov commented Dec 6, 2019

@olafurpg fixes introduced, except for this #1089 (comment) - just comment, what you think, and I fix it, if needed.

@alekseiAlefirov alekseiAlefirov marked this pull request as ready for review Dec 6, 2019
Copy link
Member

olafurpg left a comment

LGTM 👍 I'm super happy that we can support worksheets in all editors! 😊

@ckipp01
ckipp01 approved these changes Dec 6, 2019
Copy link
Member

ckipp01 left a comment

Really excited to see this get merged!

LGTM 👍

@olafurpg olafurpg merged commit f463c1b into scalameta:master Dec 7, 2019
10 of 11 checks passed
10 of 11 checks passed
ubuntu-latest tests
Details
windows-latest tests windows-latest tests
Details
macOS-latest tests
Details
Sbt integration
Details
Maven integration
Details
Gradle integration
Details
Mill integration
Details
Pants integration
Details
Slow tests
Details
Scala cross tests
Details
Scalafmt/Scalacheck/Docs
Details
@alekseiAlefirov alekseiAlefirov deleted the alekseiAlefirov:worksheets-fallback branch Dec 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.