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

User32 ActivateKeyBoardLayout #7451

Merged

Conversation

elachlan
Copy link
Contributor

@elachlan elachlan commented Jul 19, 2022

#7445

Microsoft Reviewers: Open in CodeFlow

@elachlan elachlan requested a review from a team as a code owner July 19, 2022 07:16
@ghost ghost assigned elachlan Jul 19, 2022
@RussKie RussKie added the waiting-review This item is waiting on review by one or more members of team label Jul 19, 2022
@RussKie RussKie requested a review from lonitra July 19, 2022 11:17
Copy link
Member

@JeremyKuhne JeremyKuhne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please keep a single NativeMethods.txt in the root for discoverability.

@ghost 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 Jul 19, 2022
@ghost 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 Jul 20, 2022
@elachlan
Copy link
Contributor Author

I performed the merge manually, I detected more zero white space and manually removed it. I suggest everyone be very careful when doing merges.

@elachlan elachlan requested a review from lonitra July 20, 2022 23:10
@@ -51,7 +53,7 @@ public static InputLanguage CurrentInputLanguage
value = DefaultInputLanguage;
}

IntPtr handleOld = User32.ActivateKeyboardLayout(value.Handle, 0);
HKL handleOld = PInvoke.ActivateKeyboardLayout(new HKL(value.Handle), 0);
if (handleOld == IntPtr.Zero)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we still comparing it with IntPtr.Zero? can we just check == 0 here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried, I think the compiler doesn't like it because HKL doesn't have an implicit conversion to int? I wasn't sure what to do.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just tried this syntax. It compiled fine when targeting .NET 6 and net472:

using Windows.Win32.UI.TextServices;

HKL hkl = default;
if (hkl == 0)
{
}

This is using CsWin32 0.2.1-beta.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HKL should have a .NULL on it, if not get the .Value and compare to 0.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HKL looks like a candidate for the IsNull property that we're supposed to generate on pointer-sized handles, per microsoft/CsWin32#206. If HKL is a handle (or a pointer) and we missed that, can you file a new issue for it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RussKie RussKie added waiting-review This item is waiting on review by one or more members of team and removed waiting-review This item is waiting on review by one or more members of team labels Jul 21, 2022
@@ -51,7 +53,7 @@ public static InputLanguage CurrentInputLanguage
value = DefaultInputLanguage;
}

IntPtr handleOld = User32.ActivateKeyboardLayout(value.Handle, 0);
HKL handleOld = PInvoke.ActivateKeyboardLayout(new HKL(value.Handle), 0);
if (handleOld == IntPtr.Zero)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just tried this syntax. It compiled fine when targeting .NET 6 and net472:

using Windows.Win32.UI.TextServices;

HKL hkl = default;
if (hkl == 0)
{
}

This is using CsWin32 0.2.1-beta.

var handles = new IntPtr[size];
fixed (IntPtr* pHandles = handles)
var handles = new HKL[size];
fixed (HKL* pHandles = handles)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for fixed here any more. With CsWin32 you can do this:

var handles = new HKL[5];
PInvoke.GetKeyboardLayoutList(handles);

IntPtr handleOld = User32.ActivateKeyboardLayout(value.Handle, 0);
if (handleOld == IntPtr.Zero)
HKL handleOld = PInvoke.ActivateKeyboardLayout(new HKL(value.Handle), 0);
if (handleOld.Value == 0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: the .Value here shouldn't be required.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me, it seems it is.
image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird. Well, I'm glad you have a workaround.

Copy link
Contributor Author

@elachlan elachlan Jul 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing a cast to nint works and there is no nint.Zero. HKL has an operator for nint, but not int. Should I just do the cast or is there a better way?

 HKL handleOld = PInvoke.ActivateKeyboardLayout(new HKL(value.Handle), 0);
                if (handleOld == (nint)0)
                {
                    throw new ArgumentException(SR.ErrorBadInputLanguage, nameof(value));
                }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cast isn't necessary in my project. Maybe it's an odd C# 11 or .NET 7 targeting thing. You can choose whatever syntax you're happiest with. They're all equivalent in the end.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does if (handleOld == default) work?

Copy link
Contributor Author

@elachlan elachlan Jul 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does if (handleOld == default) work?

yes! Well it compiles. I don't know if its the same. Would you like me to push that through?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for integral types such as int, IntPtr, etc. default == 0. So it's equivalent.

@dreddy-work
Copy link
Member

@elachlan , can you resolve merge conflict?

@dreddy-work dreddy-work added ready-to-merge PRs that are ready to merge but worth notifying the internal team. and removed waiting-review This item is waiting on review by one or more members of team labels Jul 29, 2022
Changes from review

remove zero width space character

changes from review

use default instead of .value==0
@elachlan elachlan force-pushed the User32-ActivateKeyboardLayout branch from 0bb7fd8 to 70314f5 Compare July 29, 2022 23:42
@dreddy-work dreddy-work merged commit 3b508c4 into dotnet:feature/win32 Jul 30, 2022
@elachlan elachlan deleted the User32-ActivateKeyboardLayout branch July 30, 2022 05:11
RussKie pushed a commit that referenced this pull request Aug 9, 2022
Migrate PInvokes Using CsWin32

Changes from review

remove zero width space character

changes from review

use default instead of .value==0
@ghost ghost locked as resolved and limited conversation to collaborators Aug 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
ready-to-merge PRs that are ready to merge but worth notifying the internal team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants