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

[API Proposal]: Implement IEquatable interface on DpiScale #9996

Open
h3xds1nz opened this issue Oct 25, 2024 · 2 comments
Open

[API Proposal]: Implement IEquatable interface on DpiScale #9996

h3xds1nz opened this issue Oct 25, 2024 · 2 comments
Labels
API suggestion Early API idea and discussion, it is NOT ready for implementation

Comments

@h3xds1nz
Copy link
Contributor

h3xds1nz commented Oct 25, 2024

Background and motivation

Currently, the DpiScale struct does not implement IEquatable, while we have an internal implemented wrapper class DpiScale2 that does implement it and wraps this struct. This is for example used as a dictionary key in StylusLogic, which could then be simplified to use the regular struct and avoid for example temporary heap allocs on lookup.

There were already some boxing issues solved in #6309 but the equality is done with an internal Equals currently, however as Stephen Toub mentions, the hope is for DpiScale to implement IEquatable in the future and I couldn't agree more.

While at it, I'd mark the struct as readonly since it is designed to be immutable anyways.

The hope is to have then 1 unifying type that we can use to represent DpiScale in different scenarios and not having to depend on internal types that are just wrappers around the struct to use it in other places.

API Proposal

namespace System.Windows;

- public partial struct DpiScale
+ public readonly partial struct DpiScale : IEquatable<DpiScale>
{
+   public bool Equals(DpiScale other)
+   public override bool Equals(object obj)
+   public override int GetHashCode()
+   public static bool operator ==(DpiScale left, DpiScale right)
+   public static bool operator !=(DpiScale left, DpiScale right)
}

API Usage

Besides other places:

private readonly Dictionary<DpiScale2, Matrix> _transformToDeviceMatrices = new Dictionary<DpiScale2, Matrix>();

Possible additions

We could also add a public utility factory method that's currently also on DpiScale2, that is FromPixelsPerInch, I believe it has its uses:

internal static DpiScale2 FromPixelsPerInch(double ppiX, double ppiY)

Risks

Adding readonly is not a breaking change, adding IEquatable can obviously produce a new behavior in terms of equality comparisons but I believe it is the right thing to do moving forward.

@miloush
Copy link
Contributor

miloush commented Oct 25, 2024

I wonder why was it worth wrapping it in a new internal type rather than just implementing it directly on the original struct (or creating a comparer).

Anyway, proposal looks good to me.

@h3xds1nz
Copy link
Contributor Author

I couldn't find anything tangible either even in the old assemblies, it was added in .NetFX 4.8 from what I could dig up; previously from 4.6.2 it was just DpiScale but I guess if they wanted quick and dirty way as there otherwise weren't really public API changes in the latest releases, just bugfixes including Automation stuff.

I was hoping to check some of the hosting scenarios awareness issues and while I knew there are two different members, I didn't know that DpiScale2 was just a plain IEquatable wrapper.

@ThomasGoulet73 ThomasGoulet73 added the API suggestion Early API idea and discussion, it is NOT ready for implementation label Oct 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API suggestion Early API idea and discussion, it is NOT ready for implementation
Projects
None yet
Development

No branches or pull requests

3 participants