Add window/progress #21
Conversation
| title: string; | ||
|
|
||
| /** | ||
| * Optional progress message to display. |
There was a problem hiding this comment.
If it is unset, should the client clear the old message to keep it?
There was a problem hiding this comment.
Imo that could be up to the client to decide. VS Code may flash them in the status bar, but e.g. Eclipse may show a small log.
There was a problem hiding this comment.
The spec should define the desired behavior. Does the server need to always send the current status it wants displayed or does it only need to send the status when it changes?
There was a problem hiding this comment.
I would say a new message overrides the previous. So no message clears an existing message.
There was a problem hiding this comment.
Yeah that seems like the only reasonable behaviour, but we should check what vscode is currently doing and make the spec match up to that behaviour.
There was a problem hiding this comment.
I have no idea what the underlying implementation actually does if you give a progress update without specifying message. Will have to read the vscode src or experiment. Will want it to match what vscode does. Once we have that information we can decide if making it optional or not makes sense and will document the expected behaviours clearly.
There was a problem hiding this comment.
Source code for progress in window https://sourcegraph.com/github.com/Microsoft/vscode@1.12.1/-/blob/src/vs/workbench/services/progress/browser/progressService2.ts#L85
- They are ignoring the percentage at the moment. Note in the announcement they don't mention they support percentage, just message
- No message in an update is a bit confusing (see the playing around with
idx). I am guessing it ends up hiding the progress update, but doesn't cancel it.
There was a problem hiding this comment.
We should think about this from a spec perspective and not necessarily about what VS Code does now.
Here is what I would propose:
- Keep this optional.
- If specified, it should override any previous progress message.
- If not specified, the previous progress message is still valid.
If a server wants to clear the progress message without providing a new one, it can send the empty string.
There was a problem hiding this comment.
I got started on updating this, but realised that it may make the UI more restrictive than necessary. For example the UI could display progress messages as a log (so you can see older ones). I think the most important thing to do is specify the behaviour when unset, which I'll attempt to word exactly and update the PR. If you think specifying behaviour for the message set is correct, I'll do so.
|
|
||
| /** | ||
| * The title of the progress. | ||
| * This should be the same for all ProgressParams with the same id. |
There was a problem hiding this comment.
Would something bad happen if the server wanted to change the title of the progress for some reason? Seems like it should be ok from a technical perspective for this to change.
There was a problem hiding this comment.
For vscode the ability to change the title doesn't exist. So I assume it would just be ignored. This is a notification, so there is no response to the server to indicate failure.
|
|
||
| /** | ||
| * Set to true on the final progress update. | ||
| * No more progress notifications with the same ID should be sent. |
There was a problem hiding this comment.
Why can't we remove this and rely on percentage=100?
There was a problem hiding this comment.
percentage is optional, an LS can show undetermined progress indicators
There was a problem hiding this comment.
We only need one way to signal finished.
Even if the client does not show progress, we can use progress: 100 to indicate done.
The done parameter is unnecessary.
There was a problem hiding this comment.
The done parameter is unnecessary, but is it not much clearer? Also I assume you can be at 100% progress, but still be sending messages.
There was a problem hiding this comment.
What message might get sent after 100% progress?
To me having two parameters that appear to mean the same thing is less clear, not more.
There was a problem hiding this comment.
If there is a valid case where progress=100 and done != true, then it would be helpful to document when this could happen.
There was a problem hiding this comment.
The progress number is whatever the LS wants to set it too. I am sure you could make it non-monotonic if that is what you wanted. I know in the case of golang, we probably wouldn't use the progress number at all and just use the progress messages. To me it makes a lot of sense to not overload the meaning of this number and instead have it just passed through for the UI to interpret.
Also if you want to get technical, the number is floating point so if we wanted to define == 100, we would probably have to define what it means for a floating point number to equal 100 (ie with what precision / rounding).
protocol.md
Outdated
| * Set to true on the final progress update. | ||
| * No more progress notifications with the same ID should be sent. | ||
| */ | ||
| done?: bool; |
|
Why delete the branch? Shouldn't the next step be to open the PR at ms/lsp? |
|
Proposal at microsoft#245 |
|
@felixfbecker using the same branch would include our other changes in master. I created a new branch. |
Proposing this as of microsoft#245 (comment). Existing discussion regarding the title: sourcegraph#21 (comment).
Proposing this as of microsoft#245 (comment). Existing discussion regarding the title: #21 (comment).
This is a proposal for a PR to Microsoft's repo
This is to enable use of a new feature available to vscode extensions. The progress extension allows logging to source control and the window. However, we do not include that ability in this spec since:
See discussion on https://github.com/sourcegraph/sourcegraph/issues/5565 for more context.