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

Fix ControlPaint regressions #3956

Merged
merged 4 commits into from
Sep 23, 2020

Conversation

JeremyKuhne
Copy link
Member

@JeremyKuhne JeremyKuhne commented Sep 19, 2020

Proposing porting the changes that fix the class of off by one bugs for the end point of lines that were switched from GDI+ to GDI (for performance and memory usage). Note that the majority of the code in these commits are test code.

Fixes #3953
Fixes #3945
Fixes #3931
Fixes #3944

Proposed changes

  • Adjust end points one over to render correctly
  • Remove unreachable code
  • Move misplaced return statement

Customer Impact

  • Control vendors controls render incorrectly as they build on our helper code
  • Customer UI has gaps in numerous controls and strange artifacts in the NumericUpDown and DomainUpDown controls.

Regression?

  • Yes

Risk

  • Very low

Screenshots

UpDownControl example, before and after:

image
image

Good grab handle is on left, bad is on right: (while the right may look more appealing out of context making it "centered" would require all users to upgrade their code to get their existing behavior)

image

Good DrawBorder is on the left, bad on the right. The dotted/dashed are ok as that always draws with GDI+.

image

Bad DrawBorder on left, good on right.

image

Test methodology

  • Manual pixel validation between 3.1 and 5.0
  • Wrote automated rendering regression tests
  • Look at internal callers of helper methods to ensure they are using the APIs the way they used to
Microsoft Reviewers: Open in CodeFlow

GDI and GDI+ don't end their lines and rectangles on the same pixel. Lines end on the prior pixel and rectangles draw the right and bottom one pixel over.

- Fix the GDI coordinates to match the GDI+ rendering
- Remove the GDI+ fork as it is impossible to set a transparent background color for NumericUpDown
- Add initial rendering tests

Validated with A/B testing between 3.1 and 5.0 with a loud background color. Validated with and without visual styles, enabled and disabled, and with different border styles.
* Fix ControlPaint.DrawBorder dashed/dotted drawing

Fixes dotnet#3944

- Move return statement so it doesn't double draw
- Add regression tests

* Remove blank line
Fixes dotnet#3945

The DrawBorder overload that takes individual side parameters needed to adjust line ends when drawing in GDI.

Manually validated output at different widths and transparencies between 3.1 and 5.0. Will follow up with automated regression tests in a separate PR.
End point needed adjusted for GDI to match GDI+

Fixes dotnet#3953
@JeremyKuhne JeremyKuhne added the Servicing-consider .NET Shiproom label indicating a PR seeks to enter into a branch under Tell-Mode criteria label Sep 19, 2020
@JeremyKuhne JeremyKuhne added this to the 5.0 RC2 milestone Sep 19, 2020
@JeremyKuhne JeremyKuhne requested a review from a team as a code owner September 19, 2020 01:33
@ghost ghost assigned JeremyKuhne Sep 19, 2020
@codecov
Copy link

codecov bot commented Sep 19, 2020

Codecov Report

Merging #3956 into release/5.0-rc2 will decrease coverage by 31.09625%.
The diff coverage is 60.00000%.

@@                    Coverage Diff                     @@
##           release/5.0-rc2       #3956          +/-   ##
==========================================================
- Coverage         67.64830%   36.55205%   -31.09626%     
==========================================================
  Files                 1410         921         -489     
  Lines               507284      250949      -256335     
  Branches             41180       36855        -4325     
==========================================================
- Hits                343169       91727      -251442     
+ Misses              158124      153961        -4163     
+ Partials              5991        5261         -730     
Flag Coverage Δ
#Debug 36.55205% <60.00000%> (-31.09626%) ⬇️
#production 36.55205% <60.00000%> (+0.01131%) ⬆️
#test ?

Flags with carried forward coverage won't be shown. Click here to find out more.

@RussKie RussKie added Servicing-approved .NET Shiproom approved the PR for merge Servicing-consider .NET Shiproom label indicating a PR seeks to enter into a branch under Tell-Mode criteria and removed Servicing-consider .NET Shiproom label indicating a PR seeks to enter into a branch under Tell-Mode criteria Servicing-approved .NET Shiproom approved the PR for merge labels Sep 21, 2020
@RussKie RussKie added Servicing-approved .NET Shiproom approved the PR for merge and removed Servicing-consider .NET Shiproom label indicating a PR seeks to enter into a branch under Tell-Mode criteria labels Sep 23, 2020
@RussKie RussKie changed the title Rendering fix port Fix ControlPaint regressions Sep 23, 2020
@RussKie RussKie added the 📬 waiting-for-testing The PR is awaiting manual testing by the primary team; no action is yet required from the author(s) label Sep 23, 2020
@RussKie RussKie merged commit 1d168d4 into dotnet:release/5.0-rc2 Sep 23, 2020
@RussKie RussKie removed the 📬 waiting-for-testing The PR is awaiting manual testing by the primary team; no action is yet required from the author(s) label Sep 28, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Jan 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Servicing-approved .NET Shiproom approved the PR for merge
Projects
None yet
2 participants