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

Remove duplicate DpiHelper implementation #2131

Merged
merged 5 commits into from
Oct 29, 2019

Conversation

hughbe
Copy link
Contributor

@hughbe hughbe commented Oct 18, 2019

Closes #2120

Proposed Changes

  • First we need to consolidate the various Interop functions (GetProcessDpiAwareness, SetProcessDpiAwareness, IsValidDpiAwarenessContext, SetProcessDpiAwarenessContext, IsProcessDPIAware and SetProcessDPIAware) allowing us to use the common file
  • Then include the common file in System.Windows.Forms.Design.Editors.csproj
  • Include the function ScaleButtonImageLogicalToDevice in the common file
Microsoft Reviewers: Open in CodeFlow

@hughbe hughbe requested a review from a team as a code owner October 18, 2019 15:06
@hughbe hughbe force-pushed the DpiHelper-consolidate branch from ad9ee99 to 059ee5b Compare October 18, 2019 16:58
@codecov
Copy link

codecov bot commented Oct 18, 2019

Codecov Report

Merging #2131 into master will decrease coverage by 0.03577%.
The diff coverage is 5.12821%.

@@                 Coverage Diff                 @@
##              master       #2131         +/-   ##
===================================================
- Coverage   29.12306%   29.08728%   -0.03578%     
===================================================
  Files            940         939          -1     
  Lines         266665      266577         -88     
  Branches       37946       37937          -9     
===================================================
- Hits           77661       77540        -121     
- Misses        183787      183818         +31     
- Partials        5217        5219          +2
Flag Coverage Δ
#Debug 29.08728% <5.12821%> (-0.03578%) ⬇️
#production 29.08728% <5.12821%> (-0.03578%) ⬇️
#test 100% <ø> (ø) ⬆️

@merriemcgaw
Copy link
Member

This could have some significant impact if not done perfectly to the layout and behavior of controls. Please do some UI validations and ensure that things appear OK on secondary monitors, and that there's no behavior changes when dragging a form with many controls from one monitor to the next. That isn't working great, but we don't want to to regress further.

@zsd4yr
Copy link
Contributor

zsd4yr commented Oct 18, 2019

@dreddy-work should definitely look at this

@hughbe hughbe force-pushed the DpiHelper-consolidate branch from 059ee5b to 9ad84d3 Compare October 18, 2019 20:36
@RussKie RussKie requested a review from dreddy-work October 21, 2019 01:15
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.

LGTM

@RussKie RussKie added the code cleanup cleanup code for unused apis/properties/comments - no functional changes. label Oct 21, 2019
@hughbe hughbe force-pushed the DpiHelper-consolidate branch from 9ad84d3 to 7000515 Compare October 23, 2019 10:25
@hughbe
Copy link
Contributor Author

hughbe commented Oct 23, 2019

This could have some significant impact if not done perfectly to the layout and behavior of controls. Please do some UI validations and ensure that things appear OK on secondary monitors, and that there's no behavior changes when dragging a form with many controls from one monitor to the next. That isn't working great, but we don't want to to regress further.

I'm not really able to do this at the moment as I only have a mac and a windows VM. Any suggestions for secondary monitor testing? I wouldn't expect any regressions as this is simply cleaning up interop then removing a duplicate file. That said, I am willing to be proved wrong :D

@hughbe hughbe force-pushed the DpiHelper-consolidate branch from 7000515 to 89bbb45 Compare October 25, 2019 09:02
@gpetrou
Copy link
Contributor

gpetrou commented Oct 25, 2019

Perhaps remove CommonUnsafeNativeMethodsTests.cs or move/enable the commented tests with this PR?

@hughbe
Copy link
Contributor Author

hughbe commented Oct 26, 2019

Yeah. smart

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.

Thanks! The existing interop methods weren't using the right data type for DPI_AWARENESS_CONTEXT. I swear I wrote this up at some point, but it might have been in the internal temporary Git repository when this project was just starting. Ugh. :)

@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 Oct 27, 2019
@hughbe hughbe force-pushed the DpiHelper-consolidate branch 2 times, most recently from 67f530c to 4c2e824 Compare October 28, 2019 10:33
@RussKie RussKie added the 📭 waiting-author-feedback The team requires more information from the author label Oct 28, 2019
@hughbe hughbe force-pushed the DpiHelper-consolidate branch from 4c2e824 to 5afe0b2 Compare October 28, 2019 12:02
@ghost ghost removed the 📭 waiting-author-feedback The team requires more information from the author label Oct 28, 2019
@hughbe hughbe force-pushed the DpiHelper-consolidate branch from 5afe0b2 to 6f7ea04 Compare October 29, 2019 09:16
@RussKie
Copy link
Member

RussKie commented Oct 29, 2019

@JeremyKuhne it is all yours :)

@dreddy-work it would be great if you had a look too.

@dreddy-work
Copy link
Member

Change functionally look good to me.

@JeremyKuhne JeremyKuhne merged commit 961316a into dotnet:master Oct 29, 2019
@ghost ghost added this to the 5.0 milestone Oct 29, 2019
@hughbe hughbe deleted the DpiHelper-consolidate branch October 29, 2019 22:32
@ghost ghost locked as resolved and limited conversation to collaborators Feb 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
code cleanup cleanup code for unused apis/properties/comments - no functional changes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Duplicatated DpiHelper implementations
8 participants