LSP general progress notification support #6055
Conversation
| tickIfHidden() | ||
| value | ||
|
|
||
| if (clientConfig.statusBarState == StatusBarState.Off) { |
There was a problem hiding this comment.
Strikes me that it might be a good idea to introduce an abstraction over StatusBar called Progress or similar which could have 2 impls, one for when statusbar support is detected, the other using the LSP progress stuff. WDYT?
There was a problem hiding this comment.
I'm not the main contributor of metals but maybe we can also check if the client has window.workDoneProgress capability. If the client supports, metals can use progress instead of the existing status.
This will be similar to what gopls does.
There was a problem hiding this comment.
Checking the capability makes sense, though as the status bar support is richer than native LSP progress notifications I wonder if the order of precedence should be status bar > LSP progress > no-op
There was a problem hiding this comment.
Strikes me that it might be a good idea to introduce an abstraction over StatusBar called Progress or similar which could have 2 impls, one for when statusbar support is detected, the other using the LSP progress stuff. WDYT?
Since it's only this function I think it would be an overkill. Looks good as it is.
I wonder if the order of precedence should be status bar > LSP progress > no-op
Makes sense, I was hoping we could replace the custom status bar, but we even added some functionality there recently.
| begin | ||
| ) | ||
|
|
||
| client.createProgress(new WorkDoneProgressCreateParams(token)) |
There was a problem hiding this comment.
This does return a CompletableFuture[Unit] which currently I'm throwing away. Not clear what this future represents and hence if it should be blocked on.
There was a problem hiding this comment.
That's probably to wait for creating the progress on the client I would guess.
| @@ -102,6 +104,12 @@ trait MetalsLanguageClient | |||
| showMessage(params) | |||
| } | |||
|
|
|||
| override def createProgress( | |||
There was a problem hiding this comment.
You might also need to add an empty method to NoopLanguageClient
| begin | ||
| ) | ||
|
|
||
| client.createProgress(new WorkDoneProgressCreateParams(token)) |
There was a problem hiding this comment.
That's probably to wait for creating the progress on the client I would guess.
| tickIfHidden() | ||
| value | ||
|
|
||
| if (clientConfig.statusBarState == StatusBarState.Off) { |
There was a problem hiding this comment.
Strikes me that it might be a good idea to introduce an abstraction over StatusBar called Progress or similar which could have 2 impls, one for when statusbar support is detected, the other using the LSP progress stuff. WDYT?
Since it's only this function I think it would be an overkill. Looks good as it is.
I wonder if the order of precedence should be status bar > LSP progress > no-op
Makes sense, I was hoping we could replace the custom status bar, but we even added some functionality there recently.
Don't have direct experience but eyeballing the integrations I believe Emacs, Vim and Sublime all support status, I think it was just Helix that didn't |
Initial PoC impl, manually tested against Helix. Feedback appreciated!