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

Warning AVLN2208: Item container in the data template #18132

Merged
merged 4 commits into from
Feb 7, 2025

Conversation

maxkatz6
Copy link
Member

@maxkatz6 maxkatz6 commented Feb 6, 2025

What does the pull request do?

It's a fairly common mistake to put item container inside of data template.
Typical example can be found even in this repository:

<ListBox Name="EventsList" ItemsSource="{Binding RecordedEvents}"
SelectedItem="{Binding SelectedEvent, Mode=TwoWay}">
<ListBox.ItemTemplate>
<DataTemplate>
<ListBoxItem Classes.handled="{Binding IsHandled}">
<Grid ColumnDefinitions="Auto,Auto,Auto,*,Auto">

The problem with this code is that DataTemplate does not affect how container itself is defined. Instead, it will create a dumb ListBoxItem inside of the proper ListBoxItem. Making it worse, styles are active for both ListBoxItems, but only outer one can be actually selected.

Example of how this code looks in the elements tree:
image

This problem is not limited by ListBoxItem, but by any items control. I often see this mistake with MenuItem and TabControl.

What is the updated/expected behavior with this PR?

New XAML analyzer is added to XamlX compiler.
Code: "AVLN2208" (can be configured as an error via project settings or editorconfig, or disabled).
Message example (actual text depends on the context):

Unexpected 'TabItem' inside of 'TabControl.DataTemplates'. 'TabControl.DataTemplates' defines template of the container content, not the container itself.

Checklist

Breaking changes

By default, this is a warning. No user code will have these errors, unless these projects have all warnings enabled as errors - but these projects typically expect new errors to be found early.

Fixed issues

Contributes towards #13707

@avaloniaui-bot
Copy link

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

@robloo
Copy link
Contributor

robloo commented Feb 7, 2025

I seem to recall UWP/WinUI has code to detect when a container is provided as the root of the data template (not sure if WPF does). In that case, no extra container is generated.

There are real-world use cases where is is much easier having direct access to the container by including it in your data template XAML as in your example above.

Bottom line, this seems to be going the wrong direction. We should actually detect and support this case... not throw a warning.

@maxkatz6
Copy link
Member Author

maxkatz6 commented Feb 7, 2025

Bottom line, this seems to be going the wrong direction. We should actually detect and support this case... not throw a warning.

It's not about a direction here, but about current state today.
Today containers in data template are not integrated into items control directly.
And this warning is necessary to make finding relevant bugs easier.

If one day we support container inside data templates, this warning can be removed. But nobody requested this feature yet as far as I can remember.

@robloo
Copy link
Contributor

robloo commented Feb 7, 2025

OK, fair enough.

But nobody requested this feature yet as far as I can remember.

I noticed this when I ported code over a while ago and worked around it. During a port it's usually better to just find a work-around and move to the next thing. Still, I should have brought it up before now. Consider this the first unofficial request.

return node;
}

// Avalonia doesn't have any reliable way to determine container type from the API.
Copy link
Member

Choose a reason for hiding this comment

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

Should we add an attribute for this at some point? It could be useful. (Not as part of this PR of course.)

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking about it.
And there was a PR for that #13104
The problem is that such attribute won't be followed by third party controls, and it gives a little of benefit otherwise, for built-in controls.

Let's add it to next api review, I suppose. If we will find a better idea.

Copy link
Member

@MrJul MrJul left a comment

Choose a reason for hiding this comment

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

LGTM!

@MrJul MrJul added this pull request to the merge queue Feb 7, 2025
Merged via the queue into master with commit 68009f8 Feb 7, 2025
12 checks passed
@MrJul MrJul deleted the container-in-data-template-warning branch February 7, 2025 18:30
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.

4 participants