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

ThemeVariant property definitions to ThemeVariantScope + fix DataGrid theme variant switch #10149

Merged
merged 12 commits into from
Feb 21, 2023

Conversation

maxkatz6
Copy link
Member

@maxkatz6 maxkatz6 commented Feb 1, 2023

What does the pull request do?

Fixes: #10292 - Popup wasn't properly inheriting theme variant from the placement target
Fixes: #10201 - Workaround for #10345, default value of actual theme now is "null" instead of "Light" (correct value is inherited)
Fixes: #9410 - Fluent DataGrid theme wasn't using theme dictionaries
Fixes: #10319 - Some Simple control themes that were ported from Fluent had the same problem as DataGrid, but instead I simplified them to be in-line with other simple control themes.
Fixes: #10309 - Invalid property name registration was breaking bindings.
Also seem to fix #10110

@maxkatz6 maxkatz6 enabled auto-merge February 1, 2023 06:58
@avaloniaui-team
Copy link
Contributor

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

@avaloniaui-team
Copy link
Contributor

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

@robloo
Copy link
Contributor

robloo commented Feb 4, 2023

I never did get through the original PR. Just noticed this though:

/// <summary>
/// Specifies a UI theme variant that should be used for the 
/// </summary>
[TypeConverter(typeof(ThemeVariantTypeConverter))]
public sealed record ThemeVariant
  1. Missing comments. Just was never completed.
  2. Is record really the best to use here?
    • I know it had many uses now but it was originally designed to represent a row in a database table and give boilerplate implementations for equals/etc based on values. It doesn't look quite right in this context. record class would look a bit better but I suppose that's preference.
    • You don't actually take advantage of any record features from what I see. You re-implemented Equals and GetHashCode anyway.
    • You DID NOT add new implementations for the equality operators... so == (implemented by the compiler) will be different than .Equals() which is bad practice.

Anyway, just added this here since you are making some fixes. I can delete and create a new issue if you want.

@maxkatz6
Copy link
Member Author

maxkatz6 commented Feb 4, 2023

Missing comments. Just was never completed.

Thanks, will fix that later.

record class would look a bit better but I suppose that's preference.

"record class" and "record" is the same thing. Unlikely "record struct" which is different.

You DID NOT add new implementations for the equality operators

No, record will use user defined Equals method. I did double check this.

You don't actually take advantage of any record features from what I see

Tbh, initially when I wrote this class, there was more benefits of using record. But then I changed much.
Still there are some benefits while no disadvantages.
See sharplab output for this type. You can replace "record" with "class" there and compare.
Differences:

  • record automatically implements IEquatable
  • record automatically defienes == and != operators
  • record automatically overrides Equals(object) method and calls user defined Equals(ThemeVariant)
  • record defines Clone for variant with {} syntax, which is rather useless as there are no publicly settable properties
    It also has defined PrintMembers method, which seems to be leftover from original generated ToString, which is replaced by user-written one. But since it's a private trimmable method, that's not a problem.

auto-merge was automatically disabled February 12, 2023 15:58

Merge queue setting changed

@maxkatz6 maxkatz6 mentioned this pull request Feb 12, 2023
3 tasks
@maxkatz6 maxkatz6 added the bug label Feb 15, 2023
@maxkatz6
Copy link
Member Author

@amwx I haven't checked what's wrong again with #10110, but by any chance could it be the same as #10201? And possibly fixed in this PR.

@avaloniaui-team
Copy link
Contributor

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

@amwx
Copy link
Contributor

amwx commented Feb 15, 2023

@amwx I haven't checked what's wrong again with #10110, but by any chance could it be the same as #10201? And possibly fixed in this PR.

Just tested with this branch and HighContrast theme seems to be working correctly now (both on load and switching)

@avaloniaui-team
Copy link
Contributor

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

@avaloniaui-team
Copy link
Contributor

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

@maxkatz6 maxkatz6 merged commit 827402d into master Feb 21, 2023
@maxkatz6 maxkatz6 deleted the theme-variant-changes branch February 21, 2023 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment