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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix DrawImage offsetting issues and improve API parameter names. #2474

Merged
merged 4 commits into from Jun 12, 2023

Conversation

JimBobSquarePants
Copy link
Member

Prerequisites

  • I have written a descriptive pull-request title
  • I have verified that there are no overlapping pull-requests open
  • I have verified that I am following the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 馃懏.
  • I have provided test coverage for my change (where applicable)

Description

Fixes #2447

In addition, I renamed the parameters and properties in the extension methods and processors to make it obvious what each thing means.

Copy link
Member

@tocsoft tocsoft left a comment

Choose a reason for hiding this comment

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

this is is quite alot of breaking changes..

we have 2 dfferent types of breakign changes happening here, we have the paramater name renames, which is a source breaking change for people using names paramaters.

And then we have the contructor change, which is both a source and binary breaking change.

I am a little less opiniated with regards the source/paramater rename break. i'm not really a fan of breaking them for not a lot of benefit but i can live with them. but we really should not be introducing the binary break.

/// <param name="colorBlendingMode">The blending mode to use when drawing the image.</param>
/// <param name="alphaCompositionMode">The Alpha blending mode to use when drawing the image.</param>
/// <param name="opacity">The opacity of the image to blend.</param>
public DrawImageProcessor(
Image image,
Point location,
Point backgroundLocation,
Copy link
Member

Choose a reason for hiding this comment

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

we should add an overload with out the ForegroundRectangle for back compatability, as this will be an binary breaking change.

Copy link
Member Author

Choose a reason for hiding this comment

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

I always forget these are public.

Copy link
Member

@tocsoft tocsoft left a comment

Choose a reason for hiding this comment

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

Aproving, but leaving my reservation on changing the paramater names, I would rather we didn't change them and only changed the docs/comments but I won't stand in the way of the change

@JimBobSquarePants
Copy link
Member Author

Aproving, but leaving my reservation on changing the paramater names, I would rather we didn't change them and only changed the docs/comments but I won't stand in the way of the change

Thanks, and very much noted. I only changed them because I actually got confused while debugging. I'd like to keep them for v3.1 as I feel they do add benefit.

@JimBobSquarePants JimBobSquarePants merged commit 119885f into main Jun 12, 2023
8 checks passed
@JimBobSquarePants JimBobSquarePants deleted the js/enhanced-drawing branch June 12, 2023 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Drawing part of an image onto another image throws ImageProcessingException or ArgumentOutOfRangeException
2 participants