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.

Sign up for GitHub

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

Jump to bottom

Refactor RangeBase to use StyledProperty #10803

Merged
merged 7 commits into from
Apr 6, 2023
Merged

Refactor RangeBase to use StyledProperty #10803

merged 7 commits into from
Apr 6, 2023

Conversation

TomEdwardsEnscape
Copy link
Contributor

@TomEdwardsEnscape TomEdwardsEnscape commented Mar 26, 2023 •
edited
Loading

What does the pull request do?

This PR changes direct properties within RangeBase to styled properties, as part of #9944.

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

  • ScrollViewer has been switched from a model in which the viewer provides template components with esoteric properties (many with "backdoor" property setters), to a model in which the template components automatically subscribe to the standard viewer properties and derive their own values. This eliminates backdoor property setters, reduces API surface area, and simplifies templates and tests.
    • Custom bindings can still be set on the affected properties, and will override built-in behaviour.
  • A bug in which children of a control can be attached to the visual or logical trees multiple times has been fixed.

Breaking changes

  • Custom ScrollViewer templates will need to have various bindings removed, so that the ScrollBar and ScrollTemplatePresenter can generate their own.
  • Anyone overriding the changed properties will have to switch to StyledProperty.

Obsoletions / Deprecations

None

@grokys
Copy link
Member

grokys commented Mar 28, 2023

Will look at fixing #10684 now.

grokys
grokys reviewed Mar 28, 2023
View reviewed changes
Copy link
Member

@grokys grokys left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Just some comments on looking through the code so far. I think we need to decide just how far we want to take this for 11.0... ScrollViewer in particular is really in need of a general overhaul but I'm not sure that can be done before 12.0.

src/Avalonia.Controls/ProgressBar.cs Outdated Show resolved Hide resolved
src/Avalonia.Base/AvaloniaObjectExtensions.cs Outdated Show resolved Hide resolved
src/Avalonia.Base/Visual.cs Show resolved Hide resolved
src/Avalonia.Controls/Primitives/ScrollBar.cs Show resolved Hide resolved
src/Avalonia.Controls/ScrollViewer.cs Outdated Show resolved Hide resolved
src/Avalonia.Themes.Fluent/Controls/MenuScrollViewer.xaml Show resolved Hide resolved
@grokys
Copy link
Member

grokys commented Mar 28, 2023

Default value coercion should be fixed in #10822.

@robloo
Copy link
Contributor

robloo commented Mar 28, 2023

I think we need to decide just how far we want to take this for 11.0... ScrollViewer in particular is really in need of a general overhaul but I'm not sure that can be done before 12.0.

I think most controls should be looked at (I keep the list updated in #9944) but some are clearly special cases that should be handled later. I'm personally not a fan of converting all properties whether they make sense or not.

ScrollViewer is a special case. Upstream WinUI is completely redoing this control for a number of reasons. Avalonia should likely follow that API after its implemented and tested. I think there are even things done there to fix some of the ItemsRepeater edge cases that never worked.

The new specs are here: microsoft/microsoft-ui-xaml#7245. No idea if it will be completed but if so, I think it would be a candidate for Avalonia 12.

@robloo robloo mentioned this pull request Mar 28, 2023
Closed
69 tasks
@TomEdwardsEnscape TomEdwardsEnscape mentioned this pull request Mar 28, 2023
Merged
@TomEdwardsEnscape
Converted RangeBase to StyledProperty
74f75b0 
Normalised ScrollViewer internal properties and bindings
@TomEdwardsEnscape
Fix tests
2a5a5d2
@TomEdwardsEnscape
Fixed children being attached to the visual or logical tree multiple …
b400a1f 
…times
@TomEdwardsEnscape
Convert ScrollGestureRecognizer to an unstyled element
e3c6ee0 
Added missing binding of IsScrollInertiaEnabled in the simple theme
@TomEdwardsEnscape TomEdwardsEnscape force-pushed the refactor/rangebase-styledproperty branch from c833baf to a4f4661 Compare April 1, 2023 11:13
@TomEdwardsEnscape TomEdwardsEnscape marked this pull request as ready for review April 1, 2023 11:13
@TomEdwardsEnscape TomEdwardsEnscape mentioned this pull request Apr 1, 2023
Closed
@TomEdwardsEnscape
Don't detach from ScrollViewer when detached from visual tree
87e2ed8 
Fixes offset resetting when switching between tabs
@TomEdwardsEnscape TomEdwardsEnscape force-pushed the refactor/rangebase-styledproperty branch from a4f4661 to 87e2ed8 Compare April 1, 2023 11:36
@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 11.0.999-cibuild0032698-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-cibuild0032700-beta. (feed url: https://pkgs.dev.azure.com/AvaloniaUI/AvaloniaUI/_packaging/avalonia-all/nuget/v3/index.json) [PRBUILDID]

@grokys grokys enabled auto-merge April 5, 2023 13:30
@avaloniaui-team
Copy link
Contributor

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

@grokys
Copy link
Member

grokys commented Apr 6, 2023

Sorry, I thought I'd already approved this but appears not. LGTM, lets get this into master to see if it breaks anything ;)

@grokys grokys added this pull request to the merge queue Apr 6, 2023
@avaloniaui-team
Copy link
Contributor

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

Merged via the queue into AvaloniaUI:master with commit 70b6da2 Apr 6, 2023
@grokys grokys deleted the refactor/rangebase-styledproperty branch April 6, 2023 08:37
@TomEdwardsEnscape
Copy link
Contributor Author

Already fixed the first bug with #10929. Should have spotted it in this PR!

grokys added a commit that referenced this pull request Apr 7, 2023
@grokys
Update tests due to changes in #10803.
e3bf19a
grokys added a commit that referenced this pull request Sep 28, 2023
@grokys
Remove Content binding from ScrollViewer template.
788ff5c 
Since #10803, the `ScrollContentPresenter.Content` binding is managed internally, not via a binding in the template.
@grokys grokys mentioned this pull request Sep 28, 2023
Merged
maxkatz6 pushed a commit that referenced this pull request Oct 1, 2023
@grokys
Fix initial logical scrollable state (#13066)
bf1be0c 
* Add failing test for #13064

* Remove Content binding from ScrollViewer template.

Since #10803, the `ScrollContentPresenter.Content` binding is managed internally, not via a binding in the template.

* Assign owner before doing subscriptions.

Fixes #13064.
grokys added a commit that referenced this pull request Oct 2, 2023
@grokys
Fix initial logical scrollable state (#13066)
6513cd4 
* Add failing test for #13064

* Remove Content binding from ScrollViewer template.

Since #10803, the `ScrollContentPresenter.Content` binding is managed internally, not via a binding in the template.

* Assign owner before doing subscriptions.

Fixes #13064.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@grokys grokys grokys approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@TomEdwardsEnscape @grokys @robloo @avaloniaui-team