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

feature(thumbnails): add the ability to define custom image processors #7409

Merged
merged 5 commits into from
Oct 17, 2023

Conversation

fschade
Copy link
Contributor

@fschade fschade commented Oct 4, 2023

Description

Thumbnails can now be changed during creation, previously the images were always scaled to fit the given frame,
but it could happen that the images were cut off because they could not be placed better due to the aspect ratio.

This pr introduces the possibility of specifying how the behavior should be, following processors are available

  • resize
  • fit
  • fill
  • thumbnail

the processor can be applied by adding the processor query param to the request, e.g. processor=fit, processor=fill, ...

to find out more how the individual processors work please read https://github.com/disintegration/imaging

if no processor is provided it behaves the same as before (resize for gif's and thumbnail for all other)

needs web changes too, next step....

Related Issue

Motivation and Context

web preview app is not just in use to preview images, but also to actually consume the content, therefor it is needed that images are un-cropped for that, in the file-list for example it is fine to have cropping in place.

How Has This Been Tested?

  • unit tests
  • manual with postman

Screenshots (if appropriate):

without processor:
Screenshot 2023-10-04 at 16 34 15

with fit processor:
Screenshot 2023-10-04 at 16 34 41

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

could need some docs magic cc.: @mmattel

@update-docs
Copy link

update-docs bot commented Oct 4, 2023

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@fschade fschade self-assigned this Oct 4, 2023
@fschade fschade added the Category:Enhancement Add new functionality label Oct 4, 2023
@fschade fschade force-pushed the image-processor branch 2 times, most recently from c59d8b1 to 148ff7a Compare October 4, 2023 14:40
@fschade fschade marked this pull request as ready for review October 4, 2023 14:43
@mmattel
Copy link
Contributor

mmattel commented Oct 4, 2023

@fschade pls add your description to the readme of the thumbnail service, thx

Co-authored-by: Martin <github@diemattels.at>
@mmattel
Copy link
Contributor

mmattel commented Oct 16, 2023

LGTM from a docs pov 👍

@sonarcloud
Copy link

sonarcloud bot commented Oct 16, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug C 2 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

60.6% 60.6% Coverage
1.7% 1.7% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@fschade fschade enabled auto-merge (squash) October 16, 2023 11:08
Returned: 15x10
Requested: 18x12
Available: 30x20, 15x10, 9x6
Returned: 15x10
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does it return the next-smaller instead of the next-bigger resolution? Would look nicer if clients down-scale a bit instead of up-scale?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure, i have to have a look into the codebase, to be honest, i do not know every thumbnail service detail. please create a issue if we should change something.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, I’ll play with it and I’ll open new issues if needed.

* `fill`
* `thumbnail`

To apply one of those, a query parameter has to be added to the request, e.g. `?processor=fit`
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the default behavior in case this parameter isn't provided?

Copy link
Contributor

Choose a reason for hiding this comment

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

If I recall correctly, this is optional, means behaviour equals before if not added.
Maybe to write: Image generation can optionally be configured...
But @fschade can clarify best.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if no processor is provided it uses the default behaviour as before (Resize fort gif, Thumbnail for everything else)

@michaelstingl
Copy link
Contributor

oC10 parameters scalingup=0 (no-upscale) and a=1 (no-crop) still supported? Also add them to the services/thumbnails/README.md?

@fschade
Copy link
Contributor Author

fschade commented Oct 16, 2023

@michaelstingl scalingup has no effect if i remember correctly, a is new to me, i can add a fallback if you want, but please let me add it in a separate pr.

@michaelstingl
Copy link
Contributor

Okay, I’ll play with it and I’ll open new issues if needed.

@fschade fschade merged commit 9abcd8a into owncloud:master Oct 17, 2023
2 of 3 checks passed
ownclouders pushed a commit that referenced this pull request Oct 17, 2023
#7409)

* feature(thumbnails): add the ability to define custom image processors

* fix(ci): add exported member comment

* docs(thumbnails): mention processors in readme

* fix: codacy and code review feedback

* fix: thumbnail readme markdown

Co-authored-by: Martin <github@diemattels.at>

---------

Co-authored-by: Martin <github@diemattels.at>
mmattel added a commit that referenced this pull request Nov 3, 2023
References: #7409 (feature(thumbnails): add the ability to define custom image processors)
The `Thumbnail Processor` section needed an update to clarify the usecase and behaviour. In addition, the list of sizes above now shows as in a single line each and not in one line.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category:Enhancement Add new functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add no-crop thumbnails rendering in images preview
4 participants