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

Update/Standardize Template Part Names #8793

Merged
merged 11 commits into from
Sep 8, 2022

Conversation

robloo
Copy link
Contributor

@robloo robloo commented Aug 21, 2022

What does the pull request do?

Updates all template parts (control template elements used in code-behind) to have a standardized PART_ prefix. This requires changes to the following controls:

  1. DatePicker
  2. DatePickerPresenter
  3. TimePicker
  4. TimePickerPresenter
  5. ToggleSwitch
  6. ManagedFileChooser
  7. Calendar
  8. CalendarItem
  9. DataGridRowGroupHeader

This PR also includes the changes discussed here: #3538 (comment)

The vast majority of template parts were already following the PART_ prefix convention as was done in WPF. However, most of the ones missing it were those ported from UWP/WinUI.

It is important to note that the convention for this changed between WPF and WinUI. WPF used the PART_ prefix while WinUI does not. This change in WinUI may have been done due to the way control templates changed (heavy usage of VisualStateManager instead of triggers). In UWP/WinUI it would be strange to have some elements in the VisualStateManager with the prefix and others without. Therefore, to improve consistency, WinUI likely got rid of it.

Avalonia, however, can follow the WPF convection because it does not use a VisualStateManager concept for styling like WinUI does. Those controls that have a PART_ prefix are those controls that are directly used in C# code-behind. Other controls in the template not used in code-behind may follow standard XAML naming conventions.

What is the current behavior?

Not all have the PART_ prefix.

What is the updated/expected behavior with this PR?

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

Find and renamed in:

  • Control.cs
  • FluentTheme.xaml
  • SimpleTheme.xaml

Checklist

Breaking changes

Yes, this is a breaking change to control templates.

Obsoletions / Deprecations

None

Fixed issues

Fixes #8057

@robloo
Copy link
Contributor Author

robloo commented Aug 21, 2022

@amwx I know you are generally against the PART_ prefix. I figure this is a good place to discuss it once and for all before we are locked in for years to come. If you have any feedback here to share with everyone for your viewpoint please let use know. I think a final discussion would be useful at least for future reference.

@avaloniaui-team
Copy link
Contributor

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

@robloo
Copy link
Contributor Author

robloo commented Aug 21, 2022

@CollinAlpert This includes fixes for your comment #3538 (comment). Please look this over when you have a chance and let me know if you found anything else.

@rabbitism
Copy link
Contributor

Actually, I am thinking about standardizing those part names, too. And there are some other issues I think can be standardized.

  1. Some PART_names are assigned to x:Name and some are assigned to Name.
  2. If we switch to Name, and make the PART_names public const members, we can refer to them with x:Static in control template.

@robloo
Copy link
Contributor Author

robloo commented Aug 21, 2022

@rabbitism Your second point is very interesting. It would allow us to remove magic strings entirely and share a single variable for each template part name.

It also cleans up and standardizes the convention between controls. Some like DataGrid already have variables to store the names (which are used only in C# code-behind). Others define the same string multiple times. Removing duplication and making the variables follow a standard naming convention would be very good.

@maxkatz6 Would there be any performance or other concerns doing this? I'm not familiar enough with how the XAML compiler would handle this case.

@rabbitism
Copy link
Contributor

rabbitism commented Aug 21, 2022

@rabbitism Your second point is very interesting. It would allow us to remove magic strings entirely and share a single variable for each template part name.

It also cleans up and standardizes the convention between controls. Some like DataGrid already have variables to store the names (which are used only in C# code-behind). Others define the same string multiple times. Removing duplication and making the variables follow a standard naming convention would be very good.

@maxkatz6 Would there be any performance or other concerns doing this? I'm not familiar enough with how the XAML compiler would handle this case.

Not perfect solution, because we still need magic string in style selector, but at least we can align the control code with style file.
----Edit---
In general, it'll be nice to support static member reference in Selector Parser, it'll benefit both part names and pseudo class names.

@robloo
Copy link
Contributor Author

robloo commented Aug 21, 2022

It's also worth noting that using x:Static to name template parts in XAML would also clearly define what IS and IS NOT a template part. This could allow removing the PART_ prefix itself. Something interesting to think about.

Overall though I might write this up as another issue or discussion. It can be done in another PR. (Removing the PART_ prefix is a lot easier than adding it).

@timunie
Copy link
Contributor

timunie commented Aug 22, 2022

I somewhat like the PART_ prefix tbh. I vote for keeping it.

@maxkatz6
Copy link
Member

@maxkatz6 Would there be any performance or other concerns doing this? I'm not familiar enough with how the XAML compiler would handle this case.

Perf-wise it's like replacing inline const string with call to a const property. Not much a difference.
But bigger inconvenience is with selectors, as you can't use markup extensions there.

@avaloniaui-team
Copy link
Contributor

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

Grid.Row="1"
Width="20"
Height="20"
HorizontalAlignment="Left">

<Grid x:Name="MovingKnobs" Width="20" Height="20">
<Grid x:Name="PART_MovingKnobs" Width="20" Height="20">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

WinUI does not have two template parts in the ToggleSwitch like this (both "PART_SwitchKnob" and "PART_MovingKnobs"). Instead, there is only the one "SwitchKnob"

This may need to be simplified in Avalonia to match WinUI.

https://github.com/microsoft/microsoft-ui-xaml/blob/ea98f11e5748cdcdc22b153e175eef50adc7ad8c/dev/CommonStyles/ToggleSwitch_themeresources_v1.xaml#L546-L563

@robloo
Copy link
Contributor Author

robloo commented Aug 24, 2022

I'm going to open up another issue for the expanded discussion about x:Static and template part name constants. It might be until the weekend before I get to that.

Additionally, I noticed the template part names themselves are not consistent. While this PR ensures the "PART_" prefix is uniform, the names themselves sometime are generic and other times includes the actual control type at the end of the name. It might be a good idea to use standard XAML naming convention and add the type name at the end of every template part.

Otherwise, this PR is all set.

@robloo robloo marked this pull request as ready for review August 24, 2022 00:15
@timunie
Copy link
Contributor

timunie commented Aug 24, 2022

It might be a good idea to use standard XAML naming convention and add the type name at the end of every template part.

I fully agree. The suffix should match the minimal base-class needed in code behind. So if it can be any Control, the name should end with Control for example.

@robloo
Copy link
Contributor Author

robloo commented Sep 7, 2022

@timunie While I think working on suffixes will be good to further standardize the names; I've decided it isn't worth the effort on my end to invest the time in it. It's not even clear this PR is going to get merged.

If you want to take this on yourself I would be happy to support.

@maxkatz6 maxkatz6 merged commit cd4f504 into AvaloniaUI:master Sep 8, 2022
@avaloniaui-team
Copy link
Contributor

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

@robloo robloo deleted the template-part-naming branch September 8, 2022 11:31
@maxkatz6
Copy link
Member

maxkatz6 commented Sep 8, 2022

@robloo this PR makes current state more consistent regardless people agree on naming conventions. If anybody would have counter points and ask to remove PART from everywhere, we have time to do so in this release.

p.s. just came back from the trip

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.

Update all TemplateParts to Match Convention
6 participants