Skip to content
This repository has been archived by the owner on Oct 4, 2021. It is now read-only.

Fixes VSTS 945987: Format document doesn't work from Navigate To when using the new editor #8323

Merged
merged 4 commits into from
Aug 20, 2019
Merged

Fixes VSTS 945987: Format document doesn't work from Navigate To when using the new editor #8323

merged 4 commits into from
Aug 20, 2019

Conversation

avodovnik
Copy link
Contributor

This is the second attempt at fixing VSTS 945987, where Format Document doesn't work if invoked from the Navigate To window. The first PR #8265 attempted to resolve in a way that would allow commands to execute correctly on the handler, but as was pointed out, we also needed a way to allow native widgets to be focused.

I built on changes from here #7521 and extended the fix as it kept throwing exceptions.

Fixes https://vsmac.dev/945987

@avodovnik avodovnik added do-not-merge Area: VS Text Editor For issues and pull requests concerning the new native Cocoa VS Text Editor in Visual Studio for Mac labels Aug 2, 2019
This was referenced Aug 2, 2019
Closed
Merged
sevoku
sevoku approved these changes Aug 2, 2019
View reviewed changes
main/src/core/MonoDevelop.Ide/MonoDevelop.Components/Control.cs Outdated

// we used to throw a NotSupportedException, however, that prevented us from
// doing anything with native controls, see: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/945987
return null;
Copy link
Member

Choose a reason for hiding this comment

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

I'm ok with being less strict here, but I'm not sure if there was a specific reason to throw here. @Therzok ?

Copy link
Contributor

Choose a reason for hiding this comment

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

GetNativeWidget should be called only when we know it should contain that type. This change can be possibly reverted if the suggested change here is applied.

Copy link
Contributor

Choose a reason for hiding this comment

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

Basically, if we are requesting the bad control type, we should throw.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Depends. This can also serve as a pattern to check - i.e. can we get a type back. This is implemented on the majority of the editor classes, where you call .GetContent<T> etc., and if we don't support it, we return null. I guess it's got more to do with pattern + consistency. I'm more of a null guy myself, than an exception guy, but, again, if that's more of the convention across the rest of the codebase, I can revert - provided we also change the other parts to remove any implicit conversions.

main/src/core/MonoDevelop.Ide/MonoDevelop.Components/Window.cs
@@ -37,6 +37,30 @@ internal protected Window (object window) : base (window)
{
}

public bool IsRealized {
Copy link
Member

Choose a reason for hiding this comment

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

The naming here might be confusing, IsRealized means in the Gtk world that the widget is "ready"/initialized and has been added to a parent, but not necessarily visible (in fact showing a widget will realize it first), for a Gtk.Window this means that it has a Gdk.Window. The better match from the Cocoa world would be NSView.Superview != null, but I'm not sure about NSWindow...

Is this used somwhere? If not, maybe let's remove that? Otherwise IsVisible would make more sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I honestly "copied" this in from @jsuarezruiz's PR, so maybe he's in a better place to provide that comment? If there's no particular reason, I'll change it to IsVisible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sevoku looking at the code, it feels IsRealized is probably the subset of both methods - as in, it will call IsRealized in GTK world, and IsVisible in Cocoa world.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we just add an XML comment on here explaining, and leave as-is, since we're already in this GTK-focused frame of mind here.

@sevoku sevoku requested a review from Therzok August 2, 2019 14:15
Therzok
Therzok reviewed Aug 7, 2019
View reviewed changes
main/src/core/MonoDevelop.Ide/MonoDevelop.Components/Control.cs
else
throw new NotSupportedException ();

throw new NotSupportedException ($"Cannot get native widget {typeof (T)}");
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

Therzok
Therzok approved these changes Aug 7, 2019
View reviewed changes
Copy link
Contributor

@Therzok Therzok left a comment

Choose a reason for hiding this comment

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

LGTM!

@sandyarmstrong
Copy link
Member

@slluis this still requires a codeowner review from you, when you get a chance

@sandyarmstrong
Copy link
Member

@avodovnik let's hold this until test failures are figured out. Sevo says DDRITs and Azure Integration test failures all start with:

Xamarin.TestEnvironment.TestFailureException : Failed to open New Project Dialog

So it seems like something regressed here for test automation.

@slluis
Copy link
Member

slluis commented Aug 8, 2019

I relaunched the tests. Let's see what happens.

jsuarezruiz and others added 3 commits August 19, 2019 11:29
@jsuarezruiz @avodovnik
Fixes VSTS #801677 - Added support to native dialogs (Cocoa) in the C…
b2ad143 
…ommandManager
@avodovnik
Added support for native controls to be last focused widgets.
c1b25fa 
This meant changing the behavior of Control, when GetNativeWidget was
called. Until now, we threw a NotSupportedException, which meant that
any check would fail (i.e. x is Gtk.Container container) would throw that
exception, since it would try and convert.
@avodovnik
PR comments, restored throwing an exception vs returning null in cast.
71f17fe
@sandyarmstrong sandyarmstrong merged commit 5dd4752 into master Aug 20, 2019
@sandyarmstrong sandyarmstrong deleted the pr-anvod-formatnavigatefix2 branch August 20, 2019 13:22
@sandyarmstrong
Copy link
Member

@monojenkins backport release-8.3

@xamarin-release-manager xamarin-release-manager mentioned this pull request Aug 20, 2019
Closed
@Therzok Therzok mentioned this pull request Aug 21, 2019
Closed
@Therzok
Copy link
Contributor

Therzok commented Aug 28, 2019

@monojenkins backport release-8.3

@monojenkins monojenkins mentioned this pull request Aug 28, 2019
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sandyarmstrong sandyarmstrong sandyarmstrong left review comments

@Therzok Therzok Therzok approved these changes

@sevoku sevoku sevoku approved these changes

@slluis slluis Awaiting requested review from slluis

Assignees
No one assigned
Labels
Area: VS Text Editor For issues and pull requests concerning the new native Cocoa VS Text Editor in Visual Studio for Mac
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@avodovnik @sandyarmstrong @slluis @Therzok @sevoku @jsuarezruiz