-
Notifications
You must be signed in to change notification settings - Fork 337
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add proposed window/progress extension #261
Changes from all commits
47827b4
5e8d8dd
da9ec7d
42b12f3
fca3df0
2853b6e
0ed0e4a
382a4c6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,86 @@ | ||
/* -------------------------------------------------------------------------------------------- | ||
* Copyright (c) Microsoft Corporation. All rights reserved. | ||
* Licensed under the MIT License. See License.txt in the project root for license information. | ||
* ------------------------------------------------------------------------------------------ */ | ||
'use strict'; | ||
|
||
import { window, Progress, ProgressLocation } from 'vscode'; | ||
|
||
import { BaseLanguageClient, StaticFeature } from './client'; | ||
import { ClientCapabilities, Proposed } from 'vscode-languageserver-protocol'; | ||
|
||
export class WindowProgressFeature implements StaticFeature { | ||
private _progresses: Map<string, WindowProgress> = new Map<string, WindowProgress>(); | ||
|
||
constructor(private _client: BaseLanguageClient) {} | ||
|
||
public fillClientCapabilities(capabilities: ClientCapabilities): void { | ||
let windowProgressCapabilities = capabilities as Proposed.WindowProgressClientCapabilities; | ||
|
||
windowProgressCapabilities.window = windowProgressCapabilities.window || {}; | ||
windowProgressCapabilities.window.progress = true; | ||
} | ||
|
||
public initialize(): void { | ||
let client = this._client; | ||
let progresses = this._progresses; | ||
|
||
let handler = function (params: Proposed.ProgressParams) { | ||
let progress = progresses.get(params.id); | ||
if (progress !== undefined) { | ||
progress.updateProgress(params); | ||
} else { | ||
window.withProgress({ location: ProgressLocation.Window }, p => { | ||
progress = new WindowProgress(p); | ||
progresses.set(params.id, progress); | ||
|
||
progress.updateProgress(params); | ||
return progress.promise; | ||
}); | ||
} | ||
// In both cases progress shouldn't be undefined, but make the compiler happy. | ||
if (params.done && progress !== undefined) { | ||
progress.finish(); | ||
progresses.delete(params.id); | ||
} | ||
}; | ||
|
||
client.onNotification(Proposed.WindowProgressNotification.type, handler); | ||
} | ||
} | ||
|
||
class WindowProgress { | ||
public promise: Promise<{}>; | ||
private resolve: (value?: {} | PromiseLike<{}> | undefined) => void; | ||
private reject: (reason?: any) => void; | ||
|
||
private progress: Progress<{ message?: string; }>; | ||
|
||
private title: string; | ||
private message: string | undefined; | ||
|
||
constructor(progress: Progress<{ message?: string; }>) { | ||
this.progress = progress; | ||
|
||
this.promise = new Promise((resolve, reject) => { | ||
this.resolve = resolve; | ||
this.reject = reject; | ||
}); | ||
} | ||
|
||
public updateProgress(params: Proposed.ProgressParams) { | ||
this.title = params.title; | ||
this.message = params.message || this.message; | ||
|
||
const details = this.message ? ` (${this.message})` : ''; | ||
this.progress.report({message: `${this.title}${details}`}); | ||
} | ||
|
||
public finish() { | ||
this.resolve(); | ||
} | ||
|
||
public cancel() { | ||
this.reject(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,63 @@ | ||
#### Reporting server task progress | ||
|
||
Many tools are capable of performing some background task processing or data streaming. From a UX point of view, it's good to report both the fact that the tool is performing some background work, but also report the progress being made for it. To realize that and to provide a simple proposal based on which the feature can be later improved, the following additions are proposed: | ||
|
||
_Client Capabilities_: | ||
|
||
The client sets the following capability if it is supporting notifying task progress. | ||
|
||
```ts | ||
/** | ||
* Experimental client capabilities. | ||
*/ | ||
experimental: { | ||
/** | ||
* The client has support for reporting progress. | ||
*/ | ||
progress?: boolean; | ||
} | ||
``` | ||
|
||
##### Window Progress Notification | ||
_Notification_: | ||
|
||
The `window/progress` notification is sent from the server to the client to ask the client to indicate progress. | ||
|
||
* method: 'window/progress' | ||
* params: `ProgressParams` defined as follows: | ||
```ts | ||
export interface ProgressParams { | ||
/** | ||
* A unique identifier to associate multiple progress notifications with | ||
* the same progress. | ||
*/ | ||
id: string; | ||
|
||
/** | ||
* Mandatory title of the progress operation. Used to briefly inform about | ||
* the kind of operation being performed. | ||
* Examples: "Indexing" or "Linking dependencies". | ||
*/ | ||
title: string; | ||
|
||
/** | ||
* Optional, more detailed associated progress message. Contains | ||
* complementary information to the `title`. | ||
* Examples: "3/25 files", "project/src/module2", "node_modules/some_dep". | ||
* If unset, the previous progress message (if any) is still valid. | ||
*/ | ||
message?: string; | ||
|
||
/** | ||
* Optional progress percentage to display (value 100 is considered 100%). | ||
* If unset, the previous progress percentage (if any) is still valid. | ||
*/ | ||
percentage?: number; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure if percentage is a good idea since users could jump form 60 -> 50 which a lot of progress API's don't like. Could we used There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And should we defined that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
My feeling is that just makes the API more complex. Usually the progress comes from some number of work done divided by some number of total work, e.g. bytes downloaded by value of The client, if rendering a progress bar, ultimately needs to set the width of some element, which is easy using the latest absolute value. A delta API means the previous percentage value always needs to be kept around and incremented.
I don't know about other LSP clients, but which APIs are you thinking of? Also some progress might actually sometimes be based on an estimate that may have to be corrected, decreasing the progress slightly.
Could you explain what you mean by that? Isn't "Infinite progress" the same as "done", for which there is the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I'd say if a client does not support "time-traveling" progress they can do a quick sanity check
Maybe it'd make sense to omit the |
||
|
||
/** | ||
* Set to true on the final progress update. | ||
* No more progress notifications with the same ID should be sent. | ||
*/ | ||
done?: boolean; | ||
} | ||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,62 @@ | ||
/* -------------------------------------------------------------------------------------------- | ||
* Copyright (c) Microsoft Corporation. All rights reserved. | ||
* Licensed under the MIT License. See License.txt in the project root for license information. | ||
* ------------------------------------------------------------------------------------------ */ | ||
'use strict'; | ||
|
||
import { NotificationType, NotificationHandler } from 'vscode-jsonrpc'; | ||
|
||
export interface WindowProgressClientCapabilities { | ||
/** | ||
* Window specific client capabilities. | ||
*/ | ||
window?: { | ||
/** | ||
* Whether client supports handling progress notifications. | ||
*/ | ||
progress?: boolean; | ||
} | ||
} | ||
|
||
export interface ProgressParams { | ||
/** | ||
* A unique identifier to associate multiple progress notifications with the same progress. | ||
*/ | ||
id: string; | ||
|
||
/** | ||
* Mandatory title of the progress operation. Used to briefly inform about | ||
* the kind of operation being performed. | ||
* Examples: "Indexing" or "Linking dependencies". | ||
*/ | ||
title: string; | ||
|
||
/** | ||
* Optional, more detailed associated progress message. Contains | ||
* complementary information to the `title`. | ||
* Examples: "3/25 files", "project/src/module2", "node_modules/some_dep". | ||
* If unset, the previous progress message (if any) is still valid. | ||
*/ | ||
message?: string; | ||
|
||
/** | ||
* Optional progress percentage to display (value 100 is considered 100%). | ||
* If unset, the previous progress percentage (if any) is still valid. | ||
*/ | ||
percentage?: number; | ||
|
||
/** | ||
* Set to true on the final progress update. | ||
* No more progress notifications with the same ID should be sent. | ||
*/ | ||
done?: boolean; | ||
} | ||
|
||
/** | ||
* The `window/progress` notification is sent from the server to the client | ||
* to inform the client about ongoing progress. | ||
*/ | ||
export namespace WindowProgressNotification { | ||
export const type = new NotificationType<ProgressParams, void>('window/progress'); | ||
export type HandlerSignature = NotificationHandler<ProgressParams>; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before we merge this in I would like that we at least think about how cancellation would work in this model since I think that this is a common pattern with progress. Options I see:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Things to consider:
Option 1 would be a possible way forward in the future that considers these (although this proposal should not be blocked on that).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto what @felixfbecker said - I wouldn't want to block this and also it's worth thinking that progress report isn't always done against a client-issued request and doesn't have to support cancellation in every case.