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

Fix location show form owner inside Search for packages #12540

Merged
merged 14 commits into from
Feb 24, 2022

Conversation

chuongmep
Copy link
Contributor

@chuongmep chuongmep commented Jan 13, 2022

Purpose

Fix location show form oner inside package search form.Display appears very annoying when using two or more monitors
Before :
When we use one or two display , it not show center to check , it can show form to other screen
DynamoSandbox_QV4Wk9YYtQ
DynamoSandbox_hvob39IO2K

After fix :
Form show center owner of package manager for user.
DynamoSandbox_Pi9TLWrFAc
DynamoSandbox_ffxmalODCi

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.
  • This PR modifies some build requirements and the readme is updated

Reviewers

@QilongTang

FYIs

@Amoursol

@chuongmep chuongmep changed the title Fix location show form oner inside Search for packages Fix location show form owner inside Search for packages Jan 13, 2022
@Amoursol
Copy link
Contributor

Brilliant addition @chuongmep - thank you very much for this 😁

@mjkkirschner
Copy link
Member

Hi @chuongmep - there are some failing tests.
Screen Shot 2022-01-18 at 2 42 51 PM

@chuongmep
Copy link
Contributor Author

me failing tests.

@mjkkirschner May be it a problem other, this is when i check the existing code on the branch
image

@mjkkirschner
Copy link
Member

Hi @chuongmep - you can read some of the docs here, though they are a bit out of date, you need to set your test runtime engine version to x64 from the test menu.
https://developer.dynamobim.org/04-Testing/4-0-testing.html

@QilongTang QilongTang added this to the 2.14.0 milestone Feb 15, 2022
@chuongmep chuongmep closed this Feb 19, 2022
@QilongTang
Copy link
Contributor

QilongTang commented Feb 23, 2022

hi @chuongmep any reason why you closed this PR? I think these are still valid improvements, Let me know if I can help you with this

@chuongmep
Copy link
Contributor Author

@QilongTang I can't complete Testing NUnit 2 Test Adapter from report of @mjkkirschner in my computer with VS 2022, so I had closed this, if team can do that, let open again, thanks you !

@mjkkirschner
Copy link
Member

mjkkirschner commented Feb 23, 2022 via email

@@ -675,7 +677,7 @@ internal async void ExecutePackageDownload(string name, PackageVersion package,
String.Format(Resources.MessageConfirmToInstallPackage, name, package.version) :
String.Format(Resources.MessageConfirmToInstallPackageToFolder, name, package.version, installPath);

var result = MessageBoxService.Show(msg,
var result = MessageBoxService.Show(Owner,msg,
Copy link
Contributor

Choose a reason for hiding this comment

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

I find out that adding the owner here would break those 8 tests. Here is the error message, @pinzart Can you help us since you added those tests? Do we need to modify the tests based on these changes?
image

Copy link
Contributor

@pinzart90 pinzart90 Feb 24, 2022

Choose a reason for hiding this comment

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

We probably need to match the new function signatures
The failing tests just need to update the expected MessageBox signatures to include the Window argument
ex:

from: m.Show(It.IsAny < string > (), It.IsAny<string>(), It.IsAny<MessageBoxButton>())
to: m.Show(It.IsAny < Window >(), It.IsAny<string>(), It.IsAny<string>(), It.IsAny<MessageBoxButton>())

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you @pinzart I will give that a try

@QilongTang
Copy link
Contributor

All tests fixed locally and waiting for verification from https://master-15.jenkins.autodesk.com/view/DYN/job/DYN-DevCI_Self_Service/932/

@QilongTang
Copy link
Contributor

@QilongTang QilongTang merged commit 99f4cd7 into DynamoDS:master Feb 24, 2022
@chuongmep
Copy link
Contributor Author

Nice to see this done, thanks a lot.

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.

5 participants