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

Make sure extension window closes with Dynamo #10557

Merged
merged 2 commits into from
Apr 9, 2020
Merged

Make sure extension window closes with Dynamo #10557

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fff2eb9
Make sure extension window closes with Dynamo
mmisol Apr 9, 2020
c8b58ce
Fix comments
mmisol Apr 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/DynamoCoreWpf/Views/Core/DynamoView.xaml.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,12 @@ internal TabItem AddExtensionTabItem(IViewExtension viewExtension, ContentContro
else
{
tab.Content = contentControl.Content;
var extensionWindow = contentControl as Window;
if (extensionWindow != null)
{
// Make sure the extension window closes with Dynamo
extensionWindow.Owner = this;
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting.. I start to think this maybe why the fix did not work for me because when I tried it, I was calling close() specifically for Tuneup while your code should work for both TuneUp and NodePropertyPalette.

Copy link
Contributor

Choose a reason for hiding this comment

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

However, giving this more thoughts. I think this approach is excellent that Dynamo should handle it properly. We may still want to release a version of Tuneup which contain a close() call so that current users who are using Tuneup can be saved.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, that's why I opened DynamoDS/TuneUp#33. We should publish a new version after we merge that one.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mmisol Gotcha

}
}

//Insert the tab at the end
Expand Down
32 changes: 27 additions & 5 deletions test/DynamoCoreWpfTests/ViewExtensions/ViewExtensionTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,23 @@ public void OnWorkspaceChangedExtensionIsNotified()
Assert.AreEqual(2, viewExtension.Counter);
}

[Test]
public void ExtensionWindowIsClosedWithDynamo()
{
var dummyExtension = new DummyViewExtension()
{
SetOwner = false
};

RaiseLoadedEvent(this.View);
var extensionManager = View.viewExtensionManager;
extensionManager.Add(dummyExtension);

View.Close();

Assert.IsTrue(dummyExtension.WindowClosed);
}

[Test]
public void ExtensionsSideBarExtensionsTest()
{
Expand Down Expand Up @@ -63,7 +80,9 @@ public static void RaiseLoadedEvent(FrameworkElement element)

public class DummyViewExtension : IViewExtension
{
public int Counter = 0;
public int Counter { get; set; }
public bool SetOwner { get; set; } = true;
public bool WindowClosed { get; set; }

public string UniqueId
{
Expand All @@ -87,11 +106,15 @@ public void Loaded(ViewLoadedParams p)
Counter++;
};

var window = new Window
var window = new Window();
window.Closed += (sender, args) =>
{
// Set the owner of the window to tuIDhe Dynamo window.
Owner = p.DynamoWindow
WindowClosed = true;
};
if (SetOwner)
{
window.Owner = p.DynamoWindow;
}

p.AddToExtensionsSideBar(this, window);
}
Expand Down Expand Up @@ -138,7 +161,6 @@ public void Loaded(ViewLoadedParams p)

var window = new Window
{
// Set the owner of the window to tuIDhe Dynamo window.
Owner = p.DynamoWindow
};

Expand Down