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

Centralize logic for handling logical children #14132

Merged
merged 5 commits into from
Apr 26, 2023

Conversation

PureWeen
Copy link
Member

@PureWeen PureWeen commented Mar 22, 2023

Description of Change

A lot of our logical children code was just being copy and pasted between different controls. This PR standardizes Adding/Removing logical children inside Element. This way, whenever we add/remove a logical control we always make sure the same set of operations occur and that the VisualDiagnostics code also gets triggered.

This PR doesn't apply these changes to the Page elements yet because I felt like that would have stuffed too much logic into one PR.

Once/if this PR is merged then I will do a follow up PR against Page and all the derived page types. This will help us to avoid scenarios where things aren't showing up in the VisualTree, plus, once we get Page straightened out, users will be able to add logical children to page, which has come up on the community toolkit CommunityToolkit/Maui#620.

The end goal will be to also remove the ChildrenNotDrawnByThisElement and AllChildren properties on Element as well. Which were only really hacks to workaround how LogicalChildren are handled inside Page

Implementation

  • The idea is that any subclasses should only call AddLogicalChildren/RemoveLogicalChildren to interact with their logical children. They shouldn't need to provide their own storage container for their own logical children. That's just handled/managed by Element now
  • I did add an override for LogicalChildrenInternalBackingStore, because there are a few cases (Page/Layout) where it's going to take a little more effort to make that code work by only calling AddLogicalChildren/RemoveLogicalChildren. Page also makes a lot of assumptions about its LogicalChildren, for example, it automatically calls LayoutChildInBounds on every single LogicalChild.

@MartyIX
Copy link
Collaborator

MartyIX commented Mar 22, 2023

Why are the children "logical"? Is there a simple explanation?

@PureWeen PureWeen force-pushed the organize_logical_children branch 2 times, most recently from 93e2526 to fea1488 Compare April 5, 2023 19:26
@github-actions
Copy link
Contributor

github-actions bot commented Apr 5, 2023

Thank you for your pull request. We are auto-formatting your source code to follow our code guidelines.

1 similar comment
@github-actions
Copy link
Contributor

github-actions bot commented Apr 5, 2023

Thank you for your pull request. We are auto-formatting your source code to follow our code guidelines.

src/Controls/src/Core/Element.cs Outdated Show resolved Hide resolved
Comment on lines 123 to 228
/// <include file="../../docs/Microsoft.Maui.Controls/Element.xml" path="//Member[@MemberName='LogicalChildren']/Docs/*" />
[EditorBrowsable(EditorBrowsableState.Never)]
[Obsolete("Do not use! This is to be removed! Just used by Hot Reload! To be replaced with IVisualTreeElement!")]
public ReadOnlyCollection<Element> LogicalChildren =>
new ReadOnlyCollection<Element>(new TemporaryWrapper(LogicalChildrenInternal));
IReadOnlyList<Element> IElementController.LogicalChildren => LogicalChildrenInternal;
Copy link
Member

Choose a reason for hiding this comment

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

Wow, gone from DO NOT USE to a public feature. Is this intentional and why was it marked so strongly before?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea, I don't fully understand the strong language. IVT really just exists because of where LogicalChildren was setup wrong. Any case where the LogicalChildren and the IVT diverge seems like a defect of the control. For example, ListView, for some reason, completely breaks if you start mapping the LogicalChildren to realized cells (basically what we do for CV).

I think we could get rid of all these hacks

protected virtual void OnBindingContextChanged()
{
BindingContextChanged?.Invoke(this, EventArgs.Empty);
if (Shell.GetBackButtonBehavior(this) is BackButtonBehavior buttonBehavior)
SetInheritedBindingContext(buttonBehavior, BindingContext);
if (Shell.GetSearchHandler(this) is SearchHandler searchHandler)
SetInheritedBindingContext(searchHandler, BindingContext);
if (Shell.GetTitleView(this) is View titleView)
SetInheritedBindingContext(titleView, BindingContext);
if (FlyoutBase.GetContextFlyout(this) is BindableObject contextFlyout)
SetInheritedBindingContext(contextFlyout, BindingContext);
}
if the LogicalChildren were accurate.

Is there any scenario where it makes sense for LogicalChildren != IVT.GetChildren?

I could rename LogicalChildren to VisualChildren and then I guess it's more in line with the language we're using for IVT.

Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIK, the original reason why LogicalChildren was discouraged was to avoid people using it to access underlying elements and build UI Structures around it rather than using Binding and acting on elements organically. For example, you wouldn't want someone to try to poke at individual elements in a ListView or CollectionView via LogicalChildren, as those items come and go as needed, and trying to pin something to one could create problems if you don't know what you're doing.

IMO "VisualChildren" is a misnomer. I believe @PureWeen brought that up before with Shell: Shell is virtual controls made up of visual elements that are not, by themselves, "Visual." But they still need to be represented in the LVT as that is the proper makeup of your application, From what the Window has on down. Then again, IMO, as long as the information is correct, I don't care what the name is, lol.

src/Controls/src/Core/Items/ItemsView.cs Outdated Show resolved Hide resolved
src/Controls/src/Core/Element.cs Show resolved Hide resolved
@PureWeen PureWeen force-pushed the organize_logical_children branch 2 times, most recently from deb0fdd to 05dc620 Compare April 7, 2023 02:20
@PureWeen PureWeen marked this pull request as ready for review April 7, 2023 08:10
src/Controls/src/Core/Element.cs Outdated Show resolved Hide resolved
src/Controls/src/Core/Element.cs Outdated Show resolved Hide resolved
@github-actions
Copy link
Contributor

Thank you for your pull request. We are auto-formatting your source code to follow our code guidelines.

@mattleibow mattleibow merged commit 519f27c into main Apr 26, 2023
28 checks passed
@mattleibow mattleibow deleted the organize_logical_children branch April 26, 2023 18:05
@samhouts samhouts added area-controls-collectionview CollectionView, CarouselView, IndicatorView area-controls-flyoutpage FlyoutPage area-layout StackLayout, GridLayout, ContentView, AbsoluteLayout, FlexLayout, ContentPresenter area-controls-shell Shell Navigation, Routes, Tabs, Flyout area-controls-border Border area-controls-flyout Flyout area-controls-menubar Desktop MenuBarItems area-controls-window Window labels Jul 11, 2023
@ghost ghost added the legacy-area-controls Label, Button, CheckBox, Slider, Stepper, Switch, Picker, Entry, Editor label Jul 11, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Dec 12, 2023
@Eilon Eilon removed the legacy-area-controls Label, Button, CheckBox, Slider, Stepper, Switch, Picker, Entry, Editor label May 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-controls-border Border area-controls-collectionview CollectionView, CarouselView, IndicatorView area-controls-flyout Flyout area-controls-flyoutpage FlyoutPage area-controls-menubar Desktop MenuBarItems area-controls-shell Shell Navigation, Routes, Tabs, Flyout area-controls-window Window area-layout StackLayout, GridLayout, ContentView, AbsoluteLayout, FlexLayout, ContentPresenter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants