[Feature] Split up the controls package.

Rosuavio commented 3 years ago

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"

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.
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

Keep the package as it is.
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

ghost commented 3 years ago

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 commented 3 years ago

I should mention my personal thoughts are that

We should group them based on domains.
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 commented 3 years ago

@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 commented 3 years ago

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

Rosuavio commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

michael-hawker commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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, orPrimitives, but those aren't that descriptive in helping find things. That's partly we still want the uberControls` 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 commented 3 years ago

@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 commented 3 years ago

See https://github.com/windows-toolkit/WindowsCommunityToolkit/issues/3578#issuecomment-738944273

Rosuavio commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

michael-hawker commented 3 years ago

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:

[x] Create Microsoft.Toolkit.Uwp.UI.Behaviors package
[x] Move Header Behaviors to Behaviors Package
[x] Fix Header Behaviors to not use Animation Expressions
[x] Remove ScrollHeader
[x] 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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

michael-hawker commented 3 years ago

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 commented 3 years ago

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

michael-hawker commented 3 years ago

@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?

CommunityToolkit / WindowsCommunityToolkit