Skip to content
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

Merged
merged 8 commits into from
Apr 12, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions client/src/main.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import { WorkspaceFoldersFeature } from './workspaceFolders';
import { FoldingRangeFeature } from './foldingRange';
import { DeclarationFeature } from './declaration';
import { SelectionRangeFeature } from './selectionRange.proposed';
import { WindowProgressFeature } from './progress.proposed';
import { CallHierarchyFeature } from './callHierarchy.proposed';

import * as Is from './utils/is';
Expand Down Expand Up @@ -512,6 +513,7 @@ export namespace ProposedFeatures {
export function createAll(client: BaseLanguageClient): (StaticFeature | DynamicFeature<any>)[] {
let result: (StaticFeature | DynamicFeature<any>)[] = [
new SelectionRangeFeature(client),
new WindowProgressFeature(client)
new CallHierarchyFeature(client)
];
return result;
Expand Down
86 changes: 86 additions & 0 deletions client/src/progress.proposed.ts
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();
}
}
10 changes: 9 additions & 1 deletion protocol/src/main.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ export * from './protocol';
export { FoldingRangeParams as FoldingRangeRequestParam } from './protocol'; // for backward compatibility

import * as callHierarchy from './protocol.callHierarchy.proposed';
import * as progress from './protocol.progress.proposed';
import * as sr from './protocol.selectionRange.proposed';

export namespace Proposed {
Expand Down Expand Up @@ -68,6 +69,13 @@ export namespace Proposed {
export type CallHierarchyDirection = callHierarchy.CallHierarchyDirection;
export type CallHierarchyItem = callHierarchy.CallHierarchyItem;
export type CallHierarchyCall = callHierarchy.CallHierarchyCall;

export type WindowProgressClientCapabilities = progress.WindowProgressClientCapabilities;
export type ProgressParams = progress.ProgressParams;
export namespace WindowProgressNotification {
export const type = progress.WindowProgressNotification.type;
export type HandlerSignature = progress.WindowProgressNotification.HandlerSignature;
}
}

export interface ProtocolConnection {
Expand Down Expand Up @@ -233,4 +241,4 @@ export type ProtocolConnetion = ProtocolConnection;

export function createProtocolConnection(reader: MessageReader, writer: MessageWriter, logger: Logger, strategy?: ConnectionStrategy): ProtocolConnection {
return createMessageConnection(reader, writer, logger, strategy);
}
}
63 changes: 63 additions & 0 deletions protocol/src/protocol.progress.proposed.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
#### Reporting server task progress
Copy link
Member

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:

  • we have a notification from the client to a server to signal cancellation based on the progress id
  • we make the progress notification a request that returns whether this is canceled or not (not my favorite)
  • we allow the progress to refer to a open request so that a client can map the things together.
  • we support that requests can provide a progress id which the server then uses to report progress on (my preferred options).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Things to consider:

  • The request ID is usually not known to the implementation of a method, it's hidden in the RPC layer.
  • A middleware proxy might rewrite request IDs (e.g. prefix them if managing many servers) but wouldn't be aware of also having to rewrite request IDs in the params of the progress notification.
  • Progresses can often not correspond to requests, but to async actions like indexing and background initialisation, dependency installation, ... These may sometimes be cancellable (although usually you'd really want to just shut down the server), but cannot be cancelled by a request ID.

Option 1 would be a possible way forward in the future that considers these (although this proposal should not be blocked on that).

Copy link
Contributor Author

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.


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;
Copy link
Member

Choose a reason for hiding this comment

The 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 worked, step or increment instead.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And should we defined that -1 as a start value us infinite progress. I am also happy with an or type or an additional property.

Copy link
Contributor

@felixfbecker felixfbecker Jan 25, 2019

Choose a reason for hiding this comment

The 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 worked, step or increment instead.

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 Content-Length header. Having to calculate that, then diff it against the last reported progress percentage just because the API expects a delta makes it more complex on the server.

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.

which a lot of progress API's don't like

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.

And should we defined that -1 as a start value us infinite progress. I am also happy with an or type or an additional property.

Could you explain what you mean by that? Isn't "Infinite progress" the same as "done", for which there is the done: true flag? I think it would be bad if -1 meant infinity because it would make comparing percentage values harder.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we used worked, step or increment instead.

I'd say if a client does not support "time-traveling" progress they can do a quick sanity check this.progress = max(newProgress, thisProgress);

And should we defined that -1 as a start value us infinite progress.

Maybe it'd make sense to omit the percentage altogether in a server message when reporting an indefinite progress?


/**
* Set to true on the final progress update.
* No more progress notifications with the same ID should be sent.
*/
done?: boolean;
}
```
62 changes: 62 additions & 0 deletions protocol/src/protocol.progress.proposed.ts
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>;
}
15 changes: 15 additions & 0 deletions protocol/src/protocol.ts
Original file line number Diff line number Diff line change
Expand Up @@ -645,6 +645,16 @@ export interface TextDocumentClientCapabilities {
};
}

/**
* Window specific client capabilities.
*/
export interface WindowClientCapabilities {
/**
* Whether client supports handling progress notifications.
*/
progress?: boolean;
}

/**
* Defines the capabilities provided by the client.
*/
Expand All @@ -659,6 +669,11 @@ export interface _ClientCapabilities {
*/
textDocument?: TextDocumentClientCapabilities;

/**
* Window specific client capabilities.
*/
window?: WindowClientCapabilities;

/**
* Experimental client capabilities.
*/
Expand Down