-
-
Notifications
You must be signed in to change notification settings - Fork 175
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
Provide support for commands #21
Conversation
2011a25
to
7471a0c
Compare
Actually, having looked at that disgusting applyEdit function, we should iterate over the changes and then apply them all in bulk. Otherwise the undo stack looks a bit odd. |
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.
Thanks for looking into it. Please see comments.
Most important to have a proof that it is working, e.g. with the JSON example server. Please add a command that does workspace edits to demonstrate it.
src/workspace.ts
Outdated
@@ -83,4 +83,36 @@ export class MonacoWorkspace implements Workspace { | |||
return this.onDidChangeTextDocumentEmitter.event; | |||
} | |||
|
|||
public applyEdit(workspaceEdit: WorkspaceEdit): Thenable<boolean> { | |||
let applied = true; | |||
if (workspaceEdit.documentChanges) { |
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.
please use asWorkspaceEdit, applying documentChanges
is not enough
src/workspace.ts
Outdated
@@ -83,4 +83,36 @@ export class MonacoWorkspace implements Workspace { | |||
return this.onDidChangeTextDocumentEmitter.event; | |||
} | |||
|
|||
public applyEdit(workspaceEdit: WorkspaceEdit): Thenable<boolean> { |
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.
please return Promise
src/workspace.ts
Outdated
let applied = true; | ||
if (workspaceEdit.documentChanges) { | ||
for (const change of workspaceEdit.documentChanges) { | ||
if (change.textDocument.version && change.textDocument.version >= 0) { |
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.
Are these checks necessary?
src/workspace.ts
Outdated
applied = false; | ||
} | ||
} else { | ||
applied = false; |
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.
Is not the whole change should be applied or rejected? It seems that changes can be applied partially by this method.
src/workspace.ts
Outdated
if (change.textDocument.version && change.textDocument.version >= 0) { | ||
const textDocument = this.documents.get(change.textDocument.uri); | ||
if (textDocument && textDocument.version === change.textDocument.version) { | ||
monaco.editor.getModel(monaco.Uri.parse(textDocument.uri)).pushEditOperations( |
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.
Should not you check that the model is defined?
src/commands.ts
Outdated
|
||
export class MonacoCommands implements Commands { | ||
|
||
public constructor(private _editor: monaco.editor.IStandaloneCodeEditor) { } |
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.
protected readonly editor
src/commands.ts
Outdated
public constructor(private _editor: monaco.editor.IStandaloneCodeEditor) { } | ||
|
||
public registerCommand(command: string, callback: (...args: any[]) => any, thisArg?: any): Disposable { | ||
return (this._editor as any)._commandService.addCommand(command, { |
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.
please introduce augmenting typings for internals instead of using any
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.
Could you explain what you mean here? Not sure I'm familiar with this technique.
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.
I meant that you should add d.ts. file that augments monaco
module add exposes _commandService
, look at https://github.com/theia-ide/theia/blob/master/packages/monaco/src/typings/monaco/index.d.ts.
src/commands.ts
Outdated
public registerCommand(command: string, callback: (...args: any[]) => any, thisArg?: any): Disposable { | ||
return (this._editor as any)._commandService.addCommand(command, { | ||
handler: (id: string, ...args: any[]) => { | ||
console.log("Executing command", command, id, args); |
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.
Won't it pollute console?
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.
Sorry, left in from testing!
src/workspace.ts
Outdated
} else { | ||
applied = false; | ||
} | ||
return Promise.resolve(applied); |
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.
Could you use high-level functions as reduce
to get rid of if-else cascade?
7471a0c
to
47cc839
Compare
I've made the changes but not written an example. Any ideas on what to use as an example?! |
@gatesn the same here please use rebase instead of merge |
I like changes but there should be a proof that it is working: tests or use of it in the example. What if the JSON server provides:
It should not be sophisticated, but only demonstrate that everything works together. |
5800486
to
6679516
Compare
@akosyakov I created an example that provides a code action for any document in all positions. This action triggers a server-side command that upper cases the entire document. |
1573c3d
to
fad66e0
Compare
@gatesn I've tested this PR and it works nicely, thank you! I've opened a new PR against your, please merge it and after that, I will approve this PR. Next time you can work in the branch directly in this repo since you have the write access instead of working on the fork. It makes easy to push new commits to PRs. |
…age server Signed-off-by: Nicholas Gates <ngates@palantir.com>
4e2162c
to
0cd9823
Compare
@akosyakov merged your changes, thanks |
Do you think we could cut a release after this merges? |
@gatesn LGTM, thank you again! |
yes, we can make a release. Should we migrate to monaco 0.10.0 before the release? |
btw i wonder why github cannot recognize your account as an author, see: https://github.com/TypeFox/monaco-languageclient/commits/master |
Probably no 0.10.0 given the breaking changes around Code actions. We might need to investigate to see what that means. |
And it seems my git setup is different on different laptop :/ |
Pull request TypeFox#21 added support for the workspace/applyEdit capability so it should be stated as such in the WorkspaceClientCapabilities. Signed-off-by: Remy Suen <remy.suen@gmail.com>
Pull request TypeFox#21 added support for the workspace/applyEdit capability so it should be stated as such in the WorkspaceClientCapabilities. Signed-off-by: Remy Suen <remy.suen@gmail.com>
Pull request #21 added support for the workspace/applyEdit capability so it should be stated as such in the WorkspaceClientCapabilities. Signed-off-by: Remy Suen <remy.suen@gmail.com>
This implements a basic applyEdit operation (more advanced ones would accept the editor and preserve / manipulate the current selections), as well as registering commands against a given editor.
Fixes #11
Fixes #12