Replace `slowTask` with LSP progress #6144

kasiaMarek · 2024-02-20T16:00:59Z

resolves: scalameta/metals-feature-requests#374
resolves: #6099
resolves: #6187

kasiaMarek · 2024-02-21T14:28:20Z

metals/src/main/scala/scala/meta/internal/metals/debug/DebugProvider.scala

-
-        connWithTimeout
+        conn
+          .withTimeout(60, TimeUnit.SECONDS)


There doesn't seem to exist a capability for supporting cancel on workDoneProgress.

Could you explain what you mean with this? There should always be a taskStart associated with a progress right, and it would just be attached to that?

The logic in here was: if client supports slowTask (thus cancel) do not add a timeout to creating connection (user can cancel it), otherwise add timeout. Now we switch to lsp workDoneProgress. Problem is that even if client supports LSP workDoneProgress it doesn't mean they support cancel for workDoneProgress, LSP specification basically says that client can ignore the cancellation option in workDoneProgress if it's not supported. So I decided that the safest option is to always add the timeout.

ahh ok that makes sense. 👍🏼

ckipp01 · 2024-03-01T15:01:46Z

metals/src/main/scala/scala/meta/internal/bsp/BspConnector.scala

@@ -36,6 +37,7 @@ class BspConnector(
    tables: Tables,
    userConfig: () => UserConfiguration,
    statusBar: StatusBar,


We don't use this in generateBspConfig anymore so you can remove it there and in here as well.

Can we also change to workDoneProgress in ScalaCliBuildTool?

It might finish too fast for it make much sense but I can do that.

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

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

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

ckipp01 · 2024-03-01T15:23:35Z

metals/src/main/scala/scala/meta/internal/metals/debug/DebugProvider.scala

-
-        connWithTimeout
+        conn
+          .withTimeout(60, TimeUnit.SECONDS)


Could you explain what you mean with this? There should always be a taskStart associated with a progress right, and it would just be attached to that?

jkciesluk

It works great, good job!

metals/src/main/scala/scala/meta/internal/builds/ShellRunner.scala

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

tgodzik · 2024-04-02T14:29:51Z

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

+  private class Compilation(
+      val timer: Timer,
+      val isNoOp: Boolean,
+      token: Option[Future[WorkDoneProgress.Token]],


Suggested change

token: Option[Future[WorkDoneProgress.Token]],

token: Option[WorkDoneProgress.Token],

could we avoid having a future here? We could probably change startProgress to be generic?

It's just to make sure that progress was actually created on client side before we send e.g. end. What I was worried about is if we send end way too fast, before client creates the token for it, so end will be ignored and the progress bar will just be visible forever. I can change this to a Promise if that helps.

We could probably change startProgress to be generic?

I'm not sure I understand.

I'm not sure I understand.

I don't remember what I had in mind 😅

Maybe we could have a sealed hierachy instead, soemthing like:

sealed trait Compilation{ def timer: Timer def taskProgress: TaskProgress } case class NoopCopilation( val timer: Timer, ){ override def taskProgress: TaskProgress = TaskProgress.empty, } case class IncrementalCompilation( val timer: Timer, token: Future[WorkDoneProgress.Token], taskProgress: TaskProgress )

what do you think?

I think that after @ckipp01's changes in Bloop we need a noop anymore, Bloop won't send startTask for it anymore.

probably not, so maybe we can just do:

case class Compilation( val timer: Timer, token: Future[WorkDoneProgress.Token], taskProgress: TaskProgress )

?

tgodzik · 2024-04-02T14:31:56Z

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

+    token.map { token =>
+      val task = taskMap.getOrDefault(token, Task.empty)
+      task.maybeProgress match {
+        case Some(progress) =>
+          progress.update(percentage)
+          val report = new WorkDoneProgressReport()
+          report.setPercentage(progress.percentage)
+          task.additionalMessage.foreach(report.setMessage)
+          val notification =
+            messages.Either.forLeft[WorkDoneProgressNotification, Object](
+              report
+            )
+          client.notifyProgress(new ProgressParams(token, notification))
+        case None =>
+      }


Suggested change

token.map { token =>

val task = taskMap.getOrDefault(token, Task.empty)

task.maybeProgress match {

case Some(progress) =>

progress.update(percentage)

val report = new WorkDoneProgressReport()

report.setPercentage(progress.percentage)

task.additionalMessage.foreach(report.setMessage)

val notification =

messages.Either.forLeft[WorkDoneProgressNotification, Object](

report

)

client.notifyProgress(new ProgressParams(token, notification))

case None =>

}

token.map(notifyProgress)

?

tgodzik

Looks good! Just two minor comments

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

tgodzik · 2024-04-16T16:17:50Z

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

+  private class Compilation(
+      val timer: Timer,
+      val isNoOp: Boolean,
+      token: Option[Future[WorkDoneProgress.Token]],


I'm not sure I understand.

I don't remember what I had in mind 😅

Maybe we could have a sealed hierachy instead, soemthing like:

sealed trait Compilation{ def timer: Timer def taskProgress: TaskProgress } case class NoopCopilation( val timer: Timer, ){ override def taskProgress: TaskProgress = TaskProgress.empty, } case class IncrementalCompilation( val timer: Timer, token: Future[WorkDoneProgress.Token], taskProgress: TaskProgress )

what do you think?

kasiaMarek changed the title ~~I374~~ Replace slowTask with LSP progress Feb 20, 2024

kasiaMarek commented Feb 21, 2024

View reviewed changes

kasiaMarek force-pushed the i374 branch from c83f8d0 to 8992d8e Compare February 21, 2024 15:30

kasiaMarek marked this pull request as ready for review February 21, 2024 16:17

kasiaMarek requested review from tgodzik and jkciesluk February 21, 2024 16:18

kasiaMarek mentioned this pull request Mar 1, 2024

build/taskProgress aren't forwarded when status bar is "off" #6187

Closed

ckipp01 reviewed Mar 1, 2024

View reviewed changes

jkciesluk reviewed Mar 6, 2024

View reviewed changes

metals/src/main/scala/scala/meta/internal/builds/ShellRunner.scala Outdated Show resolved Hide resolved

metals/src/main/scala/scala/meta/internal/metals/WorkDoneProgress.scala Outdated Show resolved Hide resolved

kasiaMarek force-pushed the i374 branch from 8ff2280 to 5baada2 Compare March 15, 2024 08:59

tgodzik requested changes Apr 2, 2024

View reviewed changes

kasiaMarek force-pushed the i374 branch from 5baada2 to ffd3d76 Compare April 2, 2024 15:41

tgodzik approved these changes Apr 16, 2024

View reviewed changes

kasiaMarek added 12 commits April 19, 2024 13:47

use slow task instead of status bar

7dde85e

delete metals slow task

b825317

fix cancel tests

0050e39

add timer

d80bb07

format

c7907b7

test fixes

b2f4562

rename slowTask to workDoneProgress

e379913

review fixes

f7d0206

review fixes [skip ci]

5c03559

delete noop logic for build/taskStart and similar

cc71841

test: WorksheetLspSuite.cancel fix

41e1e05

fix after rebase

66c6d05

kasiaMarek force-pushed the i374 branch from 3443336 to 66c6d05 Compare April 19, 2024 11:50

kasiaMarek merged commit dfa2bfe into scalameta:main Apr 19, 2024
23 of 26 checks passed

kasiaMarek mentioned this pull request May 7, 2024

improvement: delete ongoing compilations from tree view #6385

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace `slowTask` with LSP progress #6144

Replace `slowTask` with LSP progress #6144

kasiaMarek commented Feb 20, 2024 •

edited

Loading

kasiaMarek Feb 21, 2024

ckipp01 Mar 1, 2024

kasiaMarek Mar 1, 2024

ckipp01 Mar 1, 2024

ckipp01 Mar 1, 2024

jkciesluk Mar 5, 2024

kasiaMarek Mar 5, 2024

ckipp01 Mar 1, 2024

jkciesluk left a comment

tgodzik Apr 2, 2024

kasiaMarek Apr 2, 2024 •

edited

Loading

tgodzik Apr 16, 2024

kasiaMarek Apr 16, 2024

tgodzik Apr 16, 2024

tgodzik Apr 2, 2024

tgodzik left a comment

tgodzik Apr 16, 2024

	token: Option[Future[WorkDoneProgress.Token]],
	token: Option[WorkDoneProgress.Token],

Replace slowTask with LSP progress #6144

Replace slowTask with LSP progress #6144

Conversation

kasiaMarek commented Feb 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkciesluk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kasiaMarek Apr 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgodzik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Replace `slowTask` with LSP progress #6144

Replace `slowTask` with LSP progress #6144

kasiaMarek commented Feb 20, 2024 •

edited

Loading

kasiaMarek Apr 2, 2024 •

edited

Loading