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.

Sign up for GitHub

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

Jump to bottom

Apply changes directly to text buffer #62337

Merged
merged 1 commit into from
Jul 7, 2022
Merged

Apply changes directly to text buffer #62337

merged 1 commit into from
Jul 7, 2022

Conversation

tmat
Copy link
Member

@tmat tmat commented Jul 2, 2022 •
edited
Loading

Some command handlers apply changes to workspace via GetTextChangesAsync, which may block the UI thread until a background task is complete. This change updates these handlers to apply the changes directly to the editor buffer.

Contributes to #57554

Validation PR: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/409315

@tmat tmat force-pushed the SynchHandlers3 branch from d20469d to 8342412 Compare July 6, 2022 16:17
@tmat tmat force-pushed the SynchHandlers3 branch from 8342412 to fe76d42 Compare July 6, 2022 18:15
@tmat tmat changed the title Synch handlers3 Apply changes directly to text buffer Jul 6, 2022
@tmat tmat marked this pull request as ready for review July 6, 2022 21:57
@tmat tmat requested a review from a team as a code owner July 6, 2022 21:57
@tmat
Copy link
Member Author

tmat commented Jul 6, 2022

@CyrusNajmabadi PTAL

CyrusNajmabadi
CyrusNajmabadi reviewed Jul 6, 2022
View reviewed changes
src/EditorFeatures/Core/CommentSelection/AbstractCommentSelectionBase.cs

workspace.ApplyDocumentChanges(formattedDocument, cancellationToken);
transaction.Complete();
Copy link
Member

Choose a reason for hiding this comment

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

this is the part i'm least comfortable with. can you smoke test this otu a bit?

@genlu
Copy link
Member

genlu commented Jul 6, 2022

Some command handlers apply changes to workspace via GetTextChangesAsync...

I don't understand this part though, how to apply changes via GetTextChangesAsync?

@tmat
Copy link
Member Author

tmat commented Jul 6, 2022 •
edited
Loading

@genlu We have an extension method that does that:

roslyn/src/EditorFeatures/Core/Shared/Extensions/WorkspaceExtensions.cs

Line 24 in 315c2e1

var changes = newDocument.GetTextChangesAsync(oldDocument, cancellationToken).WaitAndGetResult(cancellationToken);

I should have perhaps said a combination of GetTextChangesAsync, UpdateDocument and TryApplyChanges.

Workspace.TryApplyChanges actually also runs async task and waits for it :-/

roslyn/src/Workspaces/Core/Portable/Workspace/Workspace.cs

Line 1218 in 2f9faa0

var solutionWithLinkedFileChangesMerged = newSolution.WithMergedLinkedFileChangesAsync(oldSolution, solutionChanges, cancellationToken: CancellationToken.None).Result;

@genlu
Copy link
Member

genlu commented Jul 6, 2022

Thanks for the explanation, looks like we'd see UI delay improvement in completion commit action for sure with this change👍

@tmat
Copy link
Member Author

tmat commented Jul 6, 2022

That's the hope :)

@tmat tmat merged commit 002abc1 into dotnet:main Jul 7, 2022
@tmat tmat deleted the SynchHandlers3 branch July 7, 2022 18:17
@ghost ghost added this to the Next milestone Jul 7, 2022
@runfoapp runfoapp bot mentioned this pull request Jul 7, 2022
Open
333fred added a commit to 333fred/roslyn that referenced this pull request Jul 7, 2022
@333fred
Merge remote-tracking branch 'upstream/main' into syntaxtree-search
998232c 
* upstream/main: (62 commits)
  Prevent assert from being hit (dotnet#62366)
  Don't offer '??=' for pointer types (dotnet#62476)
  Integrate generator times into /reportAnalyzer (dotnet#61661)
  Switch to a callback for cleaning up resources instead of passing in an explicit IDisposable. (dotnet#62373)
  Filter trees to only those containing global-usings or attributes prior to analyzing them. (dotnet#62444)
  Update PublishData.json
  Complete 'file' support for `SyntaxGenerator` (dotnet#62413)
  Apply changes directly to text buffer (dotnet#62337)
  Remove LangVer check from extended nameof binding (dotnet#62339)
  Fixed shared project file error (dotnet#62466)
  Handle new error codes
  Use MSBuid generated property for package path
  Exclude BCL libraries from Roslyn vsix
  Bump the integration test timeouts a bit
  Skip the balanced switch dispatch optimization for patterns on floating-point inputs (dotnet#62322)
  Test helpers shouldn't escape quotes by default (dotnet#62321)
  Reuse prior TableEntry values in the increment NodeTable builder if possible. (dotnet#62320)
  Install 3.1 runtime for SBOM tool
  Generate VS SBOM during official build.
  Minor refactoring in 'required' handling for override completion (dotnet#62422)
  ...
tmat added a commit to tmat/roslyn that referenced this pull request Jul 13, 2022
@tmat
Revert "Apply changes directly to text buffer (dotnet#62337)"
74550e9 
This reverts commit 002abc1.

Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1568401
tmat added a commit that referenced this pull request Jul 13, 2022
@tmat
Revert "Apply changes directly to text buffer (#62337)" (#62589)
0a39dcf 
This reverts commit 002abc1.

Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1568401
tmat added a commit to tmat/roslyn that referenced this pull request Jul 13, 2022
@tmat
Revert "Revert "Apply changes directly to text buffer (dotnet#62337)" (…
af73a4b 
…dotnet#62589)"

This reverts commit 0a39dcf.
tmat added a commit to tmat/roslyn that referenced this pull request Jul 13, 2022
@tmat
Revert "Revert "Apply changes directly to text buffer (dotnet#62337)" (…
1772681 
…dotnet#62589)"

This reverts commit 0a39dcf.
tmat added a commit to tmat/roslyn that referenced this pull request Jul 14, 2022
@tmat
Revert "Revert "Apply changes directly to text buffer (dotnet#62337)" (…
785204f 
…dotnet#62589)"

This reverts commit 0a39dcf.
tmat added a commit to tmat/roslyn that referenced this pull request Jul 14, 2022
@tmat
Revert "Revert "Apply changes directly to text buffer (dotnet#62337)" (…
98ade53 
…dotnet#62589)"

This reverts commit 0a39dcf.
tmat added a commit that referenced this pull request Jul 18, 2022
@tmat
Apply changes directly to text buffer - take 2 (#62617)
476248a 
* Revert "Revert "Apply changes directly to text buffer (#62337)" (#62589)"

This reverts commit 0a39dcf.

* Apply changes to ContainedDocument subject buffer
adamperlin pushed a commit to adamperlin/roslyn that referenced this pull request Jul 19, 2022
@tmat @adamperlin
Revert "Apply changes directly to text buffer (dotnet#62337)" (dotnet…
b89dc73 
…#62589)

This reverts commit 002abc1.

Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1568401
adamperlin pushed a commit to adamperlin/roslyn that referenced this pull request Jul 19, 2022
@tmat @adamperlin
Apply changes directly to text buffer - take 2 (dotnet#62617)
688cf62 
* Revert "Revert "Apply changes directly to text buffer (dotnet#62337)" (dotnet#62589)"

This reverts commit 0a39dcf.

* Apply changes to ContainedDocument subject buffer
@allisonchou allisonchou removed this from the Next milestone Jul 26, 2022
@allisonchou allisonchou added this to the 17.4 P1 milestone Jul 26, 2022
@ryzngard ryzngard mentioned this pull request Jul 27, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@genlu genlu genlu approved these changes

@CyrusNajmabadi CyrusNajmabadi CyrusNajmabadi approved these changes

Assignees
No one assigned
Labels
Area-IDE
Projects
None yet
Milestone
17.4 P1
Development

Successfully merging this pull request may close these issues.
4 participants
@tmat @genlu @CyrusNajmabadi @allisonchou