Open h3xds1nz opened 1 month ago
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.
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.
Background and motivation
Currently, the
DpiScale
struct does not implementIEquatable
, while we have an internal implemented wrapper classDpiScale2
that does implement it and wraps this struct. This is for example used as a dictionary key inStylusLogic
, 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 forDpiScale
to implementIEquatable
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
API Usage
Besides other places:
https://github.com/dotnet/wpf/blob/6c96913d693c56922e697b23b3053aa604d3bd2c/src/Microsoft.DotNet.Wpf/src/PresentationCore/System/Windows/Input/Stylus/Common/StylusLogic.cs#L183
Possible additions
We could also add a public utility factory method that's currently also on
DpiScale2
, that isFromPixelsPerInch
, I believe it has its uses: https://github.com/dotnet/wpf/blob/6c96913d693c56922e697b23b3053aa604d3bd2c/src/Microsoft.DotNet.Wpf/src/PresentationCore/MS/internal/DpiScale2.cs#L202Risks
Adding
readonly
is not a breaking change, addingIEquatable
can obviously produce a new behavior in terms of equality comparisons but I believe it is the right thing to do moving forward.