Adding support for High DPI related API's and types. #381

ghost · 2018-04-23T00:26:15Z

Adding support for High DPI related API's and types.

See https://msdn.microsoft.com/en-us/library/windows/desktop/hh447398(v=vs.85).aspx

A couple of things I'd like feedback about.

i. OpenThemeDataForDpi. Ideally, this should return a SafeHandle, but I couldn't find a good way to do this. I wish that I'd caught this in a timely fashion and given feedback to move this into uxtheme.dll rather than expose it through user32.dll. Too late now.
ii. DPI_AWARENESS_CONTEXT could be just a bunch of static readonly IntPtr values, or it could be SafeHandle. I thought about it much and decided to go with a SafeHandle implementation, but I'm really not tied to this approach. The SafeHandle approach just feels more seamless to me when it comes time to pass these handles to various other functions. I'm also tempted to override Equals/GetHashCode etc. and implement equality using AreDpiAwarenessContextsEqual, but that feels just a whiff above the scope of PInvoke library.

See https://msdn.microsoft.com/en-us/library/windows/desktop/hh447398(v=vs.85).aspx

AArnott

Thank you for your significant contribution here. I can see you've taken time to review our contribution guidelines and held to it quite well. I do request a few changes here for you to consider. I look forward to accepting your contribution.

AArnott · 2018-04-23T04:21:06Z

src/User32/User32+DIALOG_CONTROL_DPI_CHANGE_BEHAVIORS.cs

+        /// <remarks>
+        /// This enum is used with SetDialogControlDpiChangeBehavior in order to override the default per-monitor DPI scaling behavior for a child window within a dialog.
+        ///
+        /// These settings only apply to individual controls within dialogs.The dialog-wide per-monitor DPI scaling behavior of a dialog is controlled by <see cref="DIALOG_DPI_CHANGE_BEHAVIORS"/>.


Missing a space after a period. This can happen when pasting comments from MSDN when the C# language service tries to format the comment as code. You can avoid the problem by always following up on a multi-line Paste with an Undo command, which will undo the automated formatting step but not the Paste command. #Closed

AArnott · 2018-04-23T04:23:24Z

src/User32/User32+DIALOG_DPI_CHANGE_BEHAVIORS.cs

+            /// <summary>
+            /// Prevents the dialog manager from re-layouting all of the dialogue's immediate children HWNDs in response to a DPI change.
+            /// </summary>
+            DDC_DISABLE_CONTROL_RELAYOUT = 0x0003


nit: add a trailing , to the last enum value so that any additions later on can just add lines rather than edit existing ones. It makes for a cleaner diff. #Closed

AArnott · 2018-04-23T04:29:58Z

src/User32/User32+DPI_AWARENESS_CONTEXT.cs

+        /// Identifies the awareness context for a window.
+        /// </summary>
+        /// <remarks>DPI_AWARENESS_CONTEXT values should never be compared directly. Instead, use <see cref="AreDpiAwarenessContextsEqual"/></remarks>
+        public class DPI_AWARENESS_CONTEXT : SafeHandle


Given this handle is never released, I don't think we should use SafeHandle as a base type, both because its misleading and because each instance of these types adds load to the CLR as it has to track these for finalization, I believe.
I'd be inclined to just use an enum to define these well known values, if all valid values are well known ones (not really private handles at all). If there are other values without names, then IIRC we typically handle this by typing it as an IntPtr or int and then define constants in the containing type (e.g. User32) for the well-known values. #Closed

Thanks for chiming in here.

The gotcha here is that all valid values are NOT well-known ones. The OS returns DPI_AWARENESS_CONTEXT handles that are different that -1, -2, -3, and -4. In fact, I've never seen these values returned by the OS - the handles returned by the OS are always different. So we end up with AreDpiAwarenessContextsEqual(new IntPtr(-1), ) == true frequently.

That said, I'll make these well known IntPtr values in User32.cs since I don't have a strong preference. These are not enums though - these are handles, so these would be defined as static readonly IntPtr values (alongwith corresponding changes to API's that consume these).

Thanks. That makes sense. Can you move these static readonly members to the parent class (User32) so that users don't have to specify DPI_AWARENESS_CONTEXT.DPI_AWARENESS_CONTEXT_UNAWARE and can just use DPI_AWARENESS_CONTEXT_UNAWARE the way native code does? You'll find that we already follow this pattern in User32 so this will fit right in.

In reply to: 183277213 [](ancestors = 183277213)

Will do - expect another commit today.

AArnott · 2018-04-23T04:34:01Z

src/User32/User32.cs

+        /// If the function succeeds, the return value is true.
+        /// If the function fails, the return value is false. To get extended error information, call <see cref="GetLastError"/>.
+        /// </returns>
+        [DllImport(nameof(User32), SetLastError = true, CharSet = CharSet.Unicode)]


Setting the CharSet is harmless, but no longer necessary since we declare DefaultCharSetAttribute for all our assemblies now. #Closed

I see that you're using this even on methods that don't take characters as parameters. Please at least remove this CharSet from those methods, if not all of those you added. #Closed

AArnott · 2018-04-23T04:36:08Z

src/User32/User32.cs

+        [DllImport(nameof(User32), SetLastError = true, CharSet = CharSet.Unicode)]
+        [return: MarshalAs(UnmanagedType.Bool)]
+        public static extern unsafe bool AdjustWindowRectExForDpi(
+            [In][Out] RECT* lpRect,


We don't specify [In] (or [Out]) on virtually any of our parameters. [In] seems implied and redundant (I believe in C# it strictly is redundant) and [Out] has no effect either AFAIK. Am I wrong?
Unless there's a particular reason to specify them, please remove them. #Closed

You are right, in my usage, these are all redundant. These can occasionally be useful though. for e.g., [In] ref STRUCT would change the marshaling semantics from being [in, out] to [in] only, which can be useful if the underlying native function works that way. It's just a reflexive habit - whenever I see [in], [out] annotations in native API definition, I tend to transcribe them faithfully in my P/Invoke definitions. Removing. #Closed

Good to know. We don't usually use ref struct anyway, since we use struct* and [Friendly] attributes to generate the ref overload. Since the FriendlyAttribute includes in/out flags, we may want to consider generating the [In/Out] attributes for those overloads.

In reply to: 183279321 [](ancestors = 183279321)

AArnott · 2018-04-23T04:37:22Z

src/User32/User32.cs

+        /// </returns>
+        [DllImport(nameof(User32), SetLastError = true, CharSet = CharSet.Unicode)]
+        [return: MarshalAs(UnmanagedType.Bool)]
+        public static extern unsafe bool EnableNonClientDpiScaling([In] void* hwnd);


We type our HWND handles as IntPtr everywhere because they are never dereferenced in user code and thus needn't be expressed as pointers anywhere. If you change this to match, HWND handles will be easier to pass around this library across this API to others.
Here and anywhere else you pass HWND around. #Closed

AArnott · 2018-04-23T04:40:22Z

src/User32/User32+DIALOG_DPI_CHANGE_BEHAVIORS.cs

+        /// This enum works in conjunction with SetDialogDpiChangeBehavior in order to override the default DPI scaling behavior for dialogs.
+        /// This does not affect DPI scaling behavior for the child windows of dialogs(beyond re-layouting), which is controlled by <see cref="DIALOG_CONTROL_DPI_CHANGE_BEHAVIORS"/>.
+        /// </summary>
+        public enum DIALOG_DPI_CHANGE_BEHAVIORS : int


At least some uses of this enum treat this as a flags enum (where multiple values can be set). Can you add [Flags] here? #Closed

AArnott · 2018-04-23T04:40:47Z

src/User32/User32+DIALOG_CONTROL_DPI_CHANGE_BEHAVIORS.cs

+        ///
+        /// These settings only apply to individual controls within dialogs.The dialog-wide per-monitor DPI scaling behavior of a dialog is controlled by <see cref="DIALOG_DPI_CHANGE_BEHAVIORS"/>.
+        /// </remarks>
+        public enum DIALOG_CONTROL_DPI_CHANGE_BEHAVIORS : int


At least some uses of this enum treat this as a flags enum (where multiple values can be set). Can you add [Flags] here? #Closed

Summary: * Adding [Flags] attribute to DIALOG_CONTROL_DPI_CHANGE_BEHAVIORS, DIALOG_DPI_CHANGE_BEHAVIORS * Adding trailing comma (,) to newly added enums as a matter of coding style . * Fixing spacing issues. * Removing DPI_AWARENESS_CONTEXT SafeHandle type, and replacing it with DPI_AWARENESS_CONTEST staitc class that holds well-known handles. I chose to use a static class instead of putting these directly in User32 by taking inspiration from how SpecialWindowHandles is organized. * Remove CharsSet from [DllImport] attributes, except in 1 instance where it serves a self-documenting purpose. * Removing [In],[Out] attributes from parameters since these were redundant. * Changing all instances of HWND parameters to use IntPtr instead of void*.

ghost · 2018-04-23T06:37:11Z

Hopefully, the latest commit addresses your feedback. Thanks for taking the time out to review! #Closed

ghost · 2018-04-23T18:40:06Z

@AArnott ,

Since DPI_AWARENESS_CONTEXT handles are never dereferenced, would it make sense to write P/Invoke signatures involving this handle type to use IntPtr only (much like your hint about how HWND's are organized in this library), rather than using void* pointers? Now that you've pointed out the HWND precedent, I'm inclined to change them to use IntPtr, but figured I'd check first.
Re OpenThemeDataForDpi, here is what I'm thinking:
- Implement a SafeThemeHandle type in User32, identical to the one in UxTheme
- Modify UxTheme.SafeThemeHandle to derive from User32.SafeThemeHandle
- That leaves the tricky part of implementing User32.SafeThemeHandle.ReleaseHandle(), which will require a call into uxtheme.dll!CloseThemeData(HTHEME). Perhaps I could implement this P/Invoke signature as a private DllImport inside the proposed User32.SafeThemeHandle and call into that. This breaks with the convention, but it would make it more seamless for callers of User32.OpenThemeDataForDpi to get a SafeThemeHandle that can be safely closed and also be used with other related UxTheme API's. Thoughts?

AArnott · 2018-04-25T00:05:26Z

Since DPI_AWARENESS_CONTEXT handles are never dereferenced, would it make sense to write P/Invoke signatures involving this handle type to use IntPtr only (much like your hint about how HWND's are organized in this library), rather than using void* pointers?

Yes, I agree. Use of IntPtr makes sense.

Regarding SafeThemeHandle, I think I'm missing some background for this. I'm surprised that CloseThemeData comes from uxtheme.dll if user32 deals with such handles. Are these two libraries intertwined in the OS such that our assembly boundary separation of the two is going to be problematic?

I would rather we define the handle type just once. Maybe we could move it to User32 if it's important that it be there. That may not have to be a breaking change if we just use a type forwarder in the uxtheme assembly.

- Use IntPtr (instread of void*) for functions that take DPI_AWARENESS_CONTEXT parameters. This is a good approach because these pointers are never dereferenced, so there is no good use-case for using unsafe/void* signatures. - Add a SafeThemeHandle type in User32 to support user32!OpenThemeDataForDpi. This is a clone of UxTheme.SafeThemeHandle, alongwith minor changes to support CloseThemeData. Add a conversion operator in UxTheme.SafeThemeHandle to enable implicit conversions from User32.SafeThemeHandle --> UxTheme.SafeThemeHandle. This will enable seamless use of the SafeThemeHandle returned by User32.OpenThemeDataForDpi in UxTheme API's that take UxTheme.SafeThemeHandle as a parameter.

ghost · 2018-04-27T03:07:30Z

@AArnott,

I've submitted another commit that moves the DPI_AWARENESS_CONTEXT constants directly under User32.

I tried to use [assembly:TypeForwardedTo] with UxTheme.SafeThemeHandle, but it didn't work well. Being a nested class inside UxTheme, moving it to the PInvoke.User32 assembly creates a duplicate definition for UxTheme class. I don't think there is any way to forward a partial type (UxTheme class, in this case) and have that resolve seamlessly. So the fact that SafeThemeHandle is a nested type turned out to be a stumbling block. Am I perhaps missing a better approach here that would let me to successfully forward an inner type?

I've submitted an alternative that does duplicate the handle type - which I realize isn't ideal - but attempts to avoid breaking changes. I do this by introducing User32.SafeThemeHandle, and an implicit conversion operator from User32.SafeThemeHandle --> UxTheme.SafeThemeHandle, which should enable User32.SafeThemehandle produced by User32.OpenThemeDataForDpi to be used by UxTheme methods that expect UxTheme.SafeThemeHandle parameters.

Let me know what you think of this approach. If you'd rather just avoid all this complexity and return a pointer from User32.OpenThemeDataForDpi, and let the user figure out how she ought to manage that handle, then I can always change things back. That said, I do think it's worth returning a SafeHandle in this instance.

This also lets us remove User32+SafeThemeHandle, which was redundant.

AArnott

I moved the OpenThemeDataForDpi method to UxTheme. It is defined in uxtheme.h, so although it is implemented in user32.dll in Windows, by moving it to uxtheme I think it's more predictable and it allows us to entirely avoid the problem of two SafeThemeHandle classes.

AArnott · 2018-04-29T00:48:36Z

Do you agree with the commit I added to the PR? If so, I'll complete the PR.

ghost · 2018-04-29T06:53:47Z

@AArnott, It's a good trade-off you've proposed, so please go ahead with the approach of exposing OpenThemeDataForDpi through UxTheme.

Thanks for taking the time to review and proposing this alternative option!

AArnott · 2018-04-29T14:50:07Z

Thank you very much for the contribution!

Adding support for High DPI related API's and types.

3cd3400

See https://msdn.microsoft.com/en-us/library/windows/desktop/hh447398(v=vs.85).aspx

AArnott suggested changes Apr 23, 2018

View reviewed changes

vatsan-madhavan and others added 2 commits April 27, 2018 13:02

Adding entry for operator SafeThemeHandle in PublicApi.Unshipped.txt

d045633

Move OpenThemeDataForDpi to UxTheme

c9e99dc

This also lets us remove User32+SafeThemeHandle, which was redundant.

AArnott approved these changes Apr 29, 2018

View reviewed changes

AArnott merged commit 2eaa7f2 into dotnet:master Apr 29, 2018

ghost deleted the HighDpiApi branch May 1, 2018 21:12

Uh oh!

Adding support for High DPI related API's and types. #381

Adding support for High DPI related API's and types. #381

Uh oh!

Conversation

ghost commented Apr 23, 2018

Uh oh!

AArnott left a comment

Choose a reason for hiding this comment

Uh oh!

AArnott Apr 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AArnott Apr 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AArnott Apr 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost Apr 23, 2018

Choose a reason for hiding this comment

Uh oh!

AArnott Apr 23, 2018

Choose a reason for hiding this comment

Uh oh!

ghost Apr 23, 2018

Choose a reason for hiding this comment

Uh oh!

AArnott Apr 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AArnott Apr 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AArnott Apr 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost Apr 23, 2018 • edited by AArnott Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AArnott Apr 23, 2018

Choose a reason for hiding this comment

Uh oh!

AArnott Apr 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AArnott Apr 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AArnott Apr 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Apr 23, 2018 • edited by AArnott Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Apr 23, 2018

Uh oh!

AArnott commented Apr 25, 2018

Uh oh!

ghost commented Apr 27, 2018

Uh oh!

AArnott left a comment

Choose a reason for hiding this comment

Uh oh!

AArnott commented Apr 29, 2018

Uh oh!

ghost commented Apr 29, 2018 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AArnott commented Apr 29, 2018

AArnott Apr 23, 2018 •

edited

Loading

AArnott Apr 23, 2018 •

edited

Loading

AArnott Apr 23, 2018 •

edited

Loading

AArnott Apr 23, 2018 •

edited

Loading

AArnott Apr 23, 2018 •

edited

Loading

AArnott Apr 23, 2018 •

edited

Loading

ghost Apr 23, 2018 •

edited by AArnott

Loading

AArnott Apr 23, 2018 •

edited

Loading

AArnott Apr 23, 2018 •

edited

Loading

AArnott Apr 23, 2018 •

edited

Loading

ghost commented Apr 23, 2018 •

edited by AArnott

Loading

ghost commented Apr 29, 2018 •

edited by ghost

Loading