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.

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

Conversation

mmisol
Copy link
Collaborator

@mmisol mmisol commented Apr 9, 2020

Purpose

View extensions that add a Window to the extension bar, run the risk of
leaving a window open when Dynamo closes, resulting in a zombie process
. The extension could avoid the problem in either of these two ways:

  1. Setting the Owner property of the Window they create to the one
    obtained from ViewLoadedParam.DynamoWindow.

  2. Calling the Close method on the Window they created when the
    Shutdown life-time method is called on the extension.

However, Dynamo should not rely on view extensions doing this. In order
to be able to still close no matter what, we assign the Owner property
explicitly in DynamoView.AddExtensionTabItem.

Declarations

Check these if you believe they are true

  • The codebase is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning and are documented in the API Changes document.

Reviewers

@QilongTang

FYIs

@mjkkirschner

View extensions that add a Window to the extension bar, run the risk of
leaving a window open when Dynamo closes, resulting in a zombie process
. The extension could avoid the problem in either of these two ways:

1. Setting the Owner property of the Window they create to the one
obtained from ViewLoadedParam.DynamoWindow.

2. Calling the Close method on the Window they created when the
Shutdown life-time method is called on the extension.

However, Dynamo should not rely on view extensions doing this. In order
to be able to still close no matter what, we assign the Owner property
explicitly in DynamoView.AddExtensionTabItem.
@mmisol mmisol requested a review from QilongTang April 9, 2020 16:23
};
if (SetOwner)
{
// Set the owner of the window to tuIDhe Dynamo window.
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you fix the word tuIDhe, I think you mean the?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's not mine, so I'll probably remove it :)

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

@QilongTang
Copy link
Contributor

LGTM. Let's fix the comments and merge

@mmisol mmisol merged commit 85c4120 into DynamoDS:master Apr 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants