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

Allow clients to redefine untitled files protocol during their creation #4841

Merged
merged 1 commit into from
Jul 31, 2024
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
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ interface CodyAgentClient {
@JsonRequest("textDocument/edit")
fun textDocument_edit(params: TextDocumentEditParams): CompletableFuture<Boolean>
@JsonRequest("textDocument/openUntitledDocument")
fun textDocument_openUntitledDocument(params: UntitledTextDocument): CompletableFuture<Boolean>
fun textDocument_openUntitledDocument(params: UntitledTextDocument): CompletableFuture<ProtocolTextDocument?>
@JsonRequest("textDocument/show")
fun textDocument_show(params: TextDocument_ShowParams): CompletableFuture<Boolean>
@JsonRequest("workspace/edit")
Expand Down
4 changes: 2 additions & 2 deletions agent/src/TestClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -340,9 +340,9 @@ export class TestClient extends MessageHandler {
return result
})
this.registerRequest('textDocument/openUntitledDocument', params => {
this.workspace.loadDocument(ProtocolTextDocumentWithUri.fromDocument(params))
const doc = this.workspace.loadDocument(ProtocolTextDocumentWithUri.fromDocument(params))
this.notify('textDocument/didOpen', params)
return Promise.resolve(true)
return Promise.resolve(doc.protocolDocument.underlying)
})
this.registerRequest('textDocument/edit', async params => {
this.textDocumentEditParams.push(params)
Expand Down
23 changes: 22 additions & 1 deletion agent/src/agent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -487,7 +487,7 @@ export class Agent extends MessageHandler implements ExtensionClient {

this.registerNotification('textDocument/didSave', async params => {
const uri = vscode.Uri.parse(params.uri)
const document = await vscode.workspace.openTextDocument(uri)
const document = await this.workspace.openTextDocument(uri)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unrelated change but it was looking like bug to me.

vscode_shim.onDidSaveTextDocument.fire(document)
})

Expand Down Expand Up @@ -1319,6 +1319,27 @@ export class Agent extends MessageHandler implements ExtensionClient {
return this.fixups
}

public openNewDocument = async (
_: typeof vscode.workspace,
uri: vscode.Uri
): Promise<vscode.TextDocument | undefined> => {
if (uri.scheme !== 'untitled') {
return vscode_shim.workspace.openTextDocument(uri)
}

if (this.clientInfo?.capabilities?.untitledDocuments !== 'enabled') {
const errorMessage =
'Client does not support untitled documents. To fix this problem, set `untitledDocuments: "enabled"` in client capabilities'
logError('Agent', 'unsupported operation', errorMessage)
throw new Error(errorMessage)
}

const result = await this.request('textDocument/openUntitledDocument', {
Copy link
Contributor Author

@pkukielka pkukielka Jul 16, 2024

Choose a reason for hiding this comment

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

Previously this call was in the openTextDocument, I think now it is in much more logical and better isolated place (openTextDocument should not have side effects like creating new files, and we cannot guarantee that for openUntitledDocument implementation as e.g. IntelliJ creates those files on the disk.).

uri: uri.toString(),
})
return result ? vscode_shim.workspace.openTextDocument(result.uri) : undefined
}

private maybeExtension: ExtensionObjects | undefined

public async provide(extension: ExtensionObjects): Promise<vscode.Disposable> {
Expand Down
63 changes: 17 additions & 46 deletions agent/src/vscode-shim.ts
Original file line number Diff line number Diff line change
Expand Up @@ -301,16 +301,14 @@ const _workspace: typeof vscode.workspace = {
throw new Error('workspaceDocuments is uninitialized')
}

const result = toUri(uriOrString)
if (result) {
if (result.uri.scheme === 'untitled' && result.shouldOpenInClient) {
Copy link
Contributor Author

@pkukielka pkukielka Jul 16, 2024

Choose a reason for hiding this comment

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

That was source of some of our problems, as VSC does not expect any side effects from this function, but we do not control client implementation.

await openUntitledDocument(result.uri)
}
return workspaceDocuments.openTextDocument(result.uri)
}
return Promise.reject(
new Error(`workspace.openTextDocument:unsupported argument ${JSON.stringify(uriOrString)}`)
)
const uri = toUri(uriOrString)
return uri
? workspaceDocuments.openTextDocument(uri)
: Promise.reject(
new Error(
`workspace.openTextDocument: unsupported argument ${JSON.stringify(uriOrString)}`
)
)
},
workspaceFolders,
getWorkspaceFolder: () => {
Expand Down Expand Up @@ -463,56 +461,27 @@ const defaultTreeView: vscode.TreeView<any> = {
title: undefined,
}

/**
* @returns An object with a URI and a boolean indicating whether the URI should be opened in the client.
* This object with UUID path is used only when we want to create in-memory temp files, and those we do not want to send to the clients.
*/
function toUri(
uriOrString: string | vscode.Uri | { language?: string; content?: string } | undefined
): { uri: Uri; shouldOpenInClient: boolean } | undefined {
): Uri | undefined {
if (typeof uriOrString === 'string') {
return { uri: Uri.file(uriOrString), shouldOpenInClient: true }
return Uri.parse(uriOrString)
}
if (uriOrString instanceof Uri) {
return { uri: uriOrString, shouldOpenInClient: true }
return uriOrString
}
if (
typeof uriOrString === 'object' &&
((uriOrString as any)?.language || (uriOrString as any)?.content)
) {
const language = (uriOrString as any)?.language ?? ''
const extension = extensionForLanguage(language) ?? language
return {
uri: Uri.from({
scheme: 'untitled',
path: `${uuid.v4()}.${extension}`,
}),
shouldOpenInClient: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

nice 🚀

}
}
return
}

async function openUntitledDocument(uri: Uri, content?: string, language?: string) {
if (clientInfo?.capabilities?.untitledDocuments !== 'enabled') {
const errorMessage =
'Client does not support untitled documents. To fix this problem, set `untitledDocuments: "enabled"` in client capabilities'
logError('vscode.workspace.openTextDocument', 'unsupported operation', errorMessage)
throw new Error(errorMessage)
}
if (agent) {
const result = await agent.request('textDocument/openUntitledDocument', {
uri: uri.toString(),
content,
language,
return Uri.from({
scheme: 'untitled',
path: `${uuid.v4()}.${extension}`,
})

if (!result) {
throw new Error(
`client returned false from textDocument/openUntitledDocument: ${uri.toString()}`
)
}
}
return
}

function outputChannel(name: string): vscode.LogOutputChannel {
Expand Down Expand Up @@ -705,6 +674,7 @@ const _window: typeof vscode.window = {
selection.end.character
)
: undefined

const result = await agent.request('textDocument/show', {
uri,
options: {
Expand All @@ -715,6 +685,7 @@ const _window: typeof vscode.window = {
if (!result) {
throw new Error(`showTextDocument: client returned false when trying to show URI ${uri}`)
}

if (!workspaceDocuments) {
throw new Error('workspaceDocuments is undefined')
}
Expand Down
10 changes: 9 additions & 1 deletion vscode/src/extension-client.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import type { Disposable } from 'vscode'
import type { Disposable, TextDocument, Uri } from 'vscode'
import type vscode from 'vscode'
import type { EnterpriseContextFactory } from './context/enterprise-context-factory'
import type { ClientCapabilities } from './jsonrpc/agent-protocol'
import { FixupCodeLenses } from './non-stop/codelenses/provider'
Expand Down Expand Up @@ -31,6 +32,12 @@ export interface ExtensionClient {
*/
createFixupControlApplicator(fixups: FixupActor & FixupFileCollection): FixupControlApplicator

/**
* Opens a new document, creating appropriate file is required by a protocol.
* This method allows client to change the URI, so the caller should inspect returned TextDocument.
*/
openNewDocument(workspace: typeof vscode.workspace, uri: Uri): Thenable<TextDocument | undefined>

get clientName(): string
get clientVersion(): string
get capabilities(): ClientCapabilities | undefined
Expand All @@ -45,6 +52,7 @@ export function defaultVSCodeExtensionClient(): ExtensionClient {
dispose: () => {},
}),
createFixupControlApplicator: files => new FixupCodeLenses(files),
openNewDocument: (workspace, uri) => workspace.openTextDocument(uri),
clientName: 'vscode',
clientVersion: version,
capabilities: undefined,
Expand Down
2 changes: 1 addition & 1 deletion vscode/src/jsonrpc/agent-protocol.ts
Original file line number Diff line number Diff line change
Expand Up @@ -311,7 +311,7 @@ export type ServerRequests = {
'window/showMessage': [ShowWindowMessageParams, string | null]

'textDocument/edit': [TextDocumentEditParams, boolean]
'textDocument/openUntitledDocument': [UntitledTextDocument, boolean]
'textDocument/openUntitledDocument': [UntitledTextDocument, ProtocolTextDocument | undefined | null]
Copy link
Contributor

Choose a reason for hiding this comment

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

is it something normal to have both undefined and null as a possible return type? for me it looks weird

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's typical declaration in our protocol, please look at the rest of the file.

'textDocument/show': [
{
uri: string
Expand Down
12 changes: 8 additions & 4 deletions vscode/src/non-stop/FixupController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ export class FixupController

constructor(
private readonly authProvider: AuthProvider,
client: ExtensionClient
private readonly client: ExtensionClient
) {
this.controlApplicator = client.createFixupControlApplicator(this)
// Observe file renaming and deletion
Expand Down Expand Up @@ -985,16 +985,20 @@ export class FixupController
}

// append response to new file
const doc = await vscode.workspace.openTextDocument(newFileUri)
const doc = await this.client.openNewDocument(vscode.workspace, newFileUri)
if (!doc) {
throw new Error(`Cannot create file for the fixup: ${newFileUri.toString()}`)
}
Comment on lines +988 to +991
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this not just be await vscode.workspace.openTextDocument(newFileUri), or will that cause problems?

My worry is that it won't be clear to everyone that we need to use this openNewDocument method. and we will introduce bugs with untitled files elsewhere

Copy link
Contributor Author

@pkukielka pkukielka Jul 16, 2024

Choose a reason for hiding this comment

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

We do not need to use it for all untitled:// files (even more, we shouldn't), only files which we want to present to the users. I do not think there is right now any other such use case other than edits. Perhaps we should change the name to reflect that fact better.

Problem is fundamental:
Jetbrains do not have untitled:// (or unsaved) files support.
They need to be represented as normal file://
And unless we want to do a manual file mapping for every URI used in the protocol, and send request to IntelliJ for every openTextDocument call, this is best option we are left with.

I'm very open to discussion though. If you would like to jump on the call @umpox I can explain all corner cases.


const pos = new vscode.Position(Math.max(doc.lineCount - 1, 0), 0)
const range = new vscode.Range(pos, pos)
task.selectionRange = range
task.insertionPoint = range.start
task.fixupFile = this.files.replaceFile(task.fixupFile.uri, newFileUri)
task.fixupFile = this.files.replaceFile(task.fixupFile.uri, doc.uri)

// Set original text to empty as we are not replacing original text but appending to file
task.original = ''
task.destinationFile = newFileUri
task.destinationFile = doc.uri

// Show the new document before streaming start
await vscode.window.showTextDocument(doc, {
Expand Down
Loading