-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Optimize icon rendering: simplify logic and eliminate redundant calculations #13449
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
base: main
Are you sure you want to change the base?
Conversation
…lations - Replaced HDC with Graphics in DrawIcon() for smoother GDI+ integration - Optimized Rectangle management – removed unnecessary variables, directly adjusting sizes - Simplified stretch logic (Inflate) – replaced complex calculations with a concise method call - Unified Draw() and DrawUnstretched() – now both directly call DrawIcon() without intermediate processing - Eliminated duplicate Matrix.Offset calculations, ensuring targetRect is adjusted directly - No functional changes, just cleaner, more maintainable code 🚀
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #13449 +/- ##
====================================================
+ Coverage 62.21083% 77.15788% +14.94705%
====================================================
Files 3213 3279 +66
Lines 638302 645264 +6962
Branches 47201 47716 +515
====================================================
+ Hits 397093 497872 +100779
+ Misses 234153 143696 -90457
+ Partials 7056 3696 -3360
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
@JeremyKuhne can you please take a look at this PR? |
|
Looking, also letting CoPilot have a look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors Icon.DrawIcon to use Graphics instead of raw HDC, simplifies rectangle handling, and unifies the two Draw methods.
- Replaced HDC-based drawing with
GraphicsandDeviceContextHdcScope - Collapsed manual width/height calculations into
Rectangleoperations (Inflate,Offset,Intersect) - Streamlined
DrawandDrawUnstretchedto forward directly toDrawIcon
Reviewed Changes
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/System.Drawing.Common/src/System/Drawing/Icon.cs | Refactored DrawIcon signature/logic, simplified rectangle math |
| src/System.Drawing.Common/src/System/Drawing/Icon.cs (Draw overloads) | Removed manual transform offset and copy, now calls DrawIcon directly |
JeremyKuhne
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love the cleanup here! If you can do this in steps where you move the calculations into a private helper first and create some good unit tests, I'd be happy to work with you to take this. (We're extremely resource bound for a while.)
Once the helper methods are collapsed to a single line there is no need to keep them.
Ultimately this helper code should really live in the Graphics class. It could also be further optimized by avoiding creating the Matrix object by using TransformElements.
|
@JeremyKuhne I also wanted to do the same changes in the |
|
@DJm00n Presumably by putting the code in |
Co-authored-by: Copilot <[email protected]>
Regression?
Microsoft Reviewers: Open in CodeFlow