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 default template to the ContentControl, remove theme specific templates #11365

Merged
merged 7 commits into from
Nov 7, 2023

Conversation

maxkatz6
Copy link
Member

What does the pull request do?

Created this PR as I wanted to do something different in my free time, but what might improve some existing problems.
CC @grokys as I want to hear your opinion about these changes.

There were couple of different discussions before:

  1. There is a good opinion that Avalonia app should provide at least the bare minimum templates without any additional themes. We don't want to duplicate themes/templates in the C# code as "yet simpler version of simple theme", but we can at least move some common templates from XAML to Avalonia core package.
  2. There is an annoying (at least for me) issue with UserControl that forced as to use StyleInclude here. It makes resources dictionary merging less efficient. And when add transformer to eliminate StaticResource where it's possible (by referencing resource reference directly if it's in the same dictionary), this Styles+StyleInclude would make it impossible to work efficiently.

Now, adding a default value to the ContentControl.Template solves quite a lot of that:

  • ContentControl obviously will have everything it needs to render without a theme.
  • UserControl too, but also we can remove this Styles+StyleInclude combination, simplifying our default themes (and third party ones too).
  • More complex control like Window, TopLevel, Button... now also have a bare minimum template too (since they inherit ContentControl). Instead of showing transparent window. But still should be restyled to get full functionality.
  • Slightly minimal perf improvement as well I suppose, don't even want to measure if.
  • Headless tests without any theme attached now make sense. At least minimal test with Window and Border can work now, as Window will have a template to host a content.

With all of this, ControlCatalog (with some unrelated modifications) can run without fluent nor simple theme:
image

image

Can it be improved futher?

Technically, we can do the same for ItemsControl (which will cover ListBox as well), so it doesn't need to be restyled by every third-party theme.
ScrollViewer with only ScrollViewerPresenter inside, no bars, also can be done in this way, but it 100% will be reimplemented by every third-party theme, so not that many reasons. Possibly only headless.
Other controls are too complicated for that, and also will be restyled in the end anyway.

Fixed issues

Fixes #4614

@maxkatz6 maxkatz6 requested a review from grokys May 14, 2023 06:52
@TomEdwardsEnscape
Copy link
Contributor

I like this change. We are re-theming everything in our application and #4614 was a roadblock that we hit very early on. PopupRoot has the same problem I believe, which left us scratching our heads as tooltips never appeared, even though we had a theme for ToolTip.

We can't even identify all the special templates that we need, and over time new ones could easily be introduced.

The solution we ended up with was including the simple theme before our own theme, so that we can be certain that we have a template for every obscure low-level control. It would be great if we could avoid doing this.

@maxkatz6
Copy link
Member Author

@tomenscape popuproot should minimally work, as it's content control as well.

Saying that, tests didn't like this change at all.

@grokys
Copy link
Member

grokys commented May 15, 2023

I've been thinking about this, here are my thoughts so far:

Cons

There is a good opinion that Avalonia app should provide at least the bare minimum templates without any additional themes

I'm not sure that's the conclusion I come to when reading that thread: that issue seems more to be asking for any indication of why a control isn't displaying due to a missing template. At the beginning of the project for example, I considered draing a red cross for controls without a template (never did implement this, probably for the best ;) ).

We don't want to duplicate themes/templates in the C# code as "yet simpler version of simple theme", but we can at least move some common templates from XAML to Avalonia core package.

Yeah, the question would be: how far do we go? Because the logical conclusion of this is that we always ship something like the default theme as default templates.

Pros

  • we can remove this Styles+StyleInclude combination, simplifying our default themes (and third party ones too).

  • Headless tests without any theme attached now make sense.

  • Basically everything you listed!

Conclusion

I'm still not completely decided on this mainly because of the question "How far do we take this?". But, given that ContentControl (and derived controls) are such a fundamental building block, and given that 99% of the time the ContentControl template will be exactly the same, regardless of the theme being used, I'm tempted to say that this makes sense in the case, and only in the case of ContentControl.

@Gillibald
Copy link
Contributor

Our control unit tests are full of control template factories that create bare minimum templates that only contain parts that are mandatory for the control being used. So having such factory as part of the actual control would help to speed up testing and makes it easier to tests controls that rely on specific parts in their control template.

TextBox for example will need a ScrollViewer and a TextPresenter but the ScrollViewer itself isn't functional without a template. So I end up writing a template factory for ScrollViewer and for TextBox just to be able to test. All this requires internal knowledge of the controls being used.

@maxkatz6 maxkatz6 force-pushed the content-control-template-as-default-value branch from e408721 to 40aaf82 Compare June 2, 2023 15:26
@maxkatz6 maxkatz6 enabled auto-merge June 2, 2023 16:10
auto-merge was automatically disabled July 5, 2023 19:33

Merge queue setting changed

@maxkatz6 maxkatz6 force-pushed the content-control-template-as-default-value branch from 1089591 to cf3ca88 Compare August 30, 2023 05:21
@@ -430,7 +430,8 @@ public void FlowDirection_Of_RectangleContent_Updated_After_OpenPopup()
{
new ComboBoxItem()
{
Content = parentContent.Child
Content = parentContent.Child,
Template = null // ugly hack, so we can "attach" same child to the two different trees
Copy link
Member Author

Choose a reason for hiding this comment

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

This hack replicates old test behavior, when there was no template on the ComboBoxItem. Now there is one always.

@maxkatz6 maxkatz6 requested a review from grokys August 30, 2023 05:22
@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 11.0.999-cibuild0039008-beta. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@maxkatz6 maxkatz6 requested review from a team and removed request for grokys October 16, 2023 22:34
@MrJul
Copy link
Member

MrJul commented Nov 6, 2023

I had a look at the code and it's looking good to me.

What's the final consensus about merging this?

@maxkatz6
Copy link
Member Author

maxkatz6 commented Nov 7, 2023

@MrJul after discussion we agreed on removing ItemsControl template and not adding more than ContentControl template. So, this PR should be mergeable now.

@maxkatz6 maxkatz6 enabled auto-merge November 7, 2023 09:27
@maxkatz6 maxkatz6 added this pull request to the merge queue Nov 7, 2023
Merged via the queue into master with commit 60c1116 Nov 7, 2023
@maxkatz6 maxkatz6 deleted the content-control-template-as-default-value branch November 7, 2023 15:43
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.

Discussion : Application.Styles required or Window will not Render Content
6 participants