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

Add PngWriteDefines to Magick.NET #1661

Merged
merged 38 commits into from
Jul 16, 2024
Merged

Conversation

rubenscordeirobr
Copy link
Contributor

Description

This pull request introduces the PngWriteDefines class to the Magick.NET project. The PngWriteDefines class includes several enums (PngChunkFlags, PngCompressionFilter, PngCompressionStrategy),

Changes Proposed:

Added the PngWriteDefines class with the following properties:

BitDepth - Specifies the bit depth for the PNG image.
ColorType - Specifies the color type for the PNG image.
CompressionFilter - Specifies the compression filter for the PNG image using the PngCompressionFilter enum.
CompressionLevel - Specifies the compression level for the PNG image.
CompressionStrategy - Specifies the compression strategy for the PNG image using the PngCompressionStrategy enum.
ExcludeChunks - Specifies the chunks to be excluded using the PngChunkFlags enum.
IncludeChunks - Specifies the chunks to be included using the PngChunkFlags enum.
IgnoreCrc - Indicates whether the PNG decoder should ignore the CRC.
PreserveICCP - Indicates whether to preserve the ICC profile.
PreserveColormap - Indicates whether to preserve the colormap.

@rubenscordeirobr rubenscordeirobr changed the title Add PngWriteDefines to Magick.NET for PNG Chunk Management #225 Add PngWriteDefines to Magick.NET #225 Jun 27, 2024
@rubenscordeirobr rubenscordeirobr changed the title Add PngWriteDefines to Magick.NET #225 Add PngWriteDefines to Magick.NET Jun 27, 2024
…arnings

- Updated XML comments for PreserveColorMap property to resolve SA1623.
- Removed an empty line to resolve SA1507.
- Added copyright notice to the header that was previously missing.
- Added `#if NETSTANDARD == false` to hide `[Flags]` attribute for non-.NET Standard frameworks.
   This resolves build failures on Windows ARM64 processors.
- Added justification to SuppressMessage in PngChunkFlags
"The lowercase names are consistent with the naming conventions of PNG chunk types as defined in the PNG specification."
- Removed bitwise shift operators and used explicit int values for flag definitions.
- Resolved StyleCop error SA1137: Elements should have the same indentation.
Copy link
Owner

@dlemstra dlemstra left a comment

Choose a reason for hiding this comment

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

Thanks for helping me with this. I left some review comments.

  - Alphabetical ordering for CompressionLevel and CompressionFilter.
  - Converted CompressionFilter and CompressionStrategy to numeric values.
- This option is only used when reading a PNG.
- Removed TheIncludeChunksProperty class from PngWriteDefinesTests
…roperty.

- Updated the class name to follow the naming convention for property test classes.
…nNull PngWriteDefinesTests.

  - TheBitDepthProperty
  - TheColorTypeProperty
  - TheCompressionFilterProperty
  - TheExcludeChunksProperty
  - TheCompressionStrategyProperty
- Updated tests to use numeric values for properties in
  - TheColorTypeProperty
  - TheCompressionFilterProperty
  - ThreCompressionStrategyProperty
@rubenscordeirobr
Copy link
Contributor Author

Please let me know if there's anything else to do before merging this pull request, and I'll adjust it as necessary.

@dlemstra
Copy link
Owner

dlemstra commented Jul 3, 2024

I thought this was still work in progress because PngColorType is internal an not used?

@rubenscordeirobr
Copy link
Contributor Author

In the PngWriteDefines class, I noticed that the property ColorType is of the enum type ImageMagick.ColorType. However, I observed a difference between the numeric values of ImageMagick.ColorType and the required values for png:color-type. For example, ImageMagick.ColorType.Grayscale equals 2, but png:color-type requires a value of 0 for Grayscale.

To handle this, I created a PngColorType enum to correctly map and convert these values before sending them to ImageMagick. Although this solution works, I believe it might be more efficient to change the type of the ColorType property directly to PngColorType for better clarity and accuracy.

Please let me know your thoughts on this change.

Copy link
Owner

@dlemstra dlemstra left a comment

Choose a reason for hiding this comment

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

We are getting close. Left some new comments and also resolved the ones that you already resolved. There was one comment that you did not resolve yet.

- Corrected the class name from `ThreCompressionStrategyProperty` to `TheCompressionStrategyProperty`
…removed due to a misunderstanding.

- Revert "Removed IncludeChunks property in PngWriteDefines."
This reverts commit 2584741ac.
- Replaced the PngColorType enumeration with direct integer values for better performance.
- ShouldThrowExceptionWhenColorTypeIsOptimize to check for unsupported ColorType.Optimize.
- ShouldThrowExceptionWhenColorTypeIsInvalid to check for unsupported integer values as ColorType.
Copy link
Owner

@dlemstra dlemstra left a comment

Choose a reason for hiding this comment

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

Thanks for all your help! I will publish a new release that will include these changes this week.

@dlemstra dlemstra merged commit 31acab3 into dlemstra:main Jul 16, 2024
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants