improvement: Use inlay hints for worksheets #6827

tgodzik · 2024-10-07T21:29:17Z

Advantages:

this would allow us to stop using a custom extension and support more editors such as Zed
can reuse any new enhancement to inlay hints

Disadvantages:

no way to set color
could mix with normal decorations

I plan on removing decoration protocol and worksheet fallback in this PR

TODO

remove worksheet publishers
add specific test to using inlay hints

tgodzik · 2024-11-17T16:07:52Z

@ckipp01 do you know if inlay hints would work ok for nvim-metals? I want to remove as many custom extensions as I can since we get more and more editors and implementing those custom extension might be too much of a hassle.

ckipp01 · 2024-11-18T10:07:31Z

@ckipp01 do you know if inlay hints would work ok for nvim-metals?

Yup, inlay hints are supported in nvim, so this should work. If I get a bit of time I'll try this pr out with nvim-metals and see if I hit on any issues.

tgodzik · 2024-11-18T10:11:57Z

@ckipp01 do you know if inlay hints would work ok for nvim-metals?

Yup, inlay hints are supported in nvim, so this should work. If I get a bit of time I'll try this pr out with nvim-metals and see if I hit on any issues.

Thanks! I think I mostly need to fix tests here. I will also see how Zed handles it since I managed to get that set up for myself.

dragove · 2024-11-19T04:29:27Z

I did some test on nvim-metals. here are my feedbacks.

Evaluation results showed using inlay hints style as expected.
Adding a character to make code not compilable and the inlay hints not go away (while vscode only shows the error and hindes the inlay hints)
Deleting the character to make code compilable again then I can not see the evaluation results.
use :e to reload the file things get working again.

There is one thing should be mentioned. Using inlay hints also means disabling editor's inlay hints feature will cause results not showing. Example configs are:

vscode's "editor.inlayHints.enabled": "off"
neovim's vim.lsp.inlay_hint.enable(false)

tgodzik · 2024-11-19T11:52:29Z

Adding a character to make code not compilable and the inlay hints not go away (while vscode only shows the error and hindes the inlay hints)

Looks like maybe it's because of lacking inlayHints/refresh ? Does neovim support that?

tgodzik · 2024-11-19T11:53:30Z

There is one thing should be mentioned. Using inlay hints also means disabling editor's inlay hints feature will cause results not showing. Example configs are:

Sure, but it's enabled by default (at least in VS Code) and other editor users might be sued to turning things on. We need to write about it in the release notes for sure.

Advantages: - this would allow us to stop using a custom extension and support more editors such as Zed - can reuse any new feature inlay hints add - reevaluate on change? (or should we remove it?) Disadvantages: - no way to set color - could mix with normal decorations TODO: - [ ] gather feedback - [ ] fix tests - [ ] check if we can remove other ways of publishing altogether

kasiaMarek · 2024-12-09T09:47:11Z

metals-docs/src/main/scala/docs/WorksheetModifier.scala

+        "as a inlay hint at the end of the line." ->
+          "hover on the inlay hint to expand it."


Won't it be more editors than only vscode that support inline hints? Also aren't worksheet decorations using comments not supported after this PR?

kasiaMarek · 2024-12-09T09:58:09Z

metals/src/main/scala/scala/meta/internal/metals/InlayHintResolveProvider.scala

-      getLabelParts(inlayHint).zip(parseData(data))
+      Option(inlayHint.getData()) match {
+        case Some(data: JsonArray) =>
+          resolve(


Why did you move resolve to try?

kasiaMarek · 2024-12-09T10:00:30Z

metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala

        .map(_.map(_.toLsp()))
        .map(
-          _.orElse {
-            val path = params.textDocument.getUri.toAbsolutePath
-            if (path.isWorksheet)
-              worksheetProvider.hover(path, params.getPosition)
-            else
-              None
-          }.orNull
+          _.orNull


.map(_.map(_.toLsp()).orNull)

kasiaMarek · 2024-12-09T10:02:57Z

metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala

@@ -1016,7 +997,11 @@ abstract class MetalsLspService(
          if (userConfig.areSyntheticsEnabled())
            compilers.inlayHints(params, token)
          else Future.successful(List.empty[l.InlayHint].asJava)
-      } yield hints
+        worksheet <- worksheetProvider.inlayHints(
+          params.getTextDocument().getUri().toAbsolutePath,


Suggested change

params.getTextDocument().getUri().toAbsolutePath,

params.getTextDocument().getUri().toAbsolutePathSafe,

kasiaMarek · 2024-12-09T13:11:09Z

metals/src/main/scala/scala/meta/internal/worksheets/WorksheetProvider.scala

+            distance.toRevised(statEnd) match {
+              case Left(_) =>
+              case Right(right) =>
+                statEnd.setLine(right.startLine)
+                statEnd.setCharacter(right.startColumn)
+            }


Suggested change

distance.toRevised(statEnd) match {

case Left(_) =>

case Right(right) =>

statEnd.setLine(right.startLine)

statEnd.setCharacter(right.startColumn)

}

distance.toRevised(statEnd).foreach { right =>

statEnd.setLine(right.startLine)

statEnd.setCharacter(right.startColumn)

}

kasiaMarek · 2024-12-09T13:16:32Z

tests/unit/src/main/scala/tests/BaseWorksheetLspSuite.scala

-      _ = client.onWorkDoneProgressStart = (_, _) => {}
+      _ <- cancelled.future.withTimeout(10.seconds)
+      _ = client.onWorkDoneProgressStart = (message, _) => {
+        scribe.info(message)


kasiaMarek · 2024-12-09T13:16:42Z

tests/unit/src/main/scala/tests/BaseWorksheetLspSuite.scala

@@ -266,6 +208,7 @@ abstract class BaseWorksheetLspSuite(
    cleanWorkspace()
    val cancelled = Promise[Unit]()
    client.onWorkDoneProgressStart = { (message, cancelParams) =>
+      scribe.info(message)


tgodzik force-pushed the switch-inlay-hints branch from 5e92f76 to b311f11 Compare November 17, 2024 16:03

tgodzik force-pushed the switch-inlay-hints branch 3 times, most recently from 8084e55 to 4994a1f Compare November 17, 2024 17:14

tgodzik force-pushed the switch-inlay-hints branch 2 times, most recently from 2180ce1 to 00e1925 Compare November 20, 2024 10:18

tgodzik added 2 commits December 1, 2024 20:58

test: Fix worksheet tests after changes

3aea43d

tgodzik force-pushed the switch-inlay-hints branch 2 times, most recently from fb1cc3d to 476256a Compare December 7, 2024 19:16

tgodzik force-pushed the switch-inlay-hints branch from 476256a to ec2a1b6 Compare December 8, 2024 20:31

tgodzik marked this pull request as ready for review December 9, 2024 08:54

tgodzik requested a review from kasiaMarek December 9, 2024 08:54

tgodzik force-pushed the switch-inlay-hints branch from ec2a1b6 to d60c695 Compare December 9, 2024 09:02

refactor: Remove decoration protocol

1f0aa91

tgodzik force-pushed the switch-inlay-hints branch from d60c695 to 1f0aa91 Compare December 9, 2024 10:06

kasiaMarek approved these changes Dec 9, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improvement: Use inlay hints for worksheets #6827

improvement: Use inlay hints for worksheets #6827

tgodzik commented Oct 7, 2024 •

edited

Loading

tgodzik commented Nov 17, 2024

ckipp01 commented Nov 18, 2024

tgodzik commented Nov 18, 2024

dragove commented Nov 19, 2024

tgodzik commented Nov 19, 2024

tgodzik commented Nov 19, 2024

kasiaMarek Dec 9, 2024

kasiaMarek Dec 9, 2024

kasiaMarek Dec 9, 2024

kasiaMarek Dec 9, 2024

kasiaMarek Dec 9, 2024

kasiaMarek Dec 9, 2024

kasiaMarek Dec 9, 2024

		"as a inlay hint at the end of the line." ->
		"hover on the inlay hint to expand it."

	params.getTextDocument().getUri().toAbsolutePath,
	params.getTextDocument().getUri().toAbsolutePathSafe,

improvement: Use inlay hints for worksheets #6827

Are you sure you want to change the base?

improvement: Use inlay hints for worksheets #6827

Conversation

tgodzik commented Oct 7, 2024 • edited Loading

tgodzik commented Nov 17, 2024

ckipp01 commented Nov 18, 2024

tgodzik commented Nov 18, 2024

dragove commented Nov 19, 2024

tgodzik commented Nov 19, 2024

tgodzik commented Nov 19, 2024

kasiaMarek Dec 9, 2024

Choose a reason for hiding this comment

kasiaMarek Dec 9, 2024

Choose a reason for hiding this comment

kasiaMarek Dec 9, 2024

Choose a reason for hiding this comment

kasiaMarek Dec 9, 2024

Choose a reason for hiding this comment

kasiaMarek Dec 9, 2024

Choose a reason for hiding this comment

kasiaMarek Dec 9, 2024

Choose a reason for hiding this comment

kasiaMarek Dec 9, 2024

Choose a reason for hiding this comment

tgodzik commented Oct 7, 2024 •

edited

Loading