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

Rename controls to core #3676

Merged
merged 21 commits into from
Feb 2, 2021
Merged

Rename controls to core #3676

merged 21 commits into from
Feb 2, 2021

Conversation

Rosuavio
Copy link
Contributor

@Rosuavio Rosuavio commented Jan 19, 2021

Step towards #3594

Rename the Microsoft.Toolkit.Uwp.UI.Controls project to Microsoft.Toolkit.Uwp.UI.Controls.Core

PR Type

What kind of change does this PR introduce?

  • Refactoring (no functional changes, no api changes)

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 Jan 19, 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 January 19, 2021 20:55
@Rosuavio Rosuavio changed the base branch from master to dev/split-controls January 20, 2021 21:35
@michael-hawker
Copy link
Member

@RosarioPulella while you're moving the files around, mind tackling #3606 at the same time? #3639 should be almost ready, so that should help finalize our work here.

@michael-hawker michael-hawker added this to the 7.0 milestone Jan 21, 2021
@Rosuavio Rosuavio marked this pull request as ready for review January 21, 2021 21:41
@Rosuavio Rosuavio mentioned this pull request Jan 21, 2021
8 tasks
@michael-hawker
Copy link
Member

Changes look good so far, I'll pull down the NuGets when they're ready and test things out.

@RosarioPulella I believe the core package calls out dependency on both the Primitives package and the Uwp package, but it probably really needs the Uwp.UI package? @azchohfi should we be more or less specific about the required package dependencies here (i.e. just rely on the Primitives which includes Uwp.UI, or also call out Uwp.UI explicitly in addition to the Primitives)?

@ghost ghost removed the needs attention 👋 label Jan 21, 2021
@michael-hawker
Copy link
Member

@RosarioPulella @vgromfeld here's the breakdown of the Core package:

Microsoft.Toolkit.Uwp.UI.Controls.Core Additional Footprint: 1,280,989 bytes
  App Diff: (.exe) = 12,800
  App Diff: (.xml) = 3,300
  App Diff: (.appxsym) = 197,485
  Size Diff: AppxBundleManifest.xml = 28
  Size Diff: AppxBlockMap.xml = 1,975
  Size Diff: AppxManifest.xml = 27
  Size Diff: resources.pri = 141,408
  Size Diff: System.Runtime.CompilerServices.Unsafe.dll = 1,536
  Lib (self): Microsoft.Toolkit.Uwp.UI.Controls.Core.xr.xml = 39,230
  Additional: Microsoft.Toolkit.dll = 96,768
  Additional: Microsoft.Toolkit.Uwp.dll = 104,448
  Lib (self): Microsoft.Toolkit.Uwp.UI.Controls.Core.dll = 494,080
  Additional: Microsoft.Toolkit.Uwp.UI.Controls.Primitives.dll = 42,496
  Additional: Microsoft.Toolkit.Uwp.UI.dll = 145,408
-----------------------------------------

It's still got a bit of a large footprint (some of that is mitigated if you're using the other helpers within the underlying dependencies).

Thoughts on the core controls still being a +0.5mb add though?

Lib (self): Microsoft.Toolkit.Uwp.UI.Controls.Core.dll = 494,080

Do we want to try and figure out how to bucketize these into two separate ones or is this going to be sufficient?

Copy link
Member

@michael-hawker michael-hawker left a comment

Choose a reason for hiding this comment

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

Tested out the Sample App and the package stand-alone in VS, everything looking good. Think the main question is if this a granular enough package or do we want to look at having 2 buckets for the final set of controls???

@vgromfeld
Copy link
Contributor

It could be great to split the package in two and have the less used controls in a dedicated package.
Some controls like OrbitView or CameraPreview may be less used than the InAppNotification but this can be subjective and will depend on what each of us uses from the toolkit 😊

@michael-hawker
Copy link
Member

It could be great to split the package in two and have the less used controls in a dedicated package.
Some controls like OrbitView or CameraPreview may be less used than the InAppNotification but this can be subjective and will depend on what each of us uses from the toolkit 😊

Interesting idea @vgromfeld. We didn't really consider usage as a metric in the discussion of #3594 around how to pivot the remaining controls. The best we had at the moment was those that required more 'Content' vs. those that were more 'Standalone', though that's a bit iffy on a few of them around which category they fall under.

It would make it hard to know where to put a 'new' control though if we did it metric based as there would be no metrics to go off of. I think we'd have to do something more clearly defined so that we and the community know where to put things (and find them).

Wonder if I could make a quick survey that would let folks put the controls into those two buckets and we could have a few people fill it out so we could compare our thoughts to how to bucketize controls against one another?

Copy link
Contributor Author

@Rosuavio Rosuavio left a comment

Choose a reason for hiding this comment

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

mind tackling #3606 at the same time?

Renamed the MasterDetailsView -> ListDetailsView

<PackageTags>UWP Toolkit Windows Controls XAML Range Markdown BladeView Blade CameraPreview Camera Carousel DropShadow Expander GridSplitter HeaderedContent ImageEx InAppNotification InfiniteCanvas Master Details MasterDetails Orbit Radial Gauge RadiaGauge RadialProgressBar Scroll ScrollHeader Tile Tokenizing TextBox</PackageTags>
<PackageTags>UWP Toolkit Windows Controls XAML Range Markdown BladeView Blade CameraPreview Camera Carousel DropShadow Expander GridSplitter HeaderedContent ImageEx InAppNotification InfiniteCanvas List Details ListDetails Orbit Radial Gauge RadiaGauge RadialProgressBar Scroll ScrollHeader Tile Tokenizing TextBox</PackageTags>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@michael-hawker should we keep the old Tags for searchablity?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think so? I think we'll have to rely on getting the word out elsewhere?

@michael-hawker
Copy link
Member

Just updated this based on the #3702 PR, so now the Smoke Test analysis should show us the impact of the new core package. I'll do some further analysis once this builds and we can decide if we should break things into more buckets or not before we nail this down.

@Rosuavio
Copy link
Contributor Author

Rosuavio commented Feb 1, 2021

@michael-hawker @azchohfi This PR is ready, we still need #3689 before we merge into master but getting this merged into the feature branch should make things easier.

@michael-hawker
Copy link
Member

@RosarioPulella think we should merge this in first even as we figure out our last steps for #3594?

@Rosuavio
Copy link
Contributor Author

Rosuavio commented Feb 1, 2021

Ether way the Controls package is supposed to be for the meta package. It seems like a necessary change regardless.

@ghost
Copy link

ghost commented Feb 1, 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.

@michael-hawker
Copy link
Member

Yeah, I'm all good with this intermediary change then as a starting point, and we'll just move controls further if we split things up more. I believe @azchohfi took a look through here quick, so I'll have him sign-off too and we'll move forward from here.

This is still all in our dev branch anyway.

@michael-hawker michael-hawker merged commit 57e42fe into CommunityToolkit:dev/split-controls Feb 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto merge ⚡ controls 🎛️ for-review 📖 To evaluate and validate the Issues or PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants