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 missing Screen.Equals/GetHashCode overrides #17112

Merged
merged 1 commit into from
Sep 24, 2024

Conversation

MrJul
Copy link
Member

@MrJul MrJul commented Sep 24, 2024

This PR adds missing overrides for Equals(object) and GetHashCode() on Screen.
This has two purposes: first to fix some build warnings, and second to correct the screen equality behavior.

Before this PR, Screen.Equals(Screen) was delegating to base.Equals(object). I think it was meant to call the Equals(object) overridden in the derived PlatformScreen, but the base call make it non-virtual.

Basically, Equals(Screen) was always equivalent to ReferenceEquals() and thus different from Equals(object).

Equals(object) now calls Equals(Screen), which is overridden.
Ideally, we should make Screen, Screen.Equals(Screen) and Screen.GetHashCode() abstract in v12.

@MrJul MrJul added the bug label Sep 24, 2024
@MrJul MrJul requested a review from maxkatz6 September 24, 2024 13:12
@avaloniaui-bot
Copy link

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

@maxkatz6
Copy link
Member

This PR adds missing overrides for Equals(object) and GetHashCode() on Screen.

These were removed from my refactoring PR, as implementation was relying on mutable properties, when IPlatformHandle wasn't available. Which was the case with Linux initially.
Now, since Linux backend provides a proper IPlatformHandle, these overloads make more sense.

Ideally, we should make Screen, Screen.Equals(Screen) and Screen.GetHashCode() abstract in v12.

Ideally, I would make Screen class sealed (or with an internal ctor). And only (i.e. - only) created by the windowing backend, always backed by the IPlatformHandle.

@maxkatz6 maxkatz6 added this pull request to the merge queue Sep 24, 2024
Merged via the queue into AvaloniaUI:master with commit 33bd02c Sep 24, 2024
11 checks passed
@MrJul MrJul deleted the fix/screen-override-equals branch November 19, 2024 10:11
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.

3 participants