InputLanguage: Use RegLoadMUIString instead of SHLoadIndirectString API #8921

DJm00n · 2023-03-30T16:17:41Z

I have extracted part of #8573 into separate PR until decision is made on some aspects of that PR.

Proposed changes

Use RegLoadMUIString instead of the SHLoadIndirectString API as recommended at https://learn.microsoft.com/windows/win32/intl/locating-redirected-strings#load-a-language-neutral-registry-value
Move InputLanguage GetKeyboardLayoutNameForHKL method implementation to the internal LayoutId property (to ease debugging)
Added proper testing of InputLanguage.LayoutName
Added InputLanguage_Culture_ThrowsArgumentException and InputLanguage_LayoutName_UnknownExpected tests
Use PARAM.ToInt instead of casts in GetHashCode implementation

Customer Impact

Shouldn't have any visible customer impact

Regression?

No

Risk

Little

Test methodology

Auto test:

InputLanguage_InputLanguageLayoutId_Expected (updated)
InputLanguage_Culture_ThrowsArgumentException (new)
InputLanguage_LayoutName_UnknownExpected (new)

Manually:

Install "Canadian Multilingual Standard" keyboard layout in Windows.
Add breakpoint in InputLanguage_InstalledInputLanguages_Get_ReturnsExpected test and run it under debugger.
Check that its LayoutId is 00011009 under debugger.
Check that its LayoutName is "Canadian Multilingual Standard".

Microsoft Reviewers: Open in CodeFlow

DJm00n · 2023-04-05T11:21:39Z

@JeremyKuhne any objections on this topic?

JeremyKuhne

Comments are inline.

JeremyKuhne · 2023-04-05T17:40:07Z

src/System.Windows.Forms.Primitives/src/Windows/Win32/Pinvike.RegLoadMUIString.cs

+ var hkey = (System.Registry.HKEY)key.Handle.DangerousGetHandle();
+ uint bytes = 0;
+ if (RegLoadMUIString(hkey, keyName, null, 0, &bytes, 0, null) != WIN32_ERROR.ERROR_MORE_DATA)
+ return false;


Our coding style requires blocks for if .. else.

JeremyKuhne · 2023-04-05T17:42:39Z

src/System.Windows.Forms.Primitives/src/Windows/Win32/Pinvike.RegLoadMUIString.cs

+ return false;
+ }
+
+ localizedValue = buffer.Slice(0, (int)bytes / sizeof(char) - 1).ToString();


nit: use parenthesis for precedence.

JeremyKuhne · 2023-04-05T17:43:47Z

src/System.Windows.Forms.Primitives/src/Windows/Win32/Pinvike.RegLoadMUIString.cs

+
+namespace Windows.Win32
+{
+ internal static partial class PInvoke


The filename is misspelled.

JeremyKuhne · 2023-04-05T17:55:44Z

src/System.Windows.Forms/src/System/Windows/Forms/InputLanguage.cs

-
- return false;
- }
+ public override int GetHashCode() => PARAM.ToInt(_handle);


PARAM.ToInt is no longer necessary, and somewhat misleading. Just cast it to int. I'd also suggest changing _handle to nint as we've done elsewhere.

JeremyKuhne · 2023-04-05T18:10:04Z

src/System.Windows.Forms.Primitives/src/Windows/Win32/Pinvike.RegLoadMUIString.cs

+ internal static partial class PInvoke
+ {
+ /// <inheritdoc cref="RegLoadMUIString(System.Registry.HKEY, string, PWSTR, uint, uint*, uint, string)"/>
+ public static unsafe bool RegLoadMUIString(RegistryKey key, string keyName, out string localizedValue)


This looks like something that we should create an API proposal for in the runtime. RegistryKey.GetMUIString() maybe?

To be clear- I don't think we should block here, but I would assume this functionality is generally useful for our developers.

@JeremyKuhne I have added RegistryKeyExtensions.GetMUIString extension method

JeremyKuhne · 2023-04-05T18:11:01Z

src/System.Windows.Forms.Primitives/src/Windows/Win32/Pinvike.RegLoadMUIString.cs

+ public static unsafe bool RegLoadMUIString(RegistryKey key, string keyName, out string localizedValue)
+ {
+ localizedValue = string.Empty;
+ if (key.Handle.IsInvalid)


I would expect that this is uncommon, maybe better to just let if fail with the call to RegLoadMUIString().

JeremyKuhne · 2023-04-05T18:20:27Z

src/System.Windows.Forms.Primitives/src/Windows/Win32/Pinvike.RegLoadMUIString.cs

+ if (RegLoadMUIString(hkey, keyName, null, 0, &bytes, 0, null) != WIN32_ERROR.ERROR_MORE_DATA)
+ return false;
+
+ Span<char> buffer = stackalloc char[(int)bytes / sizeof(char)];


The data here appears to be unbounded. One presumes that this could be pretty expensive if it follows through to a PE file to load a string resource just to return the length.

The safer/better choice is to stack allocate some reasonable buffer and then rent/return a bigger array from the ArrayPool if the size isn't large enough. It may also make sense to just use the ArrayPool, as I the benefit of skipping the rental is probably minimal at best. Just using the ArrayPool would make the logic simpler, for sure.

One presumes that this could be pretty expensive if it follows through to a PE file to load a string resource just to return the length.

I can bet that RegLoadMUIString is caching MUI strings to avoid this rereading but I'll do the changes.

I can bet that RegLoadMUIString is caching MUI strings to avoid this rereading but I'll do the changes.

It doesn't look like it. You'll probably get some general IO caching though. The more critical issue is that the buffer size is unbounded, which is a problem for stack allocation.

DJm00n · 2023-04-10T16:16:14Z

src/System.Windows.Forms.Primitives/src/Windows/Win32/Pinvoke.RegLoadMUIString.cs

+ uint bytes;
+ WIN32_ERROR error;
+ var hkey = (System.Registry.HKEY)key.Handle.DangerousGetHandle();
+ char[] buffer = ArrayPool<char>.Shared.Rent(MAX_PATH + 1);


Several places in code are already using MAX_PATH for displayName buffer...

We shouldn't use MAX_PATH as a buffer value unless it is actually explicitly tied to the value. This has nothing to do with paths. Also consider that the ArrayPool returns buffers in fixed sizes (2^n). This would get you a buffer of 512 chars. 128 is probably a sensible initial buffer that would cover the vast majority of cases.

JeremyKuhne

Added a few more comments.

JeremyKuhne · 2023-04-10T22:30:47Z

src/System.Windows.Forms.Primitives/src/Windows/Win32/Pinvoke.RegLoadMUIString.cs

+ uint bytes;
+ WIN32_ERROR error;
+ var hkey = (System.Registry.HKEY)key.Handle.DangerousGetHandle();
+ char[] buffer = ArrayPool<char>.Shared.Rent(MAX_PATH + 1);


We shouldn't use MAX_PATH as a buffer value unless it is actually explicitly tied to the value. This has nothing to do with paths. Also consider that the ArrayPool returns buffers in fixed sizes (2^n). This would get you a buffer of 512 chars. 128 is probably a sensible initial buffer that would cover the vast majority of cases.

JeremyKuhne · 2023-04-11T00:24:41Z

src/System.Windows.Forms.Primitives/src/Windows/Win32/Pinvoke.RegLoadMUIString.cs

+ localizedValue = new string(buffer, 0, Math.Max((int)(bytes / sizeof(char)) - 1, 0));
+ ArrayPool<char>.Shared.Return(buffer);
+
+ return HRESULT.HRESULT_FROM_WIN32(error).Succeeded;


You should only try to create the string when ERROR_SUCCESS is returned and that should be the only thing you check to return success.

DJm00n · 2023-04-18T19:58:28Z

Hi @JeremyKuhne! Do you have any updates on this review?

JeremyKuhne · 2023-04-18T20:38:39Z

@DJm00n I'm in the process of adding a BufferScope that makes writing this sort of thing easier. #8999

It would be good to use that to simplify this if possible. You should be able to do something similar to what I did with PInvoke.GetModuleFileNameLongPath.cs in that change.

Hopefully I'll have it checked in this afternoon.

DJm00n · 2023-04-19T10:57:02Z

GetModuleFileNameLongPath

Nice! This is indeed looks more simple!

Pushed updated version of PInvoke.RegLoadMUIString.

DJm00n · 2023-04-25T14:56:42Z

Hi @JeremyKuhne! Any updates on this review?

JeremyKuhne

Looks great- thanks for your patience!

dreddy-work · 2023-04-26T00:31:50Z

@DJm00n, can you rebase this?

As recommended at https://learn.microsoft.com/windows/win32/intl/locating-redirected-strings#load-a-language-neutral-registry-value Additional changes: - Move InputLanguage GetKeyboardLayoutNameForHKL method implementation to the internal LayoutId property - Better testing of InputLanguage.LayoutName

DJm00n · 2023-04-26T10:07:44Z

@dreddy-work done

DJm00n requested a review from a team as a code owner March 30, 2023 16:17

ghost assigned DJm00n Mar 30, 2023

DJm00n changed the title ~~Use RegLoadMUIString API instead of SHLoadIndirectString~~ InputLanguage: Use RegLoadMUIString API instead of SHLoadIndirectString Mar 30, 2023

DJm00n changed the title ~~InputLanguage: Use RegLoadMUIString API instead of SHLoadIndirectString~~ InputLanguage: Use RegLoadMUIString instead of SHLoadIndirectString API Mar 30, 2023

runfoapp bot mentioned this pull request Mar 30, 2023

Flaky test: monthcalendar_click_date_invokeeventsasync #8740

Open

dreddy-work requested a review from JeremyKuhne March 30, 2023 17:57

DJm00n force-pushed the use-regloadmuistring branch 2 times, most recently from 2e1b71d to 6524066 Compare April 4, 2023 14:06

JeremyKuhne requested changes Apr 5, 2023

View reviewed changes

ghost added 📭 waiting-author-feedback The team requires more information from the author and removed 📭 waiting-author-feedback The team requires more information from the author labels Apr 5, 2023

DJm00n force-pushed the use-regloadmuistring branch from 6524066 to 7028c9d Compare April 10, 2023 16:11

DJm00n requested a review from JeremyKuhne April 10, 2023 16:14

DJm00n commented Apr 10, 2023

View reviewed changes

JeremyKuhne requested changes Apr 11, 2023

View reviewed changes

ghost added the 📭 waiting-author-feedback The team requires more information from the author label Apr 11, 2023

DJm00n force-pushed the use-regloadmuistring branch from 7028c9d to fb449f0 Compare April 11, 2023 11:44

DJm00n requested a review from JeremyKuhne April 11, 2023 11:44

ghost removed the 📭 waiting-author-feedback The team requires more information from the author label Apr 11, 2023

This was referenced Apr 11, 2023

system.windows.forms.uitests.buttontests.button_dialogresult_clickdefaultbuttontocloseformasync #8734

Open

Flaky test: button_dialogresult_clickdefaultbuttontocloseformasync(dialogresult: abort) #8747

Open

DJm00n force-pushed the use-regloadmuistring branch from fb449f0 to f7f33eb Compare April 19, 2023 10:55

JeremyKuhne previously approved these changes Apr 25, 2023

View reviewed changes

dreddy-work added the 📭 waiting-author-feedback The team requires more information from the author label Apr 26, 2023

DJm00n dismissed JeremyKuhne’s stale review via 392e638 April 26, 2023 10:05

DJm00n force-pushed the use-regloadmuistring branch from f7f33eb to 392e638 Compare April 26, 2023 10:05

ghost removed the 📭 waiting-author-feedback The team requires more information from the author label Apr 26, 2023

DJm00n requested a review from JeremyKuhne April 26, 2023 10:11

dreddy-work enabled auto-merge (squash) May 4, 2023 20:19

dreddy-work approved these changes May 4, 2023

View reviewed changes

dreddy-work merged commit f989dc5 into dotnet:main May 4, 2023

ghost added this to the 8.0 Preview5 milestone May 4, 2023

DJm00n deleted the use-regloadmuistring branch May 4, 2023 20:50

ghost locked as resolved and limited conversation to collaborators Jun 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

InputLanguage: Use RegLoadMUIString instead of SHLoadIndirectString API #8921

InputLanguage: Use RegLoadMUIString instead of SHLoadIndirectString API #8921

DJm00n commented Mar 30, 2023 •

edited

Loading

DJm00n commented Apr 5, 2023

JeremyKuhne left a comment

JeremyKuhne Apr 5, 2023

JeremyKuhne Apr 5, 2023

JeremyKuhne Apr 5, 2023

JeremyKuhne Apr 5, 2023

JeremyKuhne Apr 5, 2023

JeremyKuhne Apr 5, 2023

DJm00n Apr 10, 2023

JeremyKuhne Apr 5, 2023

JeremyKuhne Apr 5, 2023

DJm00n Apr 7, 2023

JeremyKuhne Apr 7, 2023

DJm00n Apr 10, 2023 •

edited

Loading

JeremyKuhne Apr 10, 2023

JeremyKuhne left a comment

JeremyKuhne Apr 10, 2023

JeremyKuhne Apr 11, 2023

DJm00n commented Apr 18, 2023

JeremyKuhne commented Apr 18, 2023

DJm00n commented Apr 19, 2023 •

edited

Loading

DJm00n commented Apr 25, 2023

JeremyKuhne left a comment

dreddy-work commented Apr 26, 2023

DJm00n commented Apr 26, 2023

InputLanguage: Use RegLoadMUIString instead of SHLoadIndirectString API #8921

InputLanguage: Use RegLoadMUIString instead of SHLoadIndirectString API #8921

Conversation

DJm00n commented Mar 30, 2023 • edited Loading

Proposed changes

Customer Impact

Regression?

Risk

Test methodology

Microsoft Reviewers: Open in CodeFlow

DJm00n commented Apr 5, 2023

JeremyKuhne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DJm00n Apr 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeremyKuhne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DJm00n commented Apr 18, 2023

JeremyKuhne commented Apr 18, 2023

DJm00n commented Apr 19, 2023 • edited Loading

DJm00n commented Apr 25, 2023

JeremyKuhne left a comment

Choose a reason for hiding this comment

dreddy-work commented Apr 26, 2023

DJm00n commented Apr 26, 2023

DJm00n commented Mar 30, 2023 •

edited

Loading

DJm00n Apr 10, 2023 •

edited

Loading

DJm00n commented Apr 19, 2023 •

edited

Loading