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

Add Control.SizeChanged Event #9307

Merged
merged 7 commits into from
Nov 1, 2022
Merged

Conversation

robloo
Copy link
Contributor

@robloo robloo commented Oct 31, 2022

What does the pull request do?

Adds a Control.SizeChanged event.

What is the current behavior?

There is no size changed event and apps must listen directly to Bounds property changes. This isn't optimal for those with existing WPF/UWP/WinUI code and those that aren't heavily using ReactiveUI.

What is the updated/expected behavior with this PR?

There is now a Control.SizeChanged event which is the equivalent to WPF's FrameworkElement.SizeChanged.

NOTE

In Avalonia this event could probably go much lower and be on Layoutable or Visual. However, I kept it on Control which is the equivalent for FrameworkElement.

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

  1. WPF event details:
  2. WinUI event details:
  3. The WPF event args class was followed instead of the UWP/WinUI one.
  4. I decided to keep using Size for the NewSize/PreviousSize properties instead of Rect that is used by the Bounds property in Avalonia. This is for two reasons (1) compatibility with WPF and (2) it ensures the event args are sufficiently generic to be used in other places.
  5. As done elsewhere in most other Avalonia events, no new EventHandler was created for this.

Checklist

Breaking changes

None

Obsoletions / Deprecations

None

Fixed issues

Closes #9311
Relates to the discussion #8473

@jp2masa
Copy link
Contributor

jp2masa commented Oct 31, 2022

I may be totally wrong, but from what I've seen in OxyPlot, maybe TransformedBounds needs to be considered here?

@robloo
Copy link
Contributor Author

robloo commented Oct 31, 2022

@jp2masa You might be right. However, I'm not entirely sure either. Hopefully one of the core maintainers can clarify.

This follows past direction in the linked discussion though.

Edit: Bounds is the equivalent of WPF's ActualHeight/Width. Actualheight/Width changes raise this property in WPF. Therefore, watching for bounds changes should be enough. ActualHeight/Width and Bounds properties better reflect the actual state of the control and take into account any transforms -- if they don't I would say there is a bug in Bounds calculation.

@avaloniaui-team
Copy link
Contributor

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

@amwx
Copy link
Contributor

amwx commented Oct 31, 2022

In Avalonia this event could probably go much lower and be on Layoutable or Visual. However, I kept it on Control which is the equivalent for FrameworkElement.

Layoutable.ArrangeCore, where Bounds is set, might make better sense for this, but it probably doesn't really matter.


> I may be totally wrong, but from what I've seen in OxyPlot, maybe `TransformedBounds` needs to be considered here?

@jp2masa You might be right. However, I'm not entirely sure either. Hopefully one of the core maintainers can clarify.

This follows past direction in the linked discussion though.

Edit: Bounds is the equivalent of WPF's ActualHeight/Width. Actualheight/Width changes raise this property in WPF. Therefore, watching for bounds changes should be enough. ActualHeight/Width and Bounds properties better reflect the actual state of the control and take into account any transforms -- if they don't I would say there is a bug in Bounds calculation.

From my understanding, Bounds takes into account layout transforms (like DPI scaling, for example) but not render transforms, so I'd say Bounds is correct here. Although there is a bug or change in the new composition renderer where TransformedBounds isn't set and is always null. (I keep meaning to open an issue about that)

@robloo
Copy link
Contributor Author

robloo commented Oct 31, 2022

In Avalonia this event could probably go much lower and be on Layoutable or Visual. However, I kept it on Control which is the equivalent for FrameworkElement.

Layoutable.ArrangeCore, where Bounds is set, might make better sense for this, but it probably doesn't really matter.

On second thought, we want to care about performance here too. This event really isn't needed on other types and making it as high up the inheritance chain as possible is a good thing. My own personal goal is WPF/WinUI compatibility so we should be ok with things as they are.

@avaloniaui-team
Copy link
Contributor

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

@avaloniaui-team
Copy link
Contributor

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

@maxkatz6
Copy link
Member

maxkatz6 commented Oct 31, 2022

This isn't optimal for those with existing WPF/UWP/WinUI code and those that aren't heavily using ReactiveUI

Why do you need ReactiveUI otherwise?

Only inconvenience was that developer need to handle property changed event and filter by property. Which isn't intuitive for developers who came from WPF/UWP.

@robloo
Copy link
Contributor Author

robloo commented Oct 31, 2022

This isn't optimal for those with existing WPF/UWP/WinUI code and those that aren't heavily using ReactiveUI

Why do you need ReactiveUI otherwise?

You don't need it. But in ReactiveUI it's fast and easy to hookup to the property changed event. If you aren't using ReactiveUI it's even more of a pain. The event is much nicer overall for those of us with apps using older patterns.

Only inconvenience was that developer need to handle property changed event and filter by property. Which isn't intuitive for developers who came from WPF/UWP.

Yes, this is much less of a problem for derived controls where you can override the internal OnPropertyChanged method. However, if you are hooking up to a user control template part and updating things internally a size change event is much easier to use.

@avaloniaui-team
Copy link
Contributor

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

/// Initializes a new instance of the <see cref="SizeChangedEventArgs"/> class.
/// </summary>
/// <param name="routedEvent">The routed event associated with these event args.</param>
public SizeChangedEventArgs(RoutedEvent? routedEvent)
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if these events should have internal ctor. Like it was discussed here #8860
But either way it's not important right now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That conversation doesn't make sense to me. There is little harm in having these public -- this API isn't going to change. More though, how are apps supposed to implement this event themselves if constructors are private? I never understood why you are hiding constructors.

@avaloniaui-team
Copy link
Contributor

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

@maxkatz6 maxkatz6 merged commit 7b5b167 into AvaloniaUI:master Nov 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

How to Implement "SizeChanged"
5 participants