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

Add a low level window class #3010

Merged
merged 2 commits into from
Mar 30, 2020
Merged

Add a low level window class #3010

merged 2 commits into from
Mar 30, 2020

Conversation

JeremyKuhne
Copy link
Member

@JeremyKuhne JeremyKuhne commented Mar 25, 2020

Adds WindowClass for testing windows messages.

Adds InternalsVisibleTo so we can expose helpers for Interop testing from internal test utility.

Ref to SWF in internal test utility should be removed. This is one step. Tests using AxHost should refactor AxHost helper methods into SWF.Primitives (don't move the whole AxHost down).

Fix a bug in Cursor interop.

Microsoft Reviewers: Open in CodeFlow

Adds WindowClass for testing windows messages.

Adds InternalsVisibleTo so we can expose helpers for Interop testing from internal test utility.

Ref to SWF in internal test utility should be removed. This is one step. Tests using AxHost should refactor AxHost helper methods into SWF.Primitives (don't move the whole AxHost down).

Fix a bug in Cursor interop.
@JeremyKuhne JeremyKuhne requested a review from RussKie March 25, 2020 05:27
@JeremyKuhne JeremyKuhne requested a review from a team as a code owner March 25, 2020 05:27
@ghost ghost assigned JeremyKuhne Mar 25, 2020
@codecov
Copy link

codecov bot commented Mar 25, 2020

Codecov Report

Merging #3010 into master will increase coverage by 0.01157%.
The diff coverage is 100.00000%.

@@                 Coverage Diff                 @@
##              master       #3010         +/-   ##
===================================================
+ Coverage   62.14366%   62.15523%   +0.01157%     
===================================================
  Files           1257        1257                 
  Lines         449428      449428                 
  Branches       39227       39227                 
===================================================
+ Hits          279291      279343         +52     
+ Misses        164660      164606         -54     
- Partials        5477        5479          +2     
Flag Coverage Δ
#Debug 62.15523% <100.00000%> (+0.01157%) ⬆️
#production 33.34748% <100.00000%> (+0.02061%) ⬆️
#test 98.98504% <ø> (ø)

public const int IDC_NO = 32648;
public const int IDC_HAND = 32649;
public const int IDC_APPSTARTING = 32650;
public const int IDC_HELP = 32651;
Copy link
Contributor

Choose a reason for hiding this comment

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

should we extract this to enum IDC

Copy link
Member Author

Choose a reason for hiding this comment

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

Thinking it over a bit. They're technically native integers (IntPtr) which is giving me some pause. Probably overthinking. :)

Copy link
Contributor

@weltkante weltkante Mar 25, 2020

Choose a reason for hiding this comment

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

Actually, no, they aren't IntPtr constants, they are 2 byte integers. You are supposed to pass them through MAKEINTRESOURCE which takes a UInt16. Of course if you drop the distinction between integer resource ids and string resource names then both can be treated as IntPtr (basically inlining MAKEINTRESOURCE into the definition)

From an API point of view I'd prefer providing overloads for enum resource ids vs. string resource names rather than merging both into IntPtr (i.e. inline MAKEINTRESOURCE into the called function overload instead of having the caller do it)

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, technically the numbers themselves are int literals, and the define usage (e.g. IDC_NO, etc.) always effectively gives you a char* or w_char* "literal". 😛

I do agree with you, however, that passing them around as int is better in this case as there aren't a lot of API entry points. In some other cases (such as HWND_TOPMOST), I am inclined to expose statics.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not trying to nitpick but my point was that any literal integer not fitting in a 2 byte integer wouldn't be allowed because it'd be either cut off when passed through MAKEINTRESOURCE or, if you would put it directly into an IntPtr, it would be interpreted as a pointer to a resource name, not a numeric resource id. So for all practical purposes the numeric range of resource ids is UInt16.

@RussKie RussKie merged commit 91218ab into dotnet:master Mar 30, 2020
@ghost ghost added this to the 5.0 milestone Mar 30, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Feb 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants