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

[WIP] Cleanup HWND related interop #1508

Closed
wants to merge 1 commit into from

Conversation

hughbe
Copy link
Contributor

@hughbe hughbe commented Jul 28, 2019

Microsoft Reviewers: Open in CodeFlow

@hughbe hughbe requested a review from a team as a code owner July 28, 2019 13:15
@hughbe hughbe force-pushed the cleanup-window-interop branch from 15323f9 to 5a2877c Compare July 28, 2019 14:16
@codecov
Copy link

codecov bot commented Jul 28, 2019

Codecov Report

Merging #1508 into master will increase coverage by 0.03902%.
The diff coverage is 43.57394%.

@@                 Coverage Diff                 @@
##              master       #1508         +/-   ##
===================================================
+ Coverage   25.94161%   25.98063%   +0.03902%     
===================================================
  Files            807         836         +29     
  Lines         267786      267992        +206     
  Branches       37960       37953          -7     
===================================================
+ Hits           69468       69626        +158     
- Misses        193385      193436         +51     
+ Partials        4933        4930          -3
Flag Coverage Δ
#Debug 25.98063% <43.57394%> (+0.03902%) ⬆️
#production 25.98063% <43.57394%> (+0.03902%) ⬆️
#test 100% <ø> (ø) ⬆️

@dreddy-work
Copy link
Member

Thanks @hughbe for this PR. We need to thoroughly validate these changes to make sure we are not regressing anything. Change pattern is straight but number of changes are huge. For core 3.0, We do not have app.config support for HDPI and validation of these changes need extra efforts. We can take this PR for 3.1.
One thing i noticed in the changes is, OS validation/api availability check missing. was that intentional? if yes, please add some explanation.
@RussKie FYI.

@hughbe hughbe mentioned this pull request Jul 29, 2019
Copy link
Member

@RussKie RussKie left a comment

Choose a reason for hiding this comment

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

Completed review of the Interop namespace
Further review pending

@hughbe hughbe force-pushed the cleanup-window-interop branch 3 times, most recently from 42f4a1f to 873cbda Compare July 29, 2019 21:01
@hughbe hughbe force-pushed the cleanup-window-interop branch 2 times, most recently from 873cbda to e2b1320 Compare July 30, 2019 14:11
@RussKie RussKie added 🚫 * NO-MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) 📭 waiting-author-feedback The team requires more information from the author labels Jul 31, 2019
@hughbe hughbe force-pushed the cleanup-window-interop branch from e2b1320 to 87dbcd5 Compare August 4, 2019 18:19
@ghost ghost removed the 📭 waiting-author-feedback The team requires more information from the author label Aug 4, 2019
@hughbe hughbe force-pushed the cleanup-window-interop branch from 87dbcd5 to d001919 Compare August 6, 2019 22:22
This was referenced Aug 7, 2019
@@ -74,8 +74,8 @@ public class CheckedListBox : ListBox

static CheckedListBox()
{
LBC_GETCHECKSTATE = SafeNativeMethods.RegisterWindowMessage("LBC_GETCHECKSTATE");
LBC_SETCHECKSTATE = SafeNativeMethods.RegisterWindowMessage("LBC_SETCHECKSTATE");
LBC_GETCHECKSTATE = Interop.User32.RegisterWindowMessageW("LBC_GETCHECKSTATE");
Copy link
Member

Choose a reason for hiding this comment

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

@JeremyKuhne previously we had safe and unsafe native methods segregated, with all the refactoring and cleanup work (incl. this one) we're loosing this distinction.
Is this something we need to be concerned with?

Copy link
Member

Choose a reason for hiding this comment

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

Is this something we need to be concerned with?

No. It isn't a pattern that makes sense with CAS gone. https://docs.microsoft.com/en-us/visualstudio/code-quality/ca1060-move-p-invokes-to-nativemethods-class?view=vs-2019

@RussKie RussKie added the 📭 waiting-author-feedback The team requires more information from the author label Aug 12, 2019
@hughbe hughbe changed the title Cleanup HWND related interop [WIP] Cleanup HWND related interop Aug 12, 2019
@ghost ghost removed the 📭 waiting-author-feedback The team requires more information from the author label Aug 12, 2019
@hughbe hughbe force-pushed the cleanup-window-interop branch 2 times, most recently from 3b2443b to 8a92483 Compare August 13, 2019 07:54
@RussKie RussKie added this to the Future milestone Aug 13, 2019
@hughbe hughbe force-pushed the cleanup-window-interop branch from 8a92483 to 6c2e07f Compare August 28, 2019 14:22
@hughbe hughbe force-pushed the cleanup-window-interop branch from 6c2e07f to 4973b41 Compare August 30, 2019 08:27
@RussKie RussKie removed the 🚫 * NO-MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Sep 4, 2019
@hughbe
Copy link
Contributor Author

hughbe commented Sep 5, 2019

Closing this temporarily.

@hughbe hughbe closed this Sep 5, 2019
@RussKie RussKie removed this from the Future milestone May 7, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Feb 5, 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