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

Convert various DirectProperty definitions to StyledProperty #10370

Conversation

TomEdwardsEnscape
Copy link
Contributor

@TomEdwardsEnscape TomEdwardsEnscape commented Feb 16, 2023

What does the pull request do?

This is the first change which directly addresses #9944, the end goal of which is to avoid public DirectProperty setters on types which derive from StyledElement.

What is the current behavior?

See #9944.

What is the updated/expected behavior with this PR?

This PR refactors a large batch of properties from DirectProperty to StyledProperty. The new SetCurrentValue method is now used to set the value of refactored properties, where necessary.

Only properties which could be easily changed are included. Properties with complicated behaviours will be handled in smaller PRs.

There are some additional changes:

  • In a few cases, a DirectProperty was not changed to StyledProperty, but instead had its setter removed. StyledElement.TemplatedParent is a notable example: the CLR property has an internal setter, but this could be evaded by calling SetValue(TemplatedParentProperty). Closing backdoors like these reduces Avalonia's API surface.
  • TemplateBinding now uses SetCurrentValue.
  • The various property AddOwner methods now accept a metadata object, mirroring WPF's API.
  • ComboBox.SelectionBoxItem has been changed from a protected property to a public property with a protected setter. This better reflects the accessibility of the DirectProperty it wraps around.
  • Button.IsPressed has been changed to a read-only DirectProperty, and an old test that tried to data bind to it has been removed.

Breaking changes

  • The refactored properties now function with binding priorities. This means that when a value source is removed, the property now reverts to a different value source (or the default value). Anyone who relied on the old, broken behaviour of the value remaining unchanged in this scenario will have to refactor their code.
  • Anyone who calls AddOwner on a refactored property will have to refactor their controls to match the new property type.
  • Anyone sneakily calling internal C# property setters with the incorrectly-exposed DirectProperty setters that this PR removes will have to stop hacking Avalonia. 🤡

Obsoletions / Deprecations

None

Fixed issues

Part of #9944

@avaloniaui-team
Copy link
Contributor

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

@TomEdwardsEnscape TomEdwardsEnscape force-pushed the refactor/readonly-directprop/easy-styledprop-conversions branch from 1fb88fa to 775d33f Compare February 21, 2023 19:13
@avaloniaui-team
Copy link
Contributor

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

@TomEdwardsEnscape TomEdwardsEnscape force-pushed the refactor/readonly-directprop/easy-styledprop-conversions branch from 775d33f to 107cb67 Compare February 23, 2023 08:43
@avaloniaui-team
Copy link
Contributor

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

@grokys
Copy link
Member

grokys commented Feb 23, 2023

Looks like Avalonia.IntegrationTests.Appium.CheckBoxTests.UncheckedCheckBox is broken after these changes.

@TomEdwardsEnscape
Copy link
Contributor Author

TomEdwardsEnscape commented Feb 24, 2023

I was hoping that would prove to be a temporary glitch, but it apparently is not. Is there a guide somewhere for setting up Appium locally?

@robloo
Copy link
Contributor

robloo commented Feb 24, 2023

@tomenscape Looks like the best guide is the Readme under tests/Avalonia.IntegrationTests.Appium.

ToggleButton changes are likely the root. However, the only thing I see that could be wrong is SetCurrentValue(). Otherwise, there is an extension method GetIsChecked() in ElementExtensions.cs which might be the issue. Honestly not sure how this works to begin with though.

@grokys
Copy link
Member

grokys commented Feb 24, 2023

It does indeed look like checkboxes are broken ;)

3MvZNjMR4s.mp4

@robloo
Copy link
Contributor

robloo commented Feb 25, 2023

Well, that's interesting. However, it might be an issue with SetCurrentValue() and derived controls. It looks similar to the ColorPicker issues as well.

@grokys
Copy link
Member

grokys commented Feb 25, 2023

Yeah, I don't have time to investigate last night. Will try to work out what's happening if no-one else gets there first :)

@grokys
Copy link
Member

grokys commented Feb 27, 2023

Another problem I've noticed: inspecting a control in DevTools breaks the margin on certain controls:

tFmW4aZ3s3.mp4

Probably caused by the change to ThicknessEditor: might it make sense to revert the changes to internal properties for the moment so we have less to deal with? We can easily change them to styled properties in future.

@grokys
Copy link
Member

grokys commented Feb 27, 2023

However, it might be an issue with SetCurrentValue()

Yep, looks like SetCurrentValue has problems, this will need a fix.

@grokys
Copy link
Member

grokys commented Feb 27, 2023

Opened #10497 with a fix.

@TomEdwardsEnscape TomEdwardsEnscape force-pushed the refactor/readonly-directprop/easy-styledprop-conversions branch from 107cb67 to dcaaad7 Compare February 27, 2023 19:01
@TomEdwardsEnscape
Copy link
Contributor Author

That PR also fixes the ThicknessEditor problem.

@avaloniaui-team
Copy link
Contributor

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

@grokys
Copy link
Member

grokys commented Mar 1, 2023

Apologies for the delay on this, PRs are being help up by problems with our CI. Waiting for the SetCurrentValue fix to hit master and will re-run the integration tests.

@avaloniaui-team
Copy link
Contributor

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

Copy link
Member

@grokys grokys left a comment

Choose a reason for hiding this comment

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

All unit and integration tests now passing, did a manual test with ControlCatalog and didn't see any problems so I say we're ready to go!

Copy link
Member

@grokys grokys left a comment

Choose a reason for hiding this comment

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

Oh wait, I just took a final look and it seems like non-Control properties are still converted to StyledProperty. In the comment in #10370 (comment) I meant that I think we should remove these changes for now, as

  1. They can't really be styled anyway
  2. Some of the properties are collection types, which are problematic in combination with styling currently (see below)

Changed Button.IsPressed to a read-only DirectProperty
@TomEdwardsEnscape TomEdwardsEnscape force-pushed the refactor/readonly-directprop/easy-styledprop-conversions branch from 68086ee to f36cf7e Compare March 2, 2023 15:29
@avaloniaui-team
Copy link
Contributor

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

@grokys grokys added this pull request to the merge queue Mar 2, 2023
Merged via the queue into AvaloniaUI:master with commit 0a97f8b Mar 2, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to it being already merged Mar 2, 2023
@TomEdwardsEnscape TomEdwardsEnscape deleted the refactor/readonly-directprop/easy-styledprop-conversions branch March 2, 2023 18:51
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.

6 participants