Send $/progress Notifications

[schrieveslaach]

trafficstars

Along with the specific event language/progressReport eclipse.jdt.ls now sends the standard Language Server Protocol event $/progress. This enables unified handling of eclipse.jdt.ls in all IDEs. For example, the fidget plugin for Neovim can rely on these events without adding code that handles eclipse.jdt.ls' specific event. Here is an example (see bottom right corner):

[eclipse-ls-bot]

Can one of the admins verify this patch?

[rgrunber]

I would think language/progressReport should be phased out in favour of this. Looks like it was introduced in 3.15.0. @testforstephen thoughts ?

[schrieveslaach]

I did not remove language/progressReport intentionally because that would be breaking change. For example, nvim-jdtls relies on it (don't know about others). But if you prefer the removal, I can remove it. Otherwise, language/progressReport could reside for a while and could be removed in another version.

[mfussenegger]

I did not remove language/progressReport intentionally because that would be breaking change. For example, nvim-jdtls relies on it (don't know about others)

nvim-jdtls works with $/progress as well (due to it being in neovim core). The PR here would actually end up in duplicate notifications in its current state. From client side perspective I think it should send either, not both.

[testforstephen]

Adapting the standard LSP notification is no problem for me. What I want to know is how does the vscode-java client respond if you send duplicate progress notifications?

[fbricon]

We should not send duplicate progress report. At all.

[schrieveslaach]

So, I assume that I should remove language/progressReport if I understood @fbricon and @testforstephen correctly. Could you react with :+1: on my comment to confirm?

[testforstephen]

vscode-java client is using language/progressReport to report status. If you migrate language/progressReport to $/progress, could you pls send a PR to fix https://github.com/redhat-developer/vscode-java/pulls as well.

[schrieveslaach]

I'm bit confused now because sending $/progress and language/progressReport for a while would be the least invasive option and would give IDE/editor plugins time to adapt (at least in my opinion). These types of events are distinct from each other so sending these additional events should not break any behavior.

nvim-jdtls works with $/progress as well (due to it being in neovim core). The PR here would actually end up in duplicate notifications in its current state. From client side perspective I think it should send either, not both.

@mfussenegger, you are right that message show up twice (see screenshot that has been after executing :lua print(vim.inspect(vim.lsp.util.get_progress_messages() in Neovim). However, that is just because of the mapping of language/progressReport to Neovim LSP messages and it does not break any functionality.

vscode-java client is using language/progressReport to report status. If you migrate language/progressReport to $/progress, could you pls send a PR to fix https://github.com/redhat-developer/vscode-java/pulls as well.

I looked at the code (I have no experience in VSCode plugin development) and it looks to me that there is no mapping to a standard $/progress handler. So this PR should not break any behavior. And the PR that uses $/progress in the VSCode plugin could be done later (by me with some expert guidance or someone else).

I think it is the best approach to send both events for a while. When dropping language/progressReport eclipse.jdt.ls should bump the major version because that would be breaking change for all consumers of language/progressReport.

[agorgl]

Waiting for this!

[phaalonso]

Waiting for this!

[yashLadha]

Any update on this?

[rgrunber]

So emitting duplicate event endpoints is the least volatile approach because each client can choose to subscribe to $/progress or continue with language/progressReport (until we remove it). @fbricon doesn't want to have us emitting "duplicate events". @fbricon is there any reason for this ? To clarify, a client would not receive duplicate events, but would have 2 different notification endpoints to choose from when subscribing to a single set of events. I don't think any client would behave badly since no client would subscribe to both .

If we simply cannot have the 2 endpoints, then the only way forward is to remove language/progressReport and use $/progress. But if that approach is taken, @testforstephen would want vscode-java migrated over as a consequence of this change. This would also break any other clients not keeping up to date with this change.

[fbricon]

unnecessary events consume CPU. If you need to support both during a transition period, I suggest you use a feature flag to enable one of them only. Remove the flag once support is done on the client side

[rgrunber]

unnecessary events consume CPU. If you need to support both during a transition period, I suggest you use a feature flag to enable one of them only. Remove the flag once support is done on the client side

A client capability seems reasonable. This already exists for language/progressReport. See https://github.com/eclipse/eclipse.jdt.ls/blob/d4aa55ade2cb6394778023aa6c98d22d54c83a37/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/ProgressReporterManager.java#L233

This is defined in https://github.com/eclipse/eclipse.jdt.ls/blob/d4aa55ade2cb6394778023aa6c98d22d54c83a37/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/preferences/ClientPreferences.java#L179 .

So could we just create the same kind of extended client capability preference (eg. notifyProgressSupport) for $/progress, within this PR, and make a single call in sendStatus, but not both. Clients can choose between the 2.

[mfussenegger]

Given that $/progress is part of the specification, would it make sense to send language/progressReport if progressReportProvider is true (like the current behavior) and if it is absent or false use $/progress ? That should result in the right behavior for any standard compliant client, without requiring extra settings, wasting extra CPU cycles or breaking anything for clients currently setting progressReportProvider = true

[rgrunber]

Given that $/progress is part of the specification, would it make sense to send language/progressReport if progressReportProvider is true (like the current behavior) and if it is absent or false use $/progress ? That should result in the right behavior for any standard compliant client, without requiring extra settings, wasting extra CPU cycles or breaking anything for clients currently setting progressReportProvider = true

I agree this is totally fine going by the specification. However, I was thinking of the fact that we provided a capability, progressReportProvider, that allowed clients to suppress the progress notifications, and whether we should continue to provide that for the new API. @fbricon , what do you think ? If $/progress is part of the spec, and there's nothing in the spec about suppressing it, do we still want to have an equivalent option that permits clients to do that, much like our legacy progressReportProvider / language/progressReport functionality ?

[rgrunber]

Ok, just spoke to @fbricon .

When isProgressReportSupported() is true, report with language/progressReport (same as now) and do not report $/progress. Otherwise, report with $/progress and do not report with language/progressReport. So basically, what @mfussenegger was hinting should happen above. Should be an easy adjustment to this PR and hopefully we can merge once that's done.

[schrieveslaach]

@rgrunber, I changed the code and it works with Neovim and fidget by setting progressReportProvider to false in nvim-jdtls.

local config = {
    init_options = {
       extendedClientCapabilities = {
          -- Switching to standard LSP progress events (as soon as it lands, see link)
          -- https://github.com/eclipse/eclipse.jdt.ls/pull/2030#issuecomment-1210815017
          progressReportProvider = false
       }
    },
}

[rgrunber]

Overall, I think this looks good. Is it possible to add a testcase in ProgressReporterManagerTest that confirms notifyProgress is called when isProgressReportSupported() is false ?

[schrieveslaach]

Nice, thank you for merging. :blush: