Remove ImageFormat out params and return on Metadata #2317
Conversation
JimBobSquarePants
changed the title
JimBobSquarePants
requested review from
tocsoft,
antonfirsov,
brianpopow and
dlemstra
JimBobSquarePants
added
API
codequality
breaking
|
@JimBobSquarePants all good. It's no wasted time. All are learning on this path. |
| stream, | ||
| s => this.Decode<TPixel>(options, s, default)); | ||
|
|
||
| this.SetDecoderFormat(options.Configuration, image); |
There was a problem hiding this comment.
rather than calling this at every overload... could this not just happen inside the Decode() call above? well instead of calling the abstract Decode<>() directly create a DecodeInternal<>() (name?) that calls the abstract method and sets the decoder rather than have to repeat the call on every overload.
There was a problem hiding this comment.
Not that I can see. Each of the sync/async calls are different calling the generic/non-generic versions. The overloads are the abstract methods.
tests/ImageSharp.Tests/TestUtilities/Tests/TestImageProviderTests.cs
Outdated
Show resolved
Hide resolved
tests/ImageSharp.Tests/TestUtilities/Tests/TestImageProviderTests.cs
Outdated
Show resolved
Hide resolved
tests/ImageSharp.Tests/TestUtilities/ReferenceCodecs/MagickReferenceDecoder.cs
Outdated
Show resolved
Hide resolved
src/ImageSharp/IImageInfo.cs
Outdated
| @@ -1,4 +1,4 @@ | |||
| // Copyright (c) Six Labors. | |||
| // Copyright (c) Six Labors. | |||
| // Licensed under the Six Labors Split License. | |||
There was a problem hiding this comment.
I think this Interface and the interface IImage are not longer used in the code. Can probably get dropped
There was a problem hiding this comment.
I've been considering this also.
IImageInfo is used in two places in ImageInfoExtensions to get the size and bounds.
IImage is only referenced by Image and is unused anywhere.
@tocsoft what do you think?
There was a problem hiding this comment.
OK. So, I actually ran with this and in my opinion, things look much better.
IImageInfo and IImage are gone.
ImageInfo is now unsealed and Image inherits that.
Size Size and Rectangle Bounds are now properties of ImageInfo not extension methods which remove the odd inconsistency between those extensions and the Width/Height properties,
There was a problem hiding this comment.
seems fine to me, we seem to be dropping interfaces where we can 👍
There was a problem hiding this comment.
Yeah. I think we went a little heavy on them originally and it actually limited our ability to improve in some areas.
What are your thoughts on the rest of the PR. OK to merge?
…sts.cs Co-authored-by: Scott Williams <[email protected]>
…sts.cs Co-authored-by: Scott Williams <[email protected]>
…erenceDecoder.cs Co-authored-by: Scott Williams <[email protected]>
Prerequisites
Description
This PR replaces and build upon #2092. Touches #2231
ImageInfotype for easier decoder implementation.IImageInfotoImage.MetadataasIImageInfo?via theDecodedImageFormatproperty and set on decode.out IImageFormatparameter and cleanup.DetectFormat,Identify, andLoadAPIs to all throwUnknownImageFormatExceptionon failure.ImageFormatManager.GetXXmethods to throwUnknownImageFormatExceptionon failure.@stefannikolei Apologies but I sent you the wrong direction with your previous PR. It's best we return the none nullable type and throw when something goes wrong with the aforementioned APIs.