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

A bit of code refactor #15366

Closed
wants to merge 2 commits into from
Closed

A bit of code refactor #15366

wants to merge 2 commits into from

Conversation

heku
Copy link
Contributor

@heku heku commented Apr 14, 2024

What does the pull request do?

1 Move IsDirect = true into DirectPropertyBase
The line below should be better moved into the base class DirectPropertyBase instead of DirectProperty<TOwner, TValue>.

IsDirect = true;

2 Rename IDirectPropertyAccessor.Owner to OwnerType
The internal interface IDirectPropertyAccessor should reuse the exising property name in AvaloniaProperty, not introduce a new
duplicate one.

@maxkatz6
Copy link
Member

Thank you, but unfortunately removing Owner property is a breaking change. Some app might have used this property.
In some cases we can justify breaking changes, but I not in this one.

@heku
Copy link
Contributor Author

heku commented Apr 26, 2024

No problem, I made the change as it's an internal interface, I agree it might impact some apps even it's a non public interface.

@cla-avalonia
Copy link
Collaborator

cla-avalonia commented Apr 26, 2024

  • All contributors have signed the CLA.

@heku
Copy link
Contributor Author

heku commented Apr 26, 2024

@cla-avalonia agree

@heku heku deleted the refactor branch June 30, 2024 03:59
@heku
Copy link
Contributor Author

heku commented Jul 7, 2024

@maxkatz6 Hi, since we already introduced a break change to internal interface IDirectPropertyAccessor in PR https://github.com/AvaloniaUI/Avalonia/pull/15342/files#diff-665144eae92b5414e2672936b5ee880232eed13fc446f309685985ec6ca311ac. How about re-open this PR?

@maxkatz6
Copy link
Member

maxkatz6 commented Jul 8, 2024

@heku that's an internal interface though. Also, it wasn't backported.

@maxkatz6
Copy link
Member

maxkatz6 commented Jul 8, 2024

In general, we have API validation on the CI builds. So, it there was any unwanted breaking change, we would know (unless there is a bug in the validator).

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.

3 participants