Add proposed window/progress extension

[Xanewok]

This proposes a simple window/progress server -> client notification that's been described in microsoft/language-server-protocol#245. I tried to follow contributing.md as closely as possible, however it's possible I missed something. Additionally I'm not a TS/JS dev, so if there are any concerns for the client implementation, I'm more than open to changing it accordingly.

I think it's a simple but often requested feature and it'd be good to land it, at least with a simple initial version to gather proposal and improve upon it.

cc @felixfbecker this proposes the sourcegraph/language-server-protocol@8986486.

[msftclas]

All CLA requirements met.

[ljw1004]

I know you've done your work solely for VSCode. I'm thinking about how it would be incorporated into LSP for use by other editors as well. In editors where "progress" is indicated by a spinner on the status bar with text-only tooltips, how do you think the "title+message+percent" should be surfaced?

This is a screenshot where two different LSP-providers are each providing their own progress report.

The reason I ask is because I'm inclined towards an API where progress is solely reported by a single plain-text string, without breaking it up into title/message/percent.

Some providers will find that a percentage number is a poor way of presenting the information. For instance, an C++ indexing provider might report indexed 5/20 files, but then change it to 6/35 files as it discovers more information. In this case a percentage number wouldn't be useful (it would fluctuate up and down and would more reflect how much of the scope had yet been discovered rather than how much of the work had been done). But the pair of numbers is very informative.

I think that with your proposed split into message/percent, we'll end up in a world where 50% of providers report their progress with a custom piece of text inside the message like 5/20 files, and others report it with a percentage number following your scheme, and this lack of standardization will mean they're not actually useful anymore.

[felixfbecker]

It should definitely work for all kinds of editors and it is up to the editor to display a spinner, a progress bar or show only the text. If the language server wants to update the message with indexing file x/y that is totally fine and it can do that in addition to the percentage. The percentage is only to power a determined progress indicator, if the editor has a UI element for that, it can use it, if it uses an indeterministic progress indicator, it doesn't have to use it. An earlier version of this proposal had the percentage split into current and total, but I don't think there is value in having these as separate programmatic-inspectable fields.

I think that with your proposed split into message/percent, we'll end up in a world where 50% of providers report their progress with a custom piece of text inside the message like 5/20 files, and others report it with a percentage number following your scheme, and this lack of standardization will mean they're not actually useful anymore.

The language server is expected to provide both (as much info as possible) and then it's up to the client to decide what info to use/how to display it.

[ljw1004]

@felixfbecker Imagine if this PR goes ahead in its current form. Then I think few LSP servers will emit both 8/16 files in their message and also a 50% progress indicator because (in the current PR) it will render as indexed 8/16 files (50%) which looks kind of irritating.

I think the idea "it's up to the client to decide" only goes so far. Because LSP servers will alter themselves to look good on the dominant client.

[Xanewok]

@ljw1004 while I recognize how you may feel about it, I think this minimal solution finds a good balance between giving freedom for the LSP clients and providing more ability to programatically inspect the given value. I think it's worth noting that separating current/total and the percentage is sometimes desired:

• sometimes the units of work are not equal, e.g. 14/30 might mean 70% of total work is done
• processing headers: 16781/321681 (5.2%) allows to quickly spot order of magnitude difference at a glance
• from a UX point of view it might be feasible to indicate progress being done even when processed/total ratio doesn't change
• you don't always want to expose the exact current/total ratio to the user - by providing only a single progress measure this forces the client to always display the exact amount in the message or always use it for a progress bar (specifying if the client can omit the percentage/value and in what context can it use it would warrant additional flags in the params, which I don't think is ideal).

That's why I think providing both optional message and optional percentage allows for more and is concise and simple enough for most cases.

Maybe we could emphasize that percentage can be consumed by the LSP client to display the progress using UI?

[ljw1004]

@Xanewok Another possibility is to indicate that the percentage should be rolled into the message (1) always, (2) if no other progress indication is available, (3) never. But that's over-engineering.

Actually I'm more concerned about title at the moment. I read through the source code but couldn't figure out how the title is rendered. Maybe a screenshot?

[felixfbecker]

I agree with @ljw1004 that it is not good to put the percentage into the title automatically in this "reference implementation". The percentage should only be used to update a deterministic progress indicator which VS Code has in other places:

https://github.com/Microsoft/vscode/blob/24d7998be78fcd340ac5c4f6e5a7a660a26b24b9/src/vs/workbench/services/progress/browser/progressService2.ts#L70

[Xanewok]

@ljw1004 yeah, I was just editing my response about the title.

I agree with you that it may in fact be too specific here. I left it here, since it was in the original proposal and VSCode is capable of using it. Here's a screen:

@felixfbecker agreed, I saw only withProgress<R> API with Progress<{ message?: string; }> that's why I wrapped it into the message. I'll try using the variant with number, thanks for pointing that out.

[Xanewok]

@felixfbecker actually I'm not sure about the exposed API, not much of a TS/VSCode dev myself, mind pointing out how can I also include the percentage there?

vscode.d.ts API doesn't seem to expose the percentage, while the implementation has it?
I'm confused 😢

[keegancsmith]

It seems the number progress was initially meant for use in the now deprecated withScmProgress. I assume when that is removed they will remove support in the implementation.

Given the number is contentious maybe we should exclude it for now? I only included it in the linked PR because at the time I proposed it, I was matching the functionality of vscode. Allowing developers to report back numbers for progress gives me unpleasant dreams back to file copy dialogs in Windows XP.

[felixfbecker]

Would be fine for me to implement it with only supporting indeterministic progress indicators for now, we can add it later as a backwards-compatible change

[ljw1004]

The next question is: once we remove percent from the API, how about we do something with title and message?

In the screenshot you appeared to show two different UXs:

• In the status bar it prints message
• In the datatip it prints ${title} - ${message}

First, the ability to display the text in the status-bar seems extraordinary. In our environment we commonly have several progress messages all at once, e.g. lint path_to_file and hh_server is initializing [naming] [1m30s] and indexing files: <path>. To fit into the status bar, (1) it will look pretty goofy if there are three things adjacent to each other, (2) they basically have to condense themselves down into just one single word.

I propose removing the current title and message and replacing them with two other fields:

// message is a mandatory string for this progress operation. Editors may display it in
// datatips. Example: 'Indexing file 5/25 (5%)'
message: string;

// shortMessage is mandatory too. Some (not all) editors will use this when displaying
// progress in space-limited situations such as the status bar. Example: 'Indexing (5%)'
shortMessage: string;

My thinking is that to fit into the status-bar we LSP-server-authors will have to craft our messages with extraordinary care and compactness. And even to fit into a datatip we still need to craft our messages carefully to be brief enough and actionable enough. Crafting those two things effectively is difficult and we shouldn't be hamstrung.

[Xanewok]

Should the percentage in the displayed message in the reference implementation not be included there, then? Also about providing a title in the params, what are your opinions? Do you think it can be actually useful or is it too specific? In the specific case of Rust Language Server we have 2 operations, one executed after another: Compiling a diagnostic build and processing analysis data, so I assume we could stick a "Compiling" and "Processing" titles for those appropriately, but since the messages will probably also include that information, I think it seems redundant, and in general case I can't still think of a good use case for that.

…

On Mon, 9 Oct 2017 at 23:15, Felix Becker ***@***.***> wrote: Would be fine for me to implement it with only supporting indeterministic progress indicators for now, we can add it later as a backwards-compatible change — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#261 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AC8y3VwihUUnbpvvm3EP6jNoam3PdV4tks5sqozmgaJpZM4PxtoN> .

[felixfbecker]

@ljw1004 I don't see a difference in content between shortMessage and title, but the implementation in this PR should not do any string concatenation of any properties. And I don't think messages should be required. If an LS wants to just indicate that it is doing some work it should be able to omit the messages, it would be silly to require them to set an empty string or whatever workaround would be needed.

[ljw1004]

@felixfbecker - you anticipated my next suggestion, which is that the LS itself should be responsible for putting the name of the LS into the message. In the PR, the tooltip is a concatenation of (1) LS id, (2) title, (3) message, (4) percent.

I think the LS should be able to decide how to display the ID itself.

Given this, if message is completely empty, the user won't even know which LS or other service it was that produced the spinner.

[Xanewok]

@ljw1004 thanks for the condensed status bar suggestion!
In this case I think the title/message/percentage is a good combo: the clients can decide in which format they should present the message.

• clients with small amount of space and no spinner/UI, such as Vim, can display it as "Title" or "Title (Percentage%)"
• editors with some progress bar or other notification UI can also (optionally!) use the passed percentage number value to drive the UI and display only "Title" or "Message" (or however)
• clients, not the servers, should care whether they are capable of displaying the longer messages or the shorter ones, hence why if, e.g., there's only a single progress reported it can use the longer version, but if the server sends 2 additional progresses it can condense each and display only a title (with a text percentage or UI progress or without it completely).

We can change the names of the passed values and/or update documentation to better reflect its intended usage, but with these values any given LSP client has all the appropriate information to present it in a convenient and descriptive manner.

Regarding the LS ID, I think it's entirely up to the client how or if at all, does it show the origin of the progress indication and the indication itself.

In this reference client implementation, the VSCode windows.withProgress API automatically prepends the language ID and I think:

• that' outside the scope of the LSP progress reporting
• this would require more tinkering and not using the window.withProgress API (this would probably require more engineering, creating a custom IStatusBarItem, gluing it to the LS client, changing the default tooltip shown etc.).

I have no idea how to do that, so if we were to get rid of the default <languageId>: prefix, any implementation tips are welcome!

[ljw1004]

@Xanewok I think it will be hard to pull off. You try it... write down five example progress reports with title+message+percentage divided up in such a way that (1) title+percent looks good, (2) title+message+percent looks good, (3) Title alone looks good, (4) Message alone looks good.

I don't think it can be done!

I myself tinkered with my progress reports off and on over a month to make them as compact and actionable as possible. Here are the ones I came up with:

• hh_server is initializing [naming] [53 seconds]
• [Hack] preparing to check edits
• [Hack] checking edits
• [Hack] checking entire project
• Starting Hack language service
• Stopping Hack language service
• Flow server is busy (/users/ljw/code/proj1)

I don't know how I'd divide those into languageId/title/message/percent effectively.

[Xanewok]

@ljw1004 I hope I didn't come off as rude, I certainly didn't intend to and I hope I wasn't received as such! 😞

I'm a bit lost at this point - not sure if we're discussing the protocol itself or the reference VSCode implementation.
From the protocol and server's responsibilities point of view, I think sending title/message/percent (each optional!) is simple and sufficient, while the presentation side, the client, should decide how it uses the information.

Now onto what a client might do...
I think messages like 'Starting/Stopping XYZ language Service' should be outside of the protocol and presented by the client on its own, not expecting the server to do so, since it's very general and tied to what the client does, not the server.
Without factoring in the languageId, I think messages like: (each part is optional)

• Initializing/hh_server is initializing [naming]/X%
• Indexing/indexing headers 312/6582/X%
• Checking/checking {edits, project}/X%
• Waiting (for server)/Flow server is busy (/users/ljw/code/proj1)

might be okay? In these cases you might present it as Initializing, Initializing (hh_server is initializing [naming]), hh_server is initializing each optionally with (X%) stuck at the end, or optionally using UI, and I think it looks good enough.

As I said, clients such as Vim may only use the title itself, since their space is limited. GUI editors could detect if they need to condense the message and only display a title (optionally with a percentage/spinner?) and possibly provide a full name on hover (e.g. somewhat what VSCode also does).

I would leave languageId out of it, since sometimes these progress reports can be presented in a separate window per each LS, or even have their own space at the status bar (e.g. VSCode) and so I think it's only fair for an extension to group related progresses and use only a single language id in a text format, if it needed - for example a Vim frontend might display:

Rust: Compiling/Processing       Hack: Checking         Flow: Waiting for server

if it had 4 tasks running for 3 different LSs, in this example.

[ljw1004]

@Xanewok you've been the model of politeness and a very pleasant person to discuss with!

Following your reference implementation, this is what people will see when they hover over the progress indicator:

[hack] Initializing - hh_server is initializing [naming] (X%)
[hack] Indexing - indexing headers 312/6582 (X%)
[hack] Checking - checking edits (X%)
[flow] Flow server us busy (/usrs/ljw/code/proj1)

The middle two look wrong. Users will immediately file bugs that say "Why is the word 'indexing duplicated? Why is the word 'checking' duplicated?" And so the language server developer will immediately be forced to omit the word from the title so as to look good in your reference VSCode implementation -- with the knock-on effect that they now look bad on clients which chose to display only the title. Or vice versa, they'll be forced to omit the word from the message -- and so will now look bad on clients which chose to display only the message.

[Xanewok]

I'm very much open to changing the reference implementation! Frankly I wanted to have something simple working and reiterate from there. I agree that currently it does not look really good and needs to change.

What I'm more worried about is the window.withProgress API - currently it allows only for a single active progress indication (shows the most recent, unfinished), which seems fairly limiting, but without making a deep surgery on the internals of vscode and creating our own piece of logic for that, I'm not sure how to proceed with that.

[Xanewok]

Now percentage is not included as text in any messages, also not using title feature in the API to prevent weird/duplicated title - message tooltip.

For context: (incl. couple of issues, along with reasoning on the current withProgress API design)

• progress.ts (what indicator internally accepts)
• Tooltip being presented as title - message
• percentage field being removed from API

[dbaeumer]

@Xanewok @ljw1004 @felixfbecker thanks a lot for the very lively discussion. Highly appreciated! I read through all comments. May be it would make sense to look at the problem from a user perspective as well. From my experience so far with building progress APIs users are usually interested in the following things:

Progress to know when things are done:

• a progress indication (either percentage or 5/35 files indexed, were the total number of files might change)
• if no progress is indicated or an infinite progress is used (an endless spinner) then users want to see a changing message indicating what the system is doing.

Progress Details:

• detailed progress is only necessary for things the user is waiting for. If for example the users triggers a build and the users needs to wait for the build to finish he wants to see progress.
• background tasks like indexing don't need detailed progress. It would be enough to simply indicate that something (or many things) are active in the background. Details might only be necessary on user request (clicking on something).
• However the indexing progress (from pure background) might change if the user starts a search which needs the indexing to be finished first. Then a detailed progress about indexing is necessary.

I haven't thought about the necessary protocol we need and the UI implementation to best support this.

[dbaeumer]

Saying percentage === undefined is infinite progress is a good solution.

I am still not convinced that a time traveling progress monitor is something users appreciate. I usually dislike them very much :-)

And I do think that not thinking about how cancellation works in this scenario is a bad idea. If we have a progress API it should be usable for client initiated requests as well where cancellation does make sense. Consider the case where find all reference is based on an index which needs to be updated when first executed. If this takes a long time and a users doesn't want to wait for the result I think he appreciate being able to cancel them (Eclipse for example supports this nicely). I agree that this is not important for server initiated progress reporting. But I don't want to end up with two stories. I will make a proposal how that should work.

[felixfbecker]

Even Microsoft/pyright is using custom progress messages:

https://github.com/Microsoft/pyright/blob/5323d14aa878ba7eaa7d5e0685f963ba108f039d/server/src/server.ts#L81-L90

[Xanewok]

Is this blocked on CI and rebase? I'll fix this today. Should I change anything here before it's considered to be merged?

[dbaeumer]

@Xanewok it is on my April plan. I will do some additional work to report progress per request as well since this is requested for other clients. But that is orthogonal to the report progress from server handled here. Having a mergable PR would definitelly be appreciated.

[Xanewok]

@dbaeumer done, rebased and fixed the PR.

[dbaeumer]

I looked at other tools how they report progress and all possible different kinds exists:

• precentage
• increment
• total work & worked

Since they all have pros and cons it doesn't really matter which one we pick. So I decided to stay with the percentage since it is indeed easy to compute. However I will add the following things:

• a client is allowed to not uopdate the progress if the number jumps pack
• no providing a value on first all will mean infinite progress. This can not be changed later on (e.g. switch from infinite to percentage and vice versa)
• I will also add support to attach progress to a request. I will do this by adding a progress id to the request params. This id can then be used in later calls.

[dbaeumer]

I will merge the PR and then make the changes on master.

[Xanewok]

Awesome, thank you! 🎉

• a client is allowed to not uopdate the progress if the number jumps pack
• no providing a value on first all will mean infinite progress. This can not be changed later on (e.g. switch from infinite to percentage and vice versa)

Definitely agree with these, seems reasonable =)