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

Ensure templated parent control theme is applied #10273

Merged
merged 4 commits into from
Feb 21, 2023

Conversation

jp2masa
Copy link
Contributor

@jp2masa jp2masa commented Feb 8, 2023

What does the pull request do?

Ensure templated parent control theme is applied.

What is the current behavior?

When templated parent control theme changes, styles are not applied, this affects Expander on Simple Theme (#10226).

What is the updated/expected behavior with this PR?

Styles are correctly applied.

How was the solution implemented (if it's not obvious)?

Override OnTemplatedParentControlThemeChanged on Layoutable and call InvalidateMeasure, like it's already being done for OnControlThemeChanged. Add missing !_templatedParentThemeApplied to ApplyStyling.

Checklist

Fixed issues

Fixes #10226

@jp2masa
Copy link
Contributor Author

jp2masa commented Feb 8, 2023

While fixing this I also found a possible bug, related to the following overrides:

private protected override void OnControlThemeChanged()
{
base.OnControlThemeChanged();
var count = VisualChildren.Count;
for (var i = 0; i < count; ++i)
{
if (VisualChildren[i] is StyledElement child &&
child.TemplatedParent == this)
{
child.OnTemplatedParentControlThemeChanged();
}
}
}
internal override void OnTemplatedParentControlThemeChanged()
{
base.OnTemplatedParentControlThemeChanged();
var count = VisualChildren.Count;
var templatedParent = TemplatedParent;
for (var i = 0; i < count; ++i)
{
if (VisualChildren[i] is TemplatedControl child &&
child.TemplatedParent == templatedParent)
{
child.OnTemplatedParentControlThemeChanged();
}
}
}

It makes sense to override OnControlThemeChanged on TemplatedControl to call OnTemplatedParentControlThemeChanged on its visual children, i.e. the template.

However, for OnTemplatedParentControlThemeChanged what I understood is that it's propagating OnTemplatedParentControlThemeChanged to all children inside the template, but to be part of the template they don't have to be TemplatedControls, right? The minimum requirement is for them to be Visuals. Am I missing something?

@jp2masa jp2masa marked this pull request as ready for review February 8, 2023 21:38
@maxkatz6
Copy link
Member

maxkatz6 commented Feb 8, 2023

@jp2masa is it possible to add some tests, so it won't be regressed?

@maxkatz6 maxkatz6 requested a review from grokys February 8, 2023 23:44
@jp2masa jp2masa force-pushed the issue-10226 branch 2 times, most recently from b8a987e to 04df472 Compare February 9, 2023 03:15
@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 11.0.999-cibuild0029785-beta. (feed url: https://pkgs.dev.azure.com/AvaloniaUI/AvaloniaUI/_packaging/avalonia-all/nuget/v3/index.json) [PRBUILDID]

Copy link
Member

@grokys grokys left a comment

Choose a reason for hiding this comment

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

Sorry, this dropped off my radar - LGTM, thanks @jp2masa !

@grokys
Copy link
Member

grokys commented Feb 21, 2023

However, for OnTemplatedParentControlThemeChanged what I understood is that it's propagating OnTemplatedParentControlThemeChanged to all children inside the template, but to be part of the template they don't have to be TemplatedControls, right? The minimum requirement is for them to be Visuals. Am I missing something?

Yes, that's correct I think. I also notice we have no test coverage covering the for loop in OnTemplatedParentControlThemeChanged so we should fix that.

@grokys grokys merged commit b8b6324 into AvaloniaUI:master Feb 21, 2023
@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 11.0.999-cibuild0030363-beta. (feed url: https://pkgs.dev.azure.com/AvaloniaUI/AvaloniaUI/_packaging/avalonia-all/nuget/v3/index.json) [PRBUILDID]

@jp2masa jp2masa deleted the issue-10226 branch February 25, 2023 02:40
@jp2masa
Copy link
Contributor Author

jp2masa commented Feb 25, 2023

However, for OnTemplatedParentControlThemeChanged what I understood is that it's propagating OnTemplatedParentControlThemeChanged to all children inside the template, but to be part of the template they don't have to be TemplatedControls, right? The minimum requirement is for them to be Visuals. Am I missing something?

Yes, that's correct I think. I also notice we have no test coverage covering the for loop in OnTemplatedParentControlThemeChanged so we should fix that.

Opened #10471 with a test and fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expander styles broken when using Simple Theme
4 participants