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

Add TitleView to VisualChildren list for Shell #14006

Merged
merged 10 commits into from
Mar 30, 2023

Conversation

drasticactions
Copy link
Contributor

@drasticactions drasticactions commented Mar 17, 2023

Fixes #13992

This adds TitleView into the Visual Tree list, allowing it to be hot reloaded. For this to work, I added IVisualTreeElement to Page and Shell, then overrode the list to include the TitleView, which is an View and automatically has its own children.

2023-03-17.17.56.10.mov

newView.Parent = owner;
VisualDiagnostics.OnChildAdded((Page)bindable, newView);
Copy link
Member

Choose a reason for hiding this comment

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

Can we just call AddLogicalChild here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add/RemoveLogicalChild are on Shell (

public void RemoveLogicalChild(Element element)
) but Bindable could be a ContentPage (At least going by the unit tests that failed when I did cast to Shell (fc2775f)).

Seeing that this is in Shell, I could check if Bindable is Shell, and if so, call Add/RemoveLogicalChild, but if it's anything else it would be missed. Is that okay?

Copy link
Member

Choose a reason for hiding this comment

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

I think so :-)

There's two places we need to account for the TitleView being added

  1. On the ContentPage
  2. On the Shell

If it's added on the ContentPage then we want the titleView to get added to the LogicalChildren of the PAge not shell. If it's added to Shell we want it to get added to the LogicalChildren of Shell not Page

@@ -1316,10 +1326,16 @@ static void OnTitleViewChanged(BindableObject bindable, object oldValue, object
var newView = (View)newValue;

if (oldView != null)
{
VisualDiagnostics.OnChildRemoved((Page)bindable, oldView, ((IVisualTreeElement)bindable).GetVisualChildren().IndexOf(oldView));
Copy link
Member

Choose a reason for hiding this comment

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

Can we just call RemoveLogicalChild here?

@Eilon Eilon added the area-controls-shell Shell Navigation, Routes, Tabs, Flyout label Mar 22, 2023
@drasticactions
Copy link
Contributor Author

I switched Shell to Add/RemoveLogicalChild and that seems to work fine. Page is proving a bit tougher. It has "LogicalChildren", "InternalLogicalChildren" and "InternalChildren". InternalChildren is public, but if I try and add TitleView to it, it seems to get stomped later on by TemplateUtilities.OnContentChanged, where it gets removed and replaced by the actual page content.

I think it's possible to refactor all of this down so that "Add/RemoveLogicalChild" is put into "Page" and out of Shell, so all of them can use it, but it'll take a bit more time.

@drasticactions
Copy link
Contributor Author

@PureWeen I removed the Page code for now and I want to spin that into another PR. I think it will have more discussion around it, and as you're already doing stuff with InternalChildren (#14132) we could talk about the views that should appear inside InternalChildren and the visual tree.

The Shell code is independent of it and switching it to use Remove/AddLogicalChild should have that work with whatever new thing is created, so that feels safe to add it as is right now.

@drasticactions drasticactions requested a review from PureWeen March 28, 2023 23:20
Copy link
Member

@PureWeen PureWeen left a comment

Choose a reason for hiding this comment

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

Looks good! Can we add some tests for this inside Controls.Core.UnitTests?

@drasticactions
Copy link
Contributor Author

Looks good! Can we add some tests for this inside Controls.Core.UnitTests?

Added with 658d298, is this okay?

@drasticactions drasticactions changed the title Add TitleView to VisualChildren list for Page/Shell Add TitleView to VisualChildren list for Shell Mar 30, 2023
@PureWeen PureWeen enabled auto-merge (squash) March 30, 2023 15:06
@PureWeen PureWeen merged commit 12aa0ee into dotnet:main Mar 30, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Dec 12, 2023
@samhouts samhouts added the fixed-in-8.0.0-preview.3.8149 Look for this fix in 8.0.0-preview.3.8149! label Aug 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-controls-shell Shell Navigation, Routes, Tabs, Flyout fixed-in-8.0.0-preview.3.8149 Look for this fix in 8.0.0-preview.3.8149!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

XAML Hot Reload is broken within Shell.TitleView
4 participants