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

TemplatePart XAML diagnostics #14180

Merged
merged 6 commits into from
Feb 24, 2024
Merged

TemplatePart XAML diagnostics #14180

merged 6 commits into from
Feb 24, 2024

Conversation

maxkatz6
Copy link
Member

What does the pull request do?

  1. Adds IsRequired property to the TemplatePart. This property should be set on these parts that control can't work without. For example, PART_ContentPresenter is optional for Button - it will work without. But PART_TextPresenter is necessary for TextBox.
  2. Makes TemplatePart inheritable. This one is a bit questionable, as WPF attribute wasn't inheritable on paper. As on practice WPF doesn't duplicate TemplateParts per each inherited type - TextBoxBase has TemplatePart, while TextBox doesn't. Also, having it inherited makes compiler job much easier.
  3. As a result of previous one - custom control can redefine Type or IsRequired parent TemplatePart. We don't have examples of this in the repo though.
  4. Finally, this PR adds three diagnostics:
  • RequiredTemplatePartMissing = AVLN2205 (enabled as error by default)
  • OptionalTemplatePartMissing = AVLN2206 (disabled by default)
  • TemplatePartWrongType = AVLN2207 (enabled as error by default)
  1. Fixes all AVLN2205 AVLN2207 and some AVLN2206 in the repository.
  2. In general I tried to not affect existing behavior much. But some controls have changed their template part requirements. CaptionButtons, for example, now doesn't crash when any button is missing.

Checklist

Breaking changes

New diagnostic errors might break projects. New errors should be either fixed or disabled with csproj/editorconfig settings.

Part of #13707

cc @robloo @danwalmsley

@maxkatz6
Copy link
Member Author

Having it implemented in XAML compiler makes old Rider YouTrack issue unnecessary. Also, works in VS.

@avaloniaui-bot
Copy link

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

@robloo
Copy link
Contributor

robloo commented Jan 13, 2024

A lot of good ideas here! It's also good someone else spent the time going through each TemplatePart. There were so many it's always easy to miss them.

Adds IsRequired property to the TemplatePart. This property should be set on these parts that control can't work without. For example, PART_ContentPresenter is optional for Button - it will work without. But PART_TextPresenter is necessary for TextBox.

This is a great idea!

Makes TemplatePart inheritable. This one is a bit questionable, as WPF attribute wasn't inheritable on paper. As on practice WPF doesn't duplicate TemplateParts per each inherited type - TextBoxBase has TemplatePart, while TextBox doesn't. Also, having it inherited makes compiler job much easier.

I just followed WPF originally for this. So I have no strong feelings about it and generally think it is a good change. It matches the inheritance of other things related to controls: properties, etc.

  1. However, it is conceivable for a derived control (parent) to completely replace the logic of the child. This means a derived control may not actually require all the template parts of the children. It may have completely different code.
  2. We have to be careful to check all template part attributes to see if there was any duplication... I suspect you already did this?

As a result of previous one - custom control can redefine Type or IsRequired parent TemplatePart. We don't have examples of this in the repo though.

Yea, since the parent can completely override the child's template part attributes there is less concern. Only in the case of a parent derived control not actually using a child template part would there be concern.

Finally, this PR adds three diagnostics:

Awesome! This one was sorely needed and you found a good way to do it.


Separately, I thought we need to review all e.NameScope.Find<T>("name") and e.NameScope.Get<T>("name") usages. These really should follow the IsRequired property of the attribute. Then I thought they should be automated somehow -- as another analyzer -- this could be done using the Name property on the template part itself. I've never written an analyzer so this just seems feasible to me.

That also logically makes more sense than defining constants all over the place and it really needs to be cleanup up sometime. I wonder if this would solve #9093 somehow too. This needs more thought but likely the only way to really make it useful is with a code generator that would automatically generate constant names for the template part attributes. Nevermind, I forgot we already talked about most of this so I was just re-discovering old ideas.

@thevortexcloud
Copy link
Contributor

thevortexcloud commented Jan 15, 2024

Thanks for adding this! I was literally asking for it just last week.

Is there any plans for a reverse of this too, where a template has parts defined that don't match an attribute?

@avaloniaui-bot
Copy link

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

@maxkatz6
Copy link
Member Author

We have to be careful to check all template part attributes to see if there was any duplication... I suspect you already did this?

I did, but there always a chance to miss something.

@maxkatz6
Copy link
Member Author

Updated PR after review. It is ready again.

@maxkatz6 maxkatz6 requested a review from a team February 11, 2024 06:47
@avaloniaui-bot
Copy link

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

@kekekeks kekekeks added this pull request to the merge queue Feb 24, 2024
Merged via the queue into master with commit 180410e Feb 24, 2024
6 checks passed
@kekekeks kekekeks deleted the template-part-errors branch February 24, 2024 07:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants