Skip to content

Fix VisualStudioActiveDocumentTracker to handle frames without text b…#58049

Merged
mavasani merged 2 commits intodotnet:mainfrom
mavasani:ActiveDocTracker
Dec 9, 2021
Merged

Fix VisualStudioActiveDocumentTracker to handle frames without text b…#58049
mavasani merged 2 commits intodotnet:mainfrom
mavasani:ActiveDocTracker

Conversation

@mavasani
Copy link
Copy Markdown
Contributor

@mavasani mavasani commented Dec 1, 2021

…uffer

There are cases where we get multiple IVsSelectionEvents.OnElementValueChanged callbacks for the same frame, some of which do not have __VSFPROPID.VSFPROPID_DocData leading to null text buffer. This leads to TryGetActiveDocument to return null even when we have an active frame. We now handle this case and ensure we swap a visible frame without text buffer when possible.

Related fix done last month: #57206

…uffer

There are cases where we get multiple `IVsSelectionEvents.OnElementValueChanged` callbacks for the same frame, some of which do not have `__VSFPROPID.VSFPROPID_DocData` leading to null text buffer. This leads to `TryGetActiveDocument` to return null even when we have an active frame.
@mavasani mavasani added this to the 17.1.P2 milestone Dec 1, 2021
@mavasani mavasani requested a review from a team as a code owner December 1, 2021 04:41
}
else if (existingFrame.TextBuffer == null)
{
// If no text buffer is associated with existing frame, remove the existing frame and add the new one.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this because there's some more async buffer creation now, and we're just looking for a text buffer too early?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I am honestly not sure, but we do get multiple callbacks where sometimes getting the DocData property succeeds and sometimes it doesn't. If you'd like, I can create a separate issue for someone more familiar with this space (probably you?) to debug through this.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

OK to merge this, but I'd say reach out to the extensibility discussion internal list and see what they have to say.

@mavasani
Copy link
Copy Markdown
Contributor Author

mavasani commented Dec 3, 2021

@jasonmalinowski Any further feedback?

@mavasani mavasani enabled auto-merge December 4, 2021 04:49
@mavasani
Copy link
Copy Markdown
Contributor Author

mavasani commented Dec 9, 2021

@dotnet/roslyn-infrastructure Is Test_Linux_Debug broken? It is failing on my PR after multiple retries with failures in compiler unit tests, but the PR only has IDE changes.

@jasonmalinowski
Copy link
Copy Markdown
Member

jasonmalinowski commented Dec 9, 2021

@mavasani You're running into #58077, since retries don't include newer source, you aren't going to get the disabling of the test.

@jasonmalinowski
Copy link
Copy Markdown
Member

/azp run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 4 pipeline(s).

@mavasani mavasani merged commit f619079 into dotnet:main Dec 9, 2021
@ghost ghost modified the milestones: 17.1.P2, Next Dec 9, 2021
@mavasani mavasani deleted the ActiveDocTracker branch December 10, 2021 02:02
@Cosifne Cosifne modified the milestones: Next, 17.1.P3 Jan 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants