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

Fix: RangeBase should not write coerced values back when DataContext is changing #11918

Merged

Conversation

timunie
Copy link
Contributor

@timunie timunie commented Jun 27, 2023

What does the pull request do?

solves an issue where the bound value was changed when data context was changed or cleared

What is the current behavior?

coerced value was written back to the datacontext which is a recent regression

What is the updated/expected behavior with this PR?

regression doesn't happen anymore

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

added a bool flag skipping the coerce if the datacontext is beeing changed

Checklist

Breaking changes

Obsoletions / Deprecations

Fixed issues

Fixes #11870

fyi @ahopper : I tested this branch with your sample and it was solved for me. I'd be happy if you can verify this PR.

@grokys Hope you don't mind I took a look into it. If the fix is wrong, feel free to cherry-pick the Unit-Test.

@robloo
Copy link
Contributor

robloo commented Jun 27, 2023

My instinct is this is a general issue and should probably be addressed at a lower level.

  1. Avalonia has DataContext switching differences from WPF (there are issues for it some place)
  2. Coercion still might not work correctly in all edge cases (again, some issues for coercion might still be open)

@avaloniaui-team
Copy link
Contributor

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

@ahopper
Copy link
Contributor

ahopper commented Jun 29, 2023

This control has always had the issue that the order you set the properties in XAML matters, if the value comes first it will be clamped to the defaults before the desired min and max are set. There seem to be issues with coercion at both ends of the lifetime.

@ahopper
Copy link
Contributor

ahopper commented Jun 29, 2023

An alternative solution might just be to make the default Minimum and Maximum values min and max double values.

@timunie
Copy link
Contributor Author

timunie commented Jun 29, 2023

An alternative solution might just be to make the default Minimum and Maximum values min and max double values.

probably an idea for now. But the underlaying issue should be solved anyway.

@avaloniaui-team
Copy link
Contributor

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

@ahopper
Copy link
Contributor

ahopper commented Aug 6, 2023

@timunie I'm currently using a hybrid of this pr and big default min max values in my app with no issues, the big limits are belt and braces and probably not needed.

@timunie
Copy link
Contributor Author

timunie commented Aug 6, 2023

We need to wait for @maxkatz6 to check and merge this PR. Currently his workload is different 😉

@timunie timunie added the backport-candidate-11.0.x Consider this PR for backporting to 11.0 branch label Sep 29, 2023
Copy link
Member

@maxkatz6 maxkatz6 left a comment

Choose a reason for hiding this comment

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

ISupportInitialize could help with XAML order, but not with DataContext reloading. Will keep this as a hack for now...

@maxkatz6 maxkatz6 merged commit d7c82a1 into AvaloniaUI:master Dec 5, 2023
5 checks passed
maxkatz6 pushed a commit that referenced this pull request Dec 5, 2023
…is changing (#11918)

* Add failing test for RangeBase overriding Value on DataContext changed

* Add bool flag to skip coercing min, max and value while DataContext is

changing
@maxkatz6 maxkatz6 added backported-11.0.x and removed backport-candidate-11.0.x Consider this PR for backporting to 11.0 branch labels Dec 5, 2023
@timunie timunie deleted the fix/RangeBase_ValueGetsOverridden branch December 5, 2023 07:40
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.

Slider clamps bound value to default min max value when removed from visual tree
5 participants