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.ActualWidthProperty and Control.ActualHeightProperty #9345

Closed
wants to merge 4 commits into from

Conversation

comesx4
Copy link
Contributor

@comesx4 comesx4 commented Nov 3, 2022

What does the pull request do?

Add Control.ActualWidth and Control.ActualHeight property.

What is the current behavior?

We can get the control's actual width and height from Bounds property, But the existing WPF coders are familiar to ActualWidth and ActualHeight property.

What is the updated/expected behavior with this PR?

Add Control.ActualWidth and Control.ActualHeight which are the equivalent to WPF's FrameworkElement.ActualWidth and ActualHeight.

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

Inspired by #9307 , simply add two DirectProperty on Control and return value form Bounds.
Raise property changed in SizeChangedEvent.

Checklist

- [ ] Added unit tests (if possible)?

  • Added XML documentation to any related classes?

- [ ] Consider submitting a PR to https://github.com/AvaloniaUI/Documentation with user documentation

Breaking changes

None

Obsoletions / Deprecations

None

Fixed issues

None

@dnfadmin
Copy link

dnfadmin commented Nov 3, 2022

CLA assistant check
All CLA requirements met.

@avaloniaui-team
Copy link
Contributor

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

@robloo
Copy link
Contributor

robloo commented Nov 3, 2022

I very briefly thought about this with size changed. However. I didnt want to complicate the API for little reason. Working with an observable on the Bounds property vs. SizeChanged event was problematic for a few reasons so it seems to be a clear missing feature.

However, ActualHeight and ActualWidth is not filling the gaps in a missing feature. It is duplicating API for compatibility. Generally, I think the decision to do this should be very rare and in this case doesn't meet the threshold. It's ridiculously easy to change code over to using .Bounds.Width instead of .ActualWidth. An actual find and replace can do it with no issue. Again, the same isn't true for SizeChanged which is a slight feature enhancement and allows compatibility with different code patterns (not something find replace can fix).

So while this is good for compatibility I'm not actually in favor of this PR. I think we should fill gaps is functionality but not try for perfect compatibility with WPF.

Besides, Avalonia seems to be the only real future for Xaml frameworks and I've found its better just to do a clean break rather than trying to maintain WinUI or WPF alongside Avalonia sources.

@maxkatz6
Copy link
Member

maxkatz6 commented Nov 5, 2022

I agree with @robloo here. It was possible to replace SizeChanged with subscribing on a property changed. But it was adding quite some unnecessary code for the very common task. Not to mention it wasn't intuitive and many developers were thinking this possibility is just missing.
While with ActualWidth/Bounds.Width we indeed have a different naming here, but it doesn't require user to write more unintuitive code.

So, I am going to close it now. Thanks @comesx4 for the interest in contributing!

@maxkatz6 maxkatz6 closed this Nov 5, 2022
@comesx4
Copy link
Contributor Author

comesx4 commented Nov 7, 2022

Fine, that's ok.

@comesx4 comesx4 deleted the add-actualsize branch December 6, 2022 01:44
@comesx4 comesx4 restored the add-actualsize branch December 6, 2022 01:44
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.

5 participants