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

Change the constructor of PixelTypeInfo and ImageMetadata from internal to public so that a user can implement IImageInfo #1929

Merged
merged 4 commits into from
Jan 11, 2022

Conversation

jz5
Copy link
Contributor

@jz5 jz5 commented Jan 8, 2022

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

Case a user implements IImageInfo and supports user-specific image formats:
IImageInfo has PixelTypeInfo and ImageMetadata properties. But

  • ImageMetadata cannot be instanced because it has an internal constructor.
  • PixelTypeInfo cannot be instanced because it has an internal constructor.

So change the constructor of PixelTypeInfo and ImageMetadata from internal to public

Related discussion: #1914

@codecov
Copy link

codecov bot commented Jan 8, 2022

Codecov Report

Merging #1929 (edfc2b2) into master (4904f32) will increase coverage by 0%.
The diff coverage is 100%.

❗ Current head edfc2b2 differs from pull request most recent head b4662e6. Consider uploading reports for the commit b4662e6 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master   #1929    +/-   ##
=======================================
  Coverage      88%     88%            
=======================================
  Files         968     966     -2     
  Lines       51564   51324   -240     
  Branches     6428    6397    -31     
=======================================
- Hits        45403   45192   -211     
+ Misses       5091    5074    -17     
+ Partials     1070    1058    -12     
Flag Coverage Δ
unittests 88% <100%> (+<1%) ⬆️

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

Impacted Files Coverage Δ
...mats/Jpeg/Components/Decoder/HuffmanScanDecoder.cs 93% <100%> (ø)
...rp/Formats/Jpeg/Components/Decoder/HuffmanTable.cs 100% <100%> (ø)
src/ImageSharp/Formats/Jpeg/JpegDecoderCore.cs 89% <100%> (-1%) ⬇️
src/ImageSharp/Formats/PixelTypeInfo.cs 100% <100%> (ø)
src/ImageSharp/Metadata/ImageMetadata.cs 100% <100%> (ø)
src/ImageSharp/Formats/Webp/WebpDecoder.cs 33% <0%> (-21%) ⬇️
src/ImageSharp/Formats/Tiff/TiffDecoder.cs 50% <0%> (-19%) ⬇️
...geSharp/Formats/Tiff/TiffDecoderMetadataCreator.cs 72% <0%> (-2%) ⬇️
...ImageSharp/Formats/Webp/BitWriter/BitWriterBase.cs 91% <0%> (-1%) ⬇️
... and 24 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dc79124...b4662e6. Read the comment docs.

@JimBobSquarePants JimBobSquarePants added this to the 2.0.0 milestone Jan 9, 2022
@@ -16,7 +16,7 @@ public class PixelTypeInfo
/// </summary>
/// <param name="bitsPerPixel">Color depth, in number of bits per pixel.</param>
/// <param name="alpha">Tthe pixel alpha transparency behavior.</param>
internal PixelTypeInfo(int bitsPerPixel, PixelAlphaRepresentation? alpha = null)
public PixelTypeInfo(int bitsPerPixel, PixelAlphaRepresentation? alpha = null)
Copy link
Member

Choose a reason for hiding this comment

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

@antonfirsov I'm wondering whether we should avoid the nullable parameter here and either provide an overload or break things for V2.

I'm not overly concerned about the implications of making this public because I'd like to do a thorough review for V3 to make sure we capture everything we should.

Copy link
Member

Choose a reason for hiding this comment

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

I think this should be refactored into 2 constructors for now. In long term, I would explore init-only setters for such API-s.

Copy link
Member

Choose a reason for hiding this comment

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

As for the V3 API concern: I think minor breaking changes are fine in advanced API-s.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that keeps with our pattern for avoiding nullable parameters when possible. @jz5 Could you please update the PixelTypeInfo constructor to use separate overloads with and without the PixelAlphaRepresentation (not nullable) parameter. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated it. How about this?

jz5 and others added 2 commits January 10, 2022 22:55
Copy link
Member

@antonfirsov antonfirsov left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

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.

3 participants