Skip to content

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

Merged
3 commits merged into from
Feb 9, 2021
Merged

Smaple.cs LookForTypeByName(string) add assmbiles to search for types. #3740

3 commits merged into from
Feb 9, 2021

Conversation

Rosuavio
Copy link
Contributor

@Rosuavio Rosuavio commented Feb 5, 2021

Fixes #3739 #3735

Smaple.cs LookForTypeByName(string) add assmbiles to search in

  • 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

PR Type

What kind of change does this PR introduce?

  • Bugfix

What is the current behavior?

What is the new behavior?

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tested code with current supported SDKs
  • Pull Request has been submitted to the documentation repository instructions. Link:
  • Sample in sample app has been added / updated (for bug fixes / features)
  • New major technical changes in the toolkit have or will be added to the Wiki e.g. build changes, source generators, testing infrastructure, sample creation changes, etc...
  • Tests for the changes have been added (for bug fixes / features) (if applicable)
  • Header has been added to all new source files (run build/UpdateHeaders.bat)
  • Contains NO breaking changes

Other information

@ghost
Copy link

ghost commented Feb 5, 2021

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 🙌

@ghost ghost requested review from michael-hawker, azchohfi and Kyaa-dost February 5, 2021 21:27
@ghost ghost added the bug 🐛 An unexpected issue that highlights incorrect behavior label Feb 5, 2021
@Rosuavio Rosuavio marked this pull request as ready for review February 5, 2021 21:27
@michael-hawker
Copy link
Member

Nice find!

@michael-hawker
Copy link
Member

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;
Copy link
Member

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.

Copy link
Contributor Author

@Rosuavio Rosuavio Feb 8, 2021

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.
@Rosuavio
Copy link
Contributor Author

Rosuavio commented Feb 8, 2021

@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)
Plus, we write the assembly name right there anyways.

@Rosuavio
Copy link
Contributor Author

Rosuavio commented Feb 8, 2021

I could use AppDomain.CurrentDomain.GetAssemblies() to just look threw all loaded assemblies.
Or if we want to stay closer to the current behavior we can use only the assemblies we listed in the comments in the order we have them.

@michael-hawker
Copy link
Member

@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

@michael-hawker michael-hawker added this to the 7.0 milestone Feb 8, 2021
@ghost ghost added the in progress 🚧 label Feb 8, 2021
@michael-hawker
Copy link
Member

@Kyaa-dost want to just take this for a quick spin too?

@ghost
Copy link

ghost commented Feb 8, 2021

Hello @michael-hawker!

Because this pull request has the auto merge :zap: label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

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 (@msftbot) and give me an instruction to get started! Learn more here.

Copy link
Contributor

@Kyaa-dost Kyaa-dost left a comment

Choose a reason for hiding this comment

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

I am seeing few missing reference errors whenever I try to run the Sample App. @RosarioPulella are you able to run and operate the sample app?

image

@michael-hawker
Copy link
Member

@Kyaa-dost if you're switching branches between main and the dev/split-controls it's generally good to close VS and do a git clean -xdf in the root of the Toolkit folder where you have it checked-out. As otherwise there's weird intermediary things that remain which can cause weird issues like this. I've had it a few times from testing out other PRs based on main vs. all the refactor work we've done for the control package.

Copy link
Contributor

@Kyaa-dost Kyaa-dost left a 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 🚀

@ghost ghost merged commit be2ebd7 into CommunityToolkit:dev/split-controls Feb 9, 2021
@Rosuavio Rosuavio deleted the sample-app-fix-loopfortypebyname branch February 9, 2021 16:21
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto merge ⚡ bug 🐛 An unexpected issue that highlights incorrect behavior in progress 🚧 sample app 🖼
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Split Controls: InAppNotification smaple throws execption. Split Controls: TileControl not animating the Y-axis
3 participants