Add cursor state syncing to workspace #98
Conversation
runtimes/protocol/workspace.ts
Outdated
| import { Range, Position, ProtocolNotificationType } from './lsp' | ||
|
|
||
| export interface CursorStateUpdateParams { | ||
| cursorState: { position: Position } | { range: Range } |
There was a problem hiding this comment.
There was a problem hiding this comment.
This can be made an array instead of a single position or range object, though for the near future a single object is also sufficient
runtimes/protocol/workspace.ts
Outdated
| cursorState: { position: Position } | { range: Range } | ||
| } | ||
|
|
||
| export const cursorStateUpdateNotificationType = new ProtocolNotificationType<CursorStateUpdateParams, void>( |
There was a problem hiding this comment.
Should this be part of extensions instead of workspace?
There was a problem hiding this comment.
afaik extensions was removed in the recent refactors
| @@ -27,4 +30,5 @@ export type Workspace = { | |||
| isFile: (path: string) => Promise<boolean> | |||
| remove: (dir: string) => Promise<void> | |||
| } | |||
| onCursorStateUpdate: (handler: NotificationHandler<CursorStateUpdateParams>) => void | |||
There was a problem hiding this comment.
nit: onCursorStateChange fits a bit better for the name
There was a problem hiding this comment.
If we included cursor position into chat & quick actions request, it would have been much less chatty.
There was a problem hiding this comment.
Yes, for now those two would be the only two places where we add the cursorstate param. But it can cause issues as the interfaces grow, we would need to add the cursor state as an optional parameter into many more requests
There was a problem hiding this comment.
How much more can we expect to have? Isn't it still better especially given default LSP methods already have cursor sync logic inside of those methods params?
|
Closing this pull request, we decided not to move forward with this approach. The chosen approach is here: #99 |
Problem
LSP doesn't provide a way to sync cursor state from client to server. See some discussions here: microsoft/language-server-protocol#718 (comment)
We want to provide the ability to sync this state. For example this could be used by chat servers to enrich the chat requests with the context of the currently highlighted code.
Solution
This change introduces a new notification type that will allow clients to send cursor state updates to servers.
Clients will be free to determine how often they send the cursor update notification. They can send every cursor change, send the latest change every X seconds or send the notification only before sending a specific request (for example a code completion request)
Tested the change on vscode using the test client in https://github.com/aws/language-servers
License
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.