Split out Controls.Media package.

[Rosuavio]

Step towards #3594

Split out Eyedropper, ImageCropper, InfiniteCanvas to Microsoft.Toolkit.Uwp.UI.Controls.Media package.

PR Type

What kind of change does this PR introduce?

• Feature

What is the current behavior?

Eyedropper, ImageCropper, InfiniteCanvas are in Microsoft.Toolkit.Uwp.UI.Controls

What is the new behavior?

Eyedropper, ImageCropper, InfiniteCanvas are in Microsoft.Toolkit.Uwp.UI.Controls.Media

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)
  • Icon has been created (if new sample) following the Thumbnail Style Guide and templates
• 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

Issue: coped ControlHelpers.XamlHost.cs, Controls -> Controls.Media Not an issue

[ghost]

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 🙌

[Rosuavio]

@michael-hawker
Hey I copied
Microsoft.Toolkit.Uwp.UI.Controls/ControlHelpers.XamlHost.cs -> Microsoft.Toolkit.Uwp.UI.Controls.Media/ControlHelpers.XamlHost.cs
Because it was needed in some of the media controls. I know this is not the best solution, what would be best?
Make it public in Controls and projref Controls?
Move it to Microsoft.Toolkit.Uwp.UI and make it public?
Move it to Microsoft.Toolkit.Uwp.UI and make it internal but use the InternalsVisibleToAttribute?
Something else?

[michael-hawker]

@RosarioPulella good question. Since it's just a one-line internal helper, I think we could duplicate it in both projects if we'd like vs. trying to move it into the UI package.

The other solution is that we just include the original ApiInformation.IsPropertyPresent call directly in the places it's used.

@azchohfi what are your thoughts on this? Is this something we'll still be using in WinUI 3? No, eh as it'll just always be available?

[Rosuavio]

I had to make 0869c79 to make a test pass but I think I means that simply moving the InfiniteCanvas to another package/assembly makes it backwards incompatible? @azchohfi

[azchohfi]

XamlRoot will always be available on WinUI3, so this code can/should be removed on our WinUI3 branch. Right now, on the winui branch, we are copying the winmds to make sure this code works as expected, but this API doesn't make much sense on net5.
For this PR, I'm ok copying the code (and changing the namespace), since it is internal, and only one line.

[Rosuavio]

Its not being picked up by CI but I am getting issues with the xaml island tests.

System.ArgumentException
  HResult=0x80070057
  Message=Value does not fall within the expected range.
  Source=Windows.UI.Xaml
  StackTrace:
   at Windows.UI.Xaml.Controls.Primitives.Popup.put_IsOpen(Boolean value)
   at Microsoft.Toolkit.Uwp.UI.Controls.Eyedropper.Eyedropper_Unloaded(Object sender, RoutedEventArgs e) in C:\Users\Rosario\source\repos\WindowsCommunityToolkit\Microsoft.Toolkit.Uwp.UI.Controls.Media\Eyedropper\Eyedropper.cs:line 324

>	Microsoft.Toolkit.Uwp.UI.Controls.Media.dll!Microsoft.Toolkit.Uwp.UI.Controls.Eyedropper.Eyedropper_Unloaded(object sender, Windows.UI.Xaml.RoutedEventArgs e) Line 324	C#
 	[External Code]	
 	UnitTests.XamlIslands.dll!UnitTests.XamlIslands.Program.Main() Line 28	C#
 	[External Code]	
 	hostpolicy.dll!coreclr_t::execute_assembly(int argc, const char * * argv, const char * managed_assembly_path, unsigned int * exit_code) Line 145	C++
 	hostpolicy.dll!run_app_for_context(const hostpolicy_context_t & context, int argc, const wchar_t * * argv) Line 247	C++
 	hostpolicy.dll!run_app(const int argc, const wchar_t * * argv) Line 276	C++
 	hostpolicy.dll!corehost_main(const int argc, const wchar_t * * argv) Line 390	C++
 	hostfxr.dll!execute_app(const std::wstring & impl_dll_dir, corehost_init_t * init, const int argc, const wchar_t * * argv) Line 146	C++
 	hostfxr.dll!`anonymous namespace'::read_config_and_execute(const std::wstring & host_command, const host_startup_info_t & host_info, const std::wstring & app_candidate, const std::unordered_map<enum known_options,std::vector<std::wstring,std::allocator<std::wstring>>,known_options_hash,std::equal_to<enum known_options>,std::allocator<std::pair<enum known_options const ,std::vector<std::wstring,std::allocator<std::wstring>>>>> & opts, int new_argc, const wchar_t * * new_argv, host_mode_t mode, wchar_t * out_buffer, int buffer_size, int * required_buffer_size) Line 521	C++
 	hostfxr.dll!fx_muxer_t::handle_exec_host_command(const std::wstring & host_command, const host_startup_info_t & host_info, const std::wstring & app_candidate, const std::unordered_map<enum known_options,std::vector<std::wstring,std::allocator<std::wstring>>,known_options_hash,std::equal_to<enum known_options>,std::allocator<std::pair<enum known_options const ,std::vector<std::wstring,std::allocator<std::wstring>>>>> & opts, int argc, const wchar_t * * argv, int argoff, host_mode_t mode, wchar_t * result_buffer, int buffer_size, int * required_buffer_size) Line 1001	C++
 	hostfxr.dll!fx_muxer_t::execute(const std::wstring host_command, const int argc, const wchar_t * * argv, const host_startup_info_t & host_info, wchar_t * result_buffer, int buffer_size, int * required_buffer_size) Line 580	C++
 	hostfxr.dll!hostfxr_main_startupinfo(const int argc, const wchar_t * * argv, const wchar_t * host_path, const wchar_t * dotnet_root, const wchar_t * app_path) Line 50	C++
 	UnitTests.XamlIslands.exe!exe_start(const int argc, const wchar_t * * argv) Line 220	C++
 	UnitTests.XamlIslands.exe!wmain(const int argc, const wchar_t * * argv) Line 287	C++
 	[External Code]	

@azchohfi are those test being run on CI? Are they supposed to run on CI?

[Rosuavio]

I seems that there are no classes in the DesignTools project for the Media controls.
https://github.com/windows-toolkit/WindowsCommunityToolkit/tree/master/Microsoft.Toolkit.Uwp.UI.Controls.Design/Controls
Do we still make a Controls.Media.DesignTools project? Just have an "empty" one? Create the Metadata and TypeData classes?
@michael-hawker @Nirmal4G

[Rosuavio]

@azchohfi @michael-hawker Is b298fd0 a bit better for now? Or just leave it?

[Nirmal4G]

@RosarioPulella I used a custom made generator project with lot of hacks to generate them but haven't refined the logic yet. So, leave it as it is. I'll add them post this PR.

Is that okay with you @michael-hawker?

[michael-hawker]

Sounds good, thanks @Nirmal4G for taking a look. This should be the last split out, then we just have to bundle what's left into a different named package before we re-create the '.Controls' package as a container for the Primitives, Core, Media, Markdown, and Layout sub-packages under the Controls namespace.

After that we can ensure we resolve any loose threads for things we've missed in the Design projects. I know I should add some for the new SwitchPresenter as well.

[michael-hawker]

@azchohfi talking to Rosario, this appears to be the assembly name. So this is going to break compatibility with previously exported InfiniteCanvas files, eh? This is going to be an issue for apps using this control, right?

Any thoughts on how we may be able to mitigate this? This is used directly by System.Text.Json right? Is there some custom Type resolver we could provide to help it do the migration?

[azchohfi]

We already have a custom type resolver:
https://github.com/windows-toolkit/WindowsCommunityToolkit/blob/0d43da7ae459d000ee29dcb108cc08a56bf5073f/Microsoft.Toolkit.Uwp.UI.Controls/InfiniteCanvas/JsonConverters/IDrawableConverter.cs#L43-L50
We could either introduce a new version or just try to handle this here. I think it makes more sense to introduce a new version.

[Rosuavio]

e4db504 Added a check for the old Discriminator.

[azchohfi]

@azchohfi are those test being run on CI? Are they supposed to run on CI?

They are not supposed to be executed on CI. No idea what is going on.

[Rosuavio]

TODO: Should actually generate new file.

[Rosuavio]

Currently there are a issues with this branch.

1. After doing some work try to test the sample app with these changes (temporarily revert 633ceef due to Sample App: OnXamlRendered does not get called. #3673, and remove graph controls from sample) My temp test branch
   I found some issues with the Media controls in the Sample app, I am getting messages like these in the sample app on the live xaml editor.

Failed to assign to property 'Windows.UI.Xaml.ResourceDictionary.Source' because the type 'Windows.Foundation.String' cannot be assigned to the type 'Windows.Foundation.Uri'. [Line: 13 Position: 10]
Cannot locate resource from 'ms-appx:///Microsoft.Toolkit.Uwp.UI.Controls/Eyedropper/Eyedropper.xaml'. [Line: 13 Position: 10]

2. The XamlIslands test fails now.

Not sure if they are blockers as this is not going into master yet.

[Rosuavio]

c61156d seemed to have fixed XamlIslands issues and the Sample app issues ( other than the graph controls issues and the #3673)

[michael-hawker]

@RosarioPulella I guess the Sample App is still broken due to the Graph Controls? I believe if you just remove the package reference that may be mostly enough? And comment out the thing that adds it to the namespace bucket we use for type loading? Or do we want to leave it broken until we fix the Graph Control problem? Just figured it's be nice to test the stuff that should still be working?

Also, shouldn't the animation packaged removed as a dependency now? I'm seeing this on the base 'Controls' package install:

Making sure we can run the sample would be handy, as I hit this crash trying to use the ColorPickerButton from the remaining Controls package:

Windows.UI.Xaml.Markup.XamlParseException
  HResult=0x802B000A
  Message=The text associated with this error code could not be found.

Cannot find a Resource with the Name/Key OverlayCornerRadius [Line: 21 Position: 10]
  Source=Windows
  StackTrace:
   at Windows.UI.Xaml.Application.LoadComponent(Object component, Uri resourceLocator, ComponentResourceLocation componentResourceLocation)
   at ControlPackagesTest.MainPage.InitializeComponent() in C:\Users\source\repos\ControlPackagesTest\ControlPackagesTest\obj\x86\Debug\MainPage.g.i.cs:line 35
   at ControlPackagesTest.MainPage..ctor() in C:\Users\source\repos\ControlPackagesTest\ControlPackagesTest\MainPage.xaml.cs:line 27

@azchohfi thoughts on if we could have the Smoke Test project also run the app somehow like TAEF does? Like just load each one and check that it loaded the main page without crashing? Or should we just beef up our integration tests to have at least basic scenarios for trying to load each control, that may be the better approach, eh? Or both?

Note: A future improvement we can make too is to add the EditorBrowsable attribute to some of the helper controls which folks probably don't want to be using (maybe I should file a separate issue? like ImageCropperThumb, and the two InfiniteCanvas*):

Otherwise the Media package on its own is looking good. 👍

[michael-hawker]

Was just thinking the ColorPicker crash may be an assumption that there's a WinUI 2 resource... FYI @robloo, not sure if you tested without WinUI 2.x resources?

[robloo]

@michael-hawker The ColorPickerButton was added with the assumption that WinUI 2 wasn't available. I didnt realize WCT had a dependency on it - thought you were only updating the target platform to 1809. So yes, it was tested without in August/September last year.

That said, when you separated out the Xaml and then added DropDownButton and SwitchPresenter was that after taking a WinUI dependency? I did not review all those Xaml changes. It could have slipped in there.

[Rosuavio]

Also, shouldn't the animation packaged removed as a dependency now? I'm seeing this on the base 'Controls' package install:

I would love to remove it as a dep, but...
https://github.com/windows-toolkit/WindowsCommunityToolkit/blob/147000a4fed8ff9c38659d7fdae25ac4441eddcf/Microsoft.Toolkit.Uwp.UI.Controls/ScrollHeader/ScrollHeader.cs#L6
https://github.com/windows-toolkit/WindowsCommunityToolkit/blob/147000a4fed8ff9c38659d7fdae25ac4441eddcf/Microsoft.Toolkit.Uwp.UI.Controls/ScrollHeader/ScrollHeader.cs#L74

[michael-hawker]

@michael-hawker The ColorPickerButton was added with the assumption that WinUI 2 wasn't available. I didnt realize WCT had a dependency on it - thought you were only updating the target platform to 1809. So yes, it was tested without in August/September last year.

That said, when you separated out the Xaml and then added DropDownButton and SwitchPresenter was that after taking a WinUI dependency? I did not review all those Xaml changes. It could have slipped in there.

Good to know. We don't have a dependency on WinUI 2.x yet, but we did update to 1809. I don't remember making any style changes with resources. We'll investigate.

@RosarioPulella ah, I thought that change to remove ScrollHeader was in the main branch still, it's fixed in the dev/animations branch work. We've been waiting to get that merged, so guess we'll have to wait to finish that. I'll investigate if we can start closing down #3639 today. @Sergio0694 we're just waiting for any last feedback on it, eh? Or do we have something else left to implement?

[Sergio0694]

@michael-hawker I think we have pretty much all the initial features we wanted ready to go now, so yeah I was mostly waiting for some feedbacks at this point, but the PR itself is ready for review. Even if we wanted to add something small later down the line, it'd be just a matter of tweaking a few bits here and there, but I wouldn't expect any major changes at this point. 🙂

[ghost]

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.

[azchohfi]

@azchohfi thoughts on if we could have the Smoke Test project also run the app somehow like TAEF does? Like just load each one and check that it loaded the main page without crashing? Or should we just beef up our integration tests to have at least basic scenarios for trying to load each control, that may be the better approach, eh? Or both?

@michael-hawker The Smoke tests should be as simple as possible. I wouldn't do any complex logic in them at all. In the future we could even parallelize them into different machines. Since it makes sense to have more tests for each control, I would put them in the integration tests.

[michael-hawker]

Awesome, took the sample app for a spin, looking good!

[michael-hawker]

I'm going to merge this so we can then focus on getting the Animations PR merged and then updating this branch so we can make the final changes all-up? Sound good @RosarioPulella @Sergio0694 @azchohfi?