-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Smaple.cs LookForTypeByName(string) add assmbiles to search for types. #3740
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
Smaple.cs LookForTypeByName(string) add assmbiles to search for types. #3740
Conversation
Thanks RosarioPulella for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌 |
Nice find! |
You doing the Input one in #3727? |
assembly = controlsProxyType.GetType().GetTypeInfo().Assembly; | ||
// Search in Microsoft.Toolkit.Uwp.UI.Controls.Core | ||
var controlsCoreProxyType = StackMode.Replace; | ||
assembly = controlsCoreProxyType.GetType().GetTypeInfo().Assembly; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually was just thinking we could create a list at the top like:
// Assemblies to search for types from
var assemblies = new List<Assembly>() {
StackMode.Replace.GetType().GetTypeInfo().Assembly, // Microsoft.Toolkit.Uwp.UI.Controls.Core
...
};
foreach (var assembly in assemblies)
{
foreach (var typeInfo in assembly.ExportedTypes)
{
if (typeInfo.Name == typeName)
{
return typeInfo;
}
}
}
Would save some code. We may also want to think about a cache or something in the future, but we can do that in the re-write of the sample app in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know its not quite like your code, but I think using linq makes it a little more clear. Should run the same way.
- Microsoft.Toolkit.Uwp.UI.Controls -> Microsoft.Toolkit.Uwp.UI.Controls.Core + Microsoft.Toolkit.Uwp.UI.Controls.Layout + Microsoft.Toolkit.Uwp.UI.Controls.Media + Microsoft.Toolkit.Uwp.UI.Controls.Media
Should run more like the for loop version. Loads types from one assembly at a time.
@michael-hawker really quick, should I reference the assemblies directly instead of find them threw types in in the assemblies? I feel like, in splitting the controls package, using the types from inside the assemblies caused some changes to have some sneaky behavior (not checking ertain assemblies anymore because lookup types other assemblys) |
I could use |
@RosarioPulella I think that'll pull in a lot of other assemblies unrelated to the UI layer, so I think this is fine for now? We can re-evaluate later and better optimize when we look at the Sample App more for 7.1 |
@Kyaa-dost want to just take this for a quick spin too? |
Hello @michael-hawker! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Kyaa-dost if you're switching branches between main and the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @michael-hawker! Working fine now 🚀
Fixes #3739 #3735
Smaple.cs LookForTypeByName(string) add assmbiles to search in
PR Type
What kind of change does this PR introduce?
What is the current behavior?
What is the new behavior?
PR Checklist
Please check if your PR fulfills the following requirements:
Other information