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

Workspace create file workaround #2019

Merged
merged 3 commits into from
Nov 19, 2020
Merged

Conversation

333fred
Copy link
Contributor

@333fred 333fred commented Nov 19, 2020

Adds a workaround for creating empty new files on the workspace side when a code action (such as generate type in a new file) is run. We need to add the document to the workspace when applying the code action, so that future updates aren't added to a transient file. However, this then triggers clients to send a new file notification. This can race with the actual content application: in fact, vscode seems to always send the update buffer request before the create event is sent, even if the create and update changes are applied on the vscode side as separate, sequential workspace edits. To work around this, we detect when a file create is sent, the disk read has no content, and the workspace already has a document with real content in it.

Adds a workaround for creating empty new files on the workspace side when a code action (such as generate type in a new file) is run. We need to add the document to the workspace when applying the code action, so that future updates aren't added to a transient file. However, this then triggers clients to send a new file notification. This can race with the actual content application: in fact, vscode seems to _always_ send the update buffer request before the create event is sent, even if the create and update changes are applied on the vscode side as separate, sequential workspace edits. To work around this, we detect when a file create is sent, the disk read has no content, and the workspace already has a document with real content in it.
// such as RunCodeAction. Unfortunately, previous attempts to have this fully controlled by the vscode
// client (such that it sent both create event and then updated existing text) wasn't successful:
// vscode seems to always trigger an update buffer event before triggering the create event.
if ((await document.GetTextAsync()).Length > 0 && isCreate && string.IsNullOrEmpty(buffer))
Copy link
Member

Choose a reason for hiding this comment

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

it would be slightly more efficient to check await document.GetTextAsync()).Length last, because the other two conditions may short circuit and there will be no need to fetch the document text anymore

@filipw
Copy link
Member

filipw commented Nov 19, 2020

Thanks! I tested this locally and as we discussed in Slack, it is hacky but it indeed solves the issue 😄
In fact, it now behaves better than in the past, because the old flow:

update buffer -> file created event (with empty read from disk) -> file changed event upon save (with proper content) meant the file was only usable after save. In reality, the initial update buffer already puts the workspace in the correct state because the file was added to workspace upon code action execution.

Now we have:
update buffer -> file created event (with empty read from disk), which is rejected
This means the file is usable already without save.

Copy link
Member

@filipw filipw left a comment

Choose a reason for hiding this comment

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

LGTM, in order to not block on this, I will merge this and we can swap the if order separately

edit: the cake tests failed.. let's see

- Remove CakeTextLoader as it will be triggered on 'document.GetTextAsync()' in BufferManader. This would force contents to be reloaded from disk and cached. Just updating buffer via UpdateBuffer requests seems like the safest approach here.
@bjorkstromm
Copy link
Member

bjorkstromm commented Nov 19, 2020

edit: the cake tests failed.. let's see

I've fixed the Cake side.

I removed CakeTextLoader as it would be triggered by document.GetTextAsync() in BufferManader. This would force contents to be reloaded from disk and cached in the Cake Scripting side. Just updating buffer in Cake Scripting via UpdateBuffer requests seems like the safest approach here.

Honestly, I can't remember the initial reason for creating the CakeTextLoader 😄

@filipw filipw merged commit 9086daf into OmniSharp:master Nov 19, 2020
@333fred 333fred deleted the filter-creates branch December 10, 2020 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants