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 a setting so providers can process images when no commands are supplied. #83

Merged
merged 3 commits into from
Jan 20, 2020
Merged

Add a setting so providers can process images when no commands are supplied. #83

merged 3 commits into from
Jan 20, 2020

Conversation

Pandorax100
Copy link
Contributor

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 matches 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

Addresses #77

This exposes a setting on individual providers to determine whether they should continue processing an image when no commands are supplied. This is important for remote providers - such as the AzureBlobStorageImageProvider - as they are unable to fall back on the static files middleware to serve images.

…cted

This allows providers to determine whether they should continue processing an image when no commands are supplied. This is important for remote providers - such as the azure blob provider - as they are unable to fall back on the static files middleware to serve images.
@claassistantio
Copy link

claassistantio commented Jan 15, 2020

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@JimBobSquarePants JimBobSquarePants left a comment

Choose a reason for hiding this comment

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

@Pandorax100 Thanks for this, much appreciated!

I think if we make a couple of minor changes this will be good to go.

src/ImageSharp.Web/Providers/IImageProvider.cs Outdated Show resolved Hide resolved
@Pandorax100
Copy link
Contributor Author

I completely agree. The interface has had the setter removed, and the setting has been changed to an enum.

@deanmarcussen
Copy link
Collaborator

This looks good.

My only caveat (just from working around this issue) is that when an image comes in with no commands and is supported as ProcessingBehaviour.All the image will still get loaded by ImageSharp and saved back out as a different stream/image.

So it just means that the image will always be mutated in some way (which has an impact on memory / performance)

It would be nice if when there are no commands the image wasn't mutated, but I leave that choice with @JimBobSquarePants

@JimBobSquarePants
Copy link
Member

@deanmarcussen makes a valid point above

@Pandorax100 We're still fully loading and saving the image when there are no commands.
We still need to identify the format so when there are no commands we should use the Image.Identify(stream, out IImageFormat format) method to get the correct mimetype.

I'm happy to accept this as-is and do the work though. I'm currently updating the build process and changing the targets to sensible ones (Core 3.1, Core 2.1) so don't want to keep you waiting longer than you need to.

@Pandorax100
Copy link
Contributor Author

Yeah, I see the problem

I've just had a little look and it doesn't seem like Image.Identify(stream, out IImageFormat format) is available with the version of ImageSharp referenced by this project. I see that there is Image.DetectFormat(inStream) but if you're in the process of updating the build process etc then I think it's probably best to accept this as is and we can come back to it at a later date. If that's alright with you?

@JimBobSquarePants
Copy link
Member

Totally fine. Will get this merged ASAP.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants