Skip to content

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
/// <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!

@JimBobSquarePants JimBobSquarePants merged commit d918502 into SixLabors:master Jan 11, 2022
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