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

[Feature] Split up the controls package. #3594

Closed
Rosuavio opened this issue Dec 2, 2020 · 33 comments · Fixed by #3660 or #3752
Closed

[Feature] Split up the controls package. #3594

Rosuavio opened this issue Dec 2, 2020 · 33 comments · Fixed by #3660 or #3752
Assignees
Labels
Completed 🔥 feature request 📬 A request for new changes to improve functionality
Milestone

Comments

@Rosuavio
Copy link
Contributor

Rosuavio commented Dec 2, 2020

Describe the problem this feature would solve

There are many different kinds of controls in the controls package and there are many different levels of abstraction between the controls.

From a technical perspective, users have to pull in the whole package and all its dependencies (like Win2d and xmal.behaviors) just to get access to simple controls (like WrapPanel).

From a perspective of usability, having all our controls grouped together under "controls"

  1. Makes the library as a whole less "understandable" as only a very general theme unifies it and consumers are not introduced to a specific domain.
  2. Makes it harder to find our controls that do address specific domains in collections of packages. As in if a user is searching for a package to help with "layout" related tasks they most likely won't stumble across our package even though we have a substantial group of controls/panels that only address layout.

Describe the solution

Split the controls package.

We can either split things based on their "weight", splitting general/minimally dependent/simple controls from opinionated/heavily dependent/complex controls.

Or split the controls based on the domains or problem spaces they live in. Domains like "Layout", "Input" or maybe "Image capture/creation/examination/manipulation"

Whatever way seems like the best, I want to see the package split up into more focused categories.

Describe alternatives you've considered

  1. Keep the package as it is.
  2. Have a separate package per control.

Analysis

Control For Layout Standalone Needs content Provides Interactions Styleless Major Deps
AdaptiveGridView x x x
DockPanel x x x
StaggeredPanel x x x
UniformGrid x x x
WrapPanel x x x
SwitchPresenter x x x
LayoutTransformControl x x
HeaderedContentControl x x
HeaderedItemsControl x x
Carousel x x x
MasterDetailsView ? x x
OrbitView ? x x Animation #3627
BladeView ? x x
Expander ? x x
InAppNotification ? x x
RotatorTile ? x
DropShadowPanel x
ScrollHeader x Animation, Behaviors
TileControl ? x Animation #3633
TextToolbar ? x
GridSplitter ? x
ImageEx x
Menu x x
Loading x
RadialProgressBar x
CameraPreview x
ColorPicker x x
RadialGauge x x
RangeSelector x x Animation #3611
RemoteDevicePicker x x
TokenizingTextBox x x
ImageCropper x x Win2d
Eyedropper x x Win2d
InfiniteCanvas x x Win2d

Related to #3145

@Rosuavio Rosuavio added the feature request 📬 A request for new changes to improve functionality label Dec 2, 2020
@ghost
Copy link

ghost commented Dec 2, 2020

Hello, 'RosarioPulella! Thanks for submitting a new feature request. I've automatically added a vote 👍 reaction to help get things started. Other community members can vote to help us prioritize this feature in the future!

@Rosuavio
Copy link
Contributor Author

Rosuavio commented Dec 4, 2020

I should mention my personal thoughts are that

  1. We should group them based on domains.
  2. We should at least split off a "Layout" package containing most of the controls that are labeled "For Layout" or "Needs content" as most of them are very lightweight and "layout" seems like a very important domain.

@michael-hawker
Copy link
Member

@LanceMcCarthy pointed us to the Xamarin Forms breakdown here: https://docs.microsoft.com/en-us/xamarin/xamarin-forms/user-interface/controls/views

This has Expander as more of a 'Content' control/View vs. a Layout specifically.

Wonder if we want to be more fine-grained with 'Needs Content' to be a specific 'Content' vs. 'Items'?

@mrlacey
Copy link
Contributor

mrlacey commented Dec 4, 2020

The sample app already has groupings, but not everything listed above shows up in the sample app :/

@Rosuavio
Copy link
Contributor Author

Rosuavio commented Dec 4, 2020

Other than the major categories, I'm curious what everyone thinks of having a separate "Panels" package separate from any "Layout" package, as they seem to distinguish themselves as not having any really visual appearance just a strategy for placing children.

Particularly I think AdaptiveGridView, DockPanel, StaggeredPanel, UniformGrid, WrapPanel would be our "Panels" (or whatever we want to name it). But something like the Carousel or the OribitView already has a strong visual style and in comparison to the "Panels" and is more opinionated/less general purpose.

@michael-hawker
Copy link
Member

The sample app already has groupings, but not everything listed above shows up in the sample app :/

@mrlacey yeah our Sample App groupings are a bit 'fuzzy' too, so they may need to be restructured based on decisions here. But it could be an interesting thing to evaluate what we did in the past there with this matrix.

Curiously, besides SwitchPresenter was something else missing from the Sample App? (I need to create the sample for that control still it's new for 7.0.)

@mrlacey
Copy link
Contributor

mrlacey commented Dec 4, 2020

Curiously, besides SwitchPresenter was something else missing from the Sample App? (I need to create the sample for that control still it's new for 7.0.)

just SwitchPresenter

@mrlacey
Copy link
Contributor

mrlacey commented Dec 4, 2020

Are there separate plans (& issues) for trying to remove the dependencies? If there are alternatives?

@michael-hawker
Copy link
Member

@mrlacey the only one I believe I filed so far is #3578 for the Animations package. I thought it was just RangeSelector and ScrollHeader, so that issue needs more details about what OrbitView and TileControl use.

@mrlacey
Copy link
Contributor

mrlacey commented Dec 4, 2020

Other than the major categories, I'm curious what everyone thinks of having a separate "Panels" package separate from any "Layout" package, as they seem to distinguish themselves as not having any really visual appearance just a strategy for placing children.

Particularly I think AdaptiveGridView, DockPanel, StaggeredPanel, UniformGrid, WrapPanel would be our "Panels" (or whatever we want to name it). But something like the Carousel or the OribitView already has a strong visual style and in comparison to the "Panels" and is more opinionated/less general purpose.

I would avoid this unless there is a good reason to have them in separate packages. While breaking into more packages makes each package smaller it also adds complexity to finding and selecting the right package.
As a consumer of the packages I wouldn't want to have to stop and work out (guess) whether I needed Controls.Layout or Controls.Panels. This could get particularly confusing if some controls ending Grid are in the Panels package and some another.
And what about DropShadowPanel? That is all about visual appearance.

@LanceMcCarthy
Copy link

I would avoid this unless there is a good reason to have them in separate packages. While breaking into more packages makes each package smaller it also adds complexity to finding and selecting the right package.

I agree with this. UWP has a linker and strips out unused stuff during release compilation with .NETNative. so it's not like having more dependencies is going to bloat your app.

This is more about developer and tooling experience, right? So, where is the balance between having a smaller dependency tree vs needing to search around for the right packages.

@michael-hawker
Copy link
Member

michael-hawker commented Dec 4, 2020

@mrlacey good points. We also already have a Microsoft.Toolkit.Uwp.UI.Controls.Layout package as well that's explicitly related to WinUI ItemsRepeater.

Panels would be a tricky sell, especially with DropShadowPanel which isn't really a Panel...

I was thinking probably we'd have names like Base, Core, or Primitives, but those aren't that descriptive in helping find things. That's partly we still want the uber Controls` package which includes them all still to be there. That way knowing what's in each of these sub-packages is more of an advanced optimization step.

I agree with this. UWP has a linker and strips out unused stuff during release compilation with .NETNative. so it's not like having more dependencies is going to bloat your app.

This is more about developer and tooling experience, right? So, where is the balance between having a smaller dependency tree vs needing to search around for the right packages.

@LanceMcCarthy not all the things for XAML get stripped as the XAML metadata hold implicit references I believe; so those benefits aren't as apparent for the Controls package. @vgromfeld has been investigating this and can explain a bit more the technical details. Ref: #3145

@Rosuavio
Copy link
Contributor Author

Rosuavio commented Dec 4, 2020

@mrlacey the only one I believe I filed so far is #3578 for the Animations package. I thought it was just RangeSelector and ScrollHeader, so that issue needs more details about what OrbitView and TileControl use.

I am putting together a list of their significant dependencies right now, it's clearer than my branches.

@Rosuavio
Copy link
Contributor Author

Rosuavio commented Dec 4, 2020

See #3578 (comment)

@Rosuavio
Copy link
Contributor Author

Rosuavio commented Dec 4, 2020

As "Layout" seems like a compelling category.

I propose that these controls belong in the Layout package.
AdaptiveGridView, DockPanel, StaggeredPanel, UniformGrid, WrapPanel, SwitchPresenter, LayoutTransformControl, HeaderedContentControl, HeaderedItemsControl, Expander, Carousel, MasterDetailsView, OrbitView, BladeView

Ask if these controls belong in the Layout package.
RotatorTile, TileControl, DropShadowPanel, InAppNotification, ScrollHeader, GridSplitter, Menu

And propose that these controls do not belong in the Layout package.
ImageEx, Loading, RadialProgressBar, CameraPreview, Eyedropper, ImageCropper, RadialGauge, RangeSelector, RemoteDevicePicker, TokenizingTextBox, InfiniteCanvas, ColorPicker, TextToolbar

@mrlacey
Copy link
Contributor

mrlacey commented Dec 4, 2020

My thoughts, based on the assumption that Layout refers to "controls that affect or define the position of multiple other controls within the view."

RotatorTile - NOT layout. It presents items rather than positions them.

TileControl - NOT layout - More presentation focused. It only works with an Image and not any content and so is more related to media.

DropShadowPanel - NOT layout - More of a presentation control. Was poorly named. (Can it be renamed?)

InAppNotification - NOT Layout - Because it doesn't have an affect on other items.

ScrollHeader - Could argue either way but I say include it as it works with other controls that clearly are Layout controls. (If I was looking for it in a general controls or layout controls package I'd go for the layout one.)

GridSplitter - Could argue either way but I say include it as it is so closely tied to the Grid and that is a Layout control. (If I was looking for it in a general controls or layout controls package I'd go for the layout one.)

Menu - NOT related to layout. Conceptually it's about Navigation or accessing other content, functionality, or Behavior. It's not how other elements are positioned

@michael-hawker
Copy link
Member

michael-hawker commented Dec 4, 2020

Ideally, we deprecate DropShadowPanel and I build a better extension that works additively to whatever you want it on (or most things), but I haven't gotten to that yet...

It's on my backlog as a follow-on from #3122.

@michael-hawker michael-hawker added this to the 7.0 milestone Dec 8, 2020
@michael-hawker
Copy link
Member

We're close to removing the Animation package dependency, though ScrollHeader is the biggest left.

Plan is to create 4 packages:

  • Microsoft.Toolkit.Uwp.UI.Controls.Primitives for Styleless base controls with no XAML styles.
  • Microsoft.Toolkit.Uwp.UI.Controls."Content" for controls which have a dependency on content or some other control to function. Potential Names: Containers? Composable?
  • Microsoft.Toolkit.Uwp.UI.Controls."Standalone" for the Standalone controls. Potential Names: Core, Base, Independent
  • Microsoft.Toolkit.Uwp.UI.Controls.Media for the Win2D based controls

Thoughts on names?

We may not split the middle-two to start and see how that works...

@mrlacey
Copy link
Contributor

mrlacey commented Dec 15, 2020

Putting multiple things together and calling it standalone has a fun level of irony to it.

@michael-hawker
Copy link
Member

michael-hawker commented Dec 15, 2020

For ScrollHeader we can apply a similar technique #3627 where for the referenced Behaviors we can change from using the Animation Expressions library to the raw composition API calls. This would at least make it so we only have a Behaviors package reference and make it easier to move these to a separate Behaviors package. I think then we actually just get rid of the ScrollHeader as it's just a super lightweight wrapper around the Behaviors themselves, and we show examples of how to apply the behavior directly to the ListView...

Though we still have the Win2D based behaviors in the Animations package to contend with how we want to deal with them... I think what we do is we create a base Behaviors package and then for the Win2D specific behaviors we move them to the Media package for now to live with the rest of the Win2D based effects?? (see open questions below)

In either case, the Animations package purpose is now squarely focused on a pure set of helpers for wrapping and making the composition APIs easier to use in either code or XAML.

Adding a TODO list here for my working branch for these steps:

  • Create Microsoft.Toolkit.Uwp.UI.Behaviors package
  • Move Header Behaviors to Behaviors Package
  • Fix Header Behaviors to not use Animation Expressions
  • Remove ScrollHeader
  • Update Samples to use Behaviors directly for ScrollHeader

Open Questions:

  • Do we move the Animation Effect Behaviors to the Win2D Media package...? (then it'll gain dependencies on Behaviors and maybe animations package...) OR do we create another package which has dependencies on Win2D and the Behaviors?
  • Could we instead have some common interface for 'Effects' in a light-weight sub-package that both the Win2D and Behaviors package could reference? Then we could create a generic Behavior which knows how to apply an animation/effect without the need to have a bunch of special behaviors to just call these effect helpers?
  • It seems most of the these behaviors like Fade, Offset, Rotate, Scale can just be done with the Implicit Animation Helpers...
  • Most of this seems to be interesting for triggering an animation based on some event, which means they should be IAction not a behavior then? (Though IAction still comes from the Behaviors library and would need a dependency unless there's some soft-dependency we could figure out with a .NET trick???)
  • Do we instead need some general attached property helper in the Media package for applying effects at instantiation similar to our implicit animation helpers? E.g.
  <SomeFrameworkElement...>
    <extensions:CompositionEffects>
      <effects:Blur ...>
    </extensions:CompositionEffects>
    <!-- AND/OR ? -->
    <animations:Implicit.ShowAnimations>
       <animations:TranslationAnimation Duration="0:0:1" From="0, -200, 0" To="0" />
       <animations:OpacityAnimation Duration="0:0:1" From="0" To="1.0" />
       <effects:BlurAnimation ..../>
    </animations:Implicit.ShowAnimations>
  </SomeFrameworkElement>

(Or that work directly with the implicit animation helpers to do these effects? I think it'd be OK for the Win2D Media package to reference our newly made light-weight animations package for that...?)

FYI @RosarioPulella @azchohfi @nmetulev for thoughts on this conundrum...

@michael-hawker
Copy link
Member

For now I'm leaving the reference on the Animation package for Behaviors in case we decide to keep that reference as part of my discussion above.

I have the new Animation and Behaviors packages building. I need to fix the Sample for ScrollHeader to show how to use the Behaviors directly, and then I can delete the ScrollHeader control. I also need to investigate a couple of the places we used the Blur behavior in the sample app.

@Rosuavio
Copy link
Contributor Author

In either case, the Animations package purpose is now squarely focused on a pure set of helpers for wrapping and making the composition APIs easier to use in either code or XAML.

That sounds rather nice.

I like where this is going, it seems like it would be a great improvement if we can decouple some of these parts of the API and make them more flexible.

I will start an investigation on composing Effects and Animations in Implicit.ShowAnimations

@michael-hawker
Copy link
Member

Alright, I think we've laid out an initial plan. Now that #3633 is merged, and we know from #3634 that the ScrollHeader will be removed. Independent of our new Animation/Behavior refactor in dev/new-animations, we can break ground on the main Controls package split. The merge between the two branches can occur independently as the only overlap is removing ScrollHeader which is a straight-forward merge.

@RosarioPulella I'll assign this to you to hold the baton on. As mentioned, just remove ScrollHeader and remove the dependencies on Animations in the Control package and you should be fine to base a branch off the mainline for this. We'll continue the animation stuff independently that way.

Let's start with 3 packages (a Styleless Microsoft.Toolkit.Uwp.UI.Controls.Primitives, a Win2D one Microsoft.Toolkit.Uwp.UI.Controls.Media, and a Microsoft.Toolkit.Uwp.UI.Controls.Core for the rest?). Don't forget to add them to the SmokeTest project so that we can get the analytics on their sizes and decide if we want a 4th. 🙂

@michael-hawker
Copy link
Member

michael-hawker commented Jan 22, 2021

@RosarioPulella was thinking of doing a quick survey to get some insight on where we could split 'Standalone' vs. Non-Standalone controls as discussed here. (I've checked and we should be able to cobble something together quick in the Office Forms.)

Thoughts on if we should try and ask about a different splitting line or anything though? Let me know your thoughts.

@michael-hawker
Copy link
Member

michael-hawker commented Feb 1, 2021

@RosarioPulella thought a bit more and decided to take a look at the WinUI categories from their control gallery and try and condense them down:

  • Input + Date & Time + Text + Status and Info (this last one could also fit in the next bucket though there's some overlap)
  • Menus & Toolbars + Dialogs & Flyouts
  • Collections + Layout + Navigation + Scrolling
  • Media (though we already use Media for Win2D)

We kind of use a similar split in our Sample App. Wondering if we could get a cleaner split of taking the rest of our stuff beyond Primitives and Media and segmenting it into 3: Input, Layout, and 'Chrome'?

Not sure what to call the last one. It's almost like Adornments... but that means something else from WPF/XAML land.

I'm also wondering if we just add to the existing Layout package if we plan to end-up with the WinUI dependency in the future anyway...

Let me know any thoughts you have in the morning and we'll sync during the day to figure out a plan for the week.

@Rosuavio
Copy link
Contributor Author

Rosuavio commented Feb 1, 2021

Interesting, so maybe the controls would split into the categories like this?

  • Input - ColorPicker, RadialGauge, RangeSelector, RemoteDevicePicker, TokenizingTextBox
  • Layout -LayoutTransformControl, HeaderedContentControl, HeaderedItemsControl, ListDetailsView, Carousel, OrbitView, BladeView, Expander, GridSplitter
  • "Chrome" - RotatorTile, DropShadowPanel, TileControl, ImageEx, Loading, RadialProgressBar, CameraPreview

What about these other controls?
InAppNotification, Menu, TextToolbar

@michael-hawker
Copy link
Member

Yeah, InAppNotification, Menu, and TextToolbar would be in 'Chrome' as well. Wonder if we just call that the 'Core' package and just pull out the Layout and Input items?

@RosarioPulella thoughts?

@Rosuavio
Copy link
Contributor Author

Rosuavio commented Feb 1, 2021

Well as far as categorization, its always easiest to have a "Un-catogrized" bucket. While Its would be hard to figure out whats in "Core" from the name I think its worth splitting the others controls out. Also if there was no "Other"-ish bucket then it would be a bigger barrier to entry for contributes. If contributors want to add a control and it does not fit in one of the established categories, they would have an easier time just adding it to "Core" than creating a new package. If a new control should be in a different or new package and they try to add to "Core" we can help them from there, rather then them having to deal with that thought process upfront and possibly on there own.

@michael-hawker
Copy link
Member

michael-hawker commented Feb 1, 2021

If a new control should be in a different or new package and they try to add to "Core" we can help them from there, rather then them having to deal with that thought process upfront and possibly on there own.

Yeah, and part of this would be sorted out in an initial discussion/issue before we get to a PR.

I think the last part is if we want to have two different 'Layout' packages or re-use the one we have. My only concern is that the current Layout package is pretty specific and small as there's no XAML bits...

@Rosuavio
Copy link
Contributor Author

Rosuavio commented Feb 1, 2021

Yeah... And between 'Primitives' and the new 'Layout', The controls in 'Primitives' provides no interactions unlike some of the controls in 'Layout'.

@michael-hawker
Copy link
Member

Something to think about... We now have a 'Primitives' package, but we had also put some helper bits in a 'Controls.Primitives' namespace (like the ColorPickerSlider)...

How do we reconcile?

Should our 'Primitives' package be a more aptly named 'Styleless' or do we change the namespace to be something else???

@windows-toolkit/toolkitteam thoughts?

@Rosuavio
Copy link
Contributor Author

Rosuavio commented Feb 4, 2021

Well do we eventually want to put our controls in different namespaces for each package?

ghost pushed a commit that referenced this issue Feb 4, 2021
### Related to #3594, working towards reducing footprint and increasing modularity

<!-- Add a brief overview here of the feature/bug & fix. -->

## PR Type
What kind of change does this PR introduce?
<!-- Please uncomment one or more that apply to this PR. -->

<!-- - Bugfix -->
<!-- - Feature -->
<!-- - Code style update (formatting) -->
- Refactoring (no functional changes, no api changes)
<!-- - Build or CI related changes -->
<!-- - Documentation content changes -->
<!-- - Sample app changes -->
<!-- - Other... Please describe: -->


## What is the current behavior?
<!-- Please describe the current behavior that you are modifying, or link to a relevant issue. -->
The `Guard` and `ThrowHelper` APIs are part of the `Microsoft.Toolkit` package.
This has a few downsides with respect to binary size for consuming package and overall API surface for consumers.

The `Guard` and `ThrowHelper` APIs relied on a number of different NuGet packages that needed to be pulled in:
 - `System.Diagnostics.Contracts`
 - `System.ValueTuple`
 - `System.Memory`
 - `System.Runtime.CompilerServices.Unsafe`

These packages are _only_ needed by those APIs, and yet with `Microsoft.Toolkit` being referenced by every other package in the whole Toolkit, every other package ended up having a dependency on those as well even when there was no use for them at all. This is not ideal both for final binary size, for the increased number of potentially unwanted APIs being imported by consumers, and due to the fact that many consumers want to minimize the number of NuGet dependencies in general.

Furthermore, the `.Diagnostics` APIs can be very useful for all kinds of developers, especially those working in backend scenarios and with a particular focus on performance. It was not ideal to force those developers to also import APIs such as observable collections and other more UI-oriented APIs that were available in the base package (in fact I've spoken with a few devs that didn't like having to include anything other than just the `Guard` and `ThrowHelper` APIs when adding the package).

For consumers on .NET 5 that don't use .NET Native, splitting the `Guard` and `ThrowHelper` APIs away means 80KB less in binary size (when not using IL trimming) when those APIs are not needed, which is effectively a 75% reduction on the base package.

## What is the new behavior?
<!-- Describe how was this issue resolved or changed? -->
The `Guard` and `ThrowHelper` APIs have now been moved to a new `Microsoft.Toolkit.Diagnostics` package.
Splitting this package completely removes all those dependencies from all other packages too 🚀
This work will also make it easier in the future to introduce a `.Diagnostics` source-only package, if we wanted to.

**Note:** marking as breaking change due to the APIs being moved to a different package, but this PR doesn't do any changes to the overall API surface across the various packages - the APIs are still all exactly the same, just modularized differently.

## PR Checklist

Please check if your PR fulfills the following requirements:

- [X] Tested code with current [supported SDKs](../readme.md#supported)
- [ ] ~~Pull Request has been submitted to the documentation repository [instructions](..\contributing.md#docs). Link: <!-- docs PR 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](https://github.com/windows-toolkit/WindowsCommunityToolkit-design-assets)~~
- [X] New major technical changes in the toolkit have or will be added to the [Wiki](https://github.com/windows-toolkit/WindowsCommunityToolkit/wiki) e.g. build changes, source generators, testing infrastructure, sample creation changes, etc...
- [X] Tests for the changes have been added (for bug fixes / features) (if applicable)
- [X] Header has been added to all new source files (run *build/UpdateHeaders.bat*)
- [ ] Contains **NO** breaking changes
@michael-hawker
Copy link
Member

@RosarioPulella they'll all stay in the Controls namespace, as the point of the uber-Controls package is that if you're just getting started you don't know that they're all in sub-packages. The sub-packages are really just a later step for app developers to optimize package sizes later.

I'm wondering if we just move the ColorPickerSlider to the main namespace but add the Browsable/EditorBrowsable component attributes to hide it from the editor?

@ghost ghost added the In-PR 🚀 label Feb 11, 2021
@ghost ghost added Completed 🔥 and removed In-PR 🚀 labels Feb 12, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Apr 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Completed 🔥 feature request 📬 A request for new changes to improve functionality
Projects
None yet
4 participants