Skip to content

Share IWpfDifferenceViewerExtensions with EditorFeatures.Cocoa#51759

Merged
sharwell merged 1 commit intodotnet:mainfrom
sharwell:port-tasks-change
Mar 9, 2021
Merged

Share IWpfDifferenceViewerExtensions with EditorFeatures.Cocoa#51759
sharwell merged 1 commit intodotnet:mainfrom
sharwell:port-tasks-change

Conversation

@sharwell
Copy link
Contributor

@sharwell sharwell commented Mar 9, 2021

No description provided.

@sharwell sharwell requested a review from a team as a code owner March 9, 2021 14:30
@sharwell sharwell requested a review from a team March 9, 2021 14:30
@ghost ghost added the Area-IDE label Mar 9, 2021
@sharwell sharwell force-pushed the port-tasks-change branch from 9207761 to 8af7125 Compare March 9, 2021 14:44
diffViewer.ViewMode = mode;

// This code path must be invoked on UI thread.
AssertIsForeground();
Copy link
Contributor

Choose a reason for hiding this comment

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

All the foreground assertions are now gone?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Foreground assertions are unnecessary in asynchronous code. If code needs a specific thread, and can and should switch to that thread. The caller's thread is generally irrelevant.

Copy link
Contributor

@Therzok Therzok Mar 9, 2021

Choose a reason for hiding this comment

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

Yup, I saw some codepaths did not change to switch to the main thread, but I assume this was because they're only called inside the UI thread context.

LGTM

Copy link
Member

@jasonmalinowski jasonmalinowski left a comment

Choose a reason for hiding this comment

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

I can confirm this makes the code look like what's in IWpfDifferenceViewerExtensions, but I didn't actually confirm if that makes any sense in the first place. I'd be curious what actually sharing the code under an #ifdef looks like, but this is an improvement at least.

@sharwell
Copy link
Contributor Author

sharwell commented Mar 9, 2021

@jasonmalinowski Sharing mostly works. I used that approach originally, but switched to this approach due to differences on these lines:

// We have the height and width required to display the inline diff snapshot now.
// Set the height and width of the ICocoaDifferenceViewer accordingly.
_diffViewer.VisualElement.SetFrameSize(new CoreGraphics.CGSize(_width, _height));
_diffViewer.VisualElement.Subviews[0].SetFrameSize(new CoreGraphics.CGSize(_width, _height));

I figure if we do share in the future, this change makes that sharing easier.

@sharwell sharwell merged commit 1f56b5f into dotnet:main Mar 9, 2021
@sharwell sharwell deleted the port-tasks-change branch March 9, 2021 18:48
@jasonmalinowski
Copy link
Member

@sharwell: yeah, in that case because it's small enough my preference is we share the file and #if COCOA since that way the difference between them is far clearer. But doing what you did is absolutely a great first step.

@allisonchou allisonchou added this to the Next milestone Mar 12, 2021
@allisonchou allisonchou modified the milestones: Next, 16.10.P2 Mar 29, 2021
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.

4 participants