X Tutup
Skip to content

fix: max image dimensions are now configurable and have a higher default#41193

Merged
jnweiger merged 1 commit intomasterfrom
fix/configurable-max-image-dimensions
Feb 23, 2024
Merged

fix: max image dimensions are now configurable and have a higher default#41193
jnweiger merged 1 commit intomasterfrom
fix/configurable-max-image-dimensions

Conversation

@DeepDiver1975
Copy link
Member

Description

Related Issue

Motivation and Context

How Has This Been Tested?

  • test environment:
  • test case 1:
  • test case 2:
  • ...

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • 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:
  • Changelog item, see TEMPLATE

@update-docs
Copy link

update-docs bot commented Feb 23, 2024

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.

@DeepDiver1975 DeepDiver1975 force-pushed the fix/configurable-max-image-dimensions branch from b880035 to c806846 Compare February 23, 2024 09:22
@jnweiger jnweiger self-requested a review February 23, 2024 09:48
Copy link
Contributor

@jnweiger jnweiger left a comment

Choose a reason for hiding this comment

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

We need to clarify the relation between
preview_max_dimensions and preview_max_x/preview_max_y

Sorry, I did not spot the latter pair earlier.

* Default is 6016x4000.
*/
'preview_max_dimensions' => '6016x4000',

Copy link
Contributor

Choose a reason for hiding this comment

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

A bit further up, we already have this:

/**
 * Define the maximum x-axis width for previews
 * The maximum width, in pixels, of a preview.
 * A value of `null` means there is no limit.
 */
'preview_max_x' => 2048,

/**
 * Define the maximum y-axis width for previews
 * The maximum height, in pixels, of a preview. A value of `null` means there is no limit.
 */
'preview_max_y' => 2048,

That is confusing now. Those values are used in apps/files_sharing/lib/Controllers/ShareController.php and lib/private/Preview.php

Copy link
Member Author

Choose a reason for hiding this comment

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

These are output values.
And the new prop defined input values.

But I see your point...... Let me think....

@DeepDiver1975 DeepDiver1975 force-pushed the fix/configurable-max-image-dimensions branch from c806846 to d05c3c9 Compare February 23, 2024 10:39
$maxDimension = \OC::$server->getConfig()->getSystemValue('preview_max_dimensions', '6016x4000');
$exploded = explode('x', strtolower($maxDimension));
if ($exploded === false || \count($exploded) !== 2) {
return [6016, 4000];
Copy link
Contributor

Choose a reason for hiding this comment

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

Code could be refactored so that the default [6016, 4000] only appears once, but up to you.

Copy link
Contributor

@jnweiger jnweiger left a comment

Choose a reason for hiding this comment

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

Ah, thanks for clarification.

@DeepDiver1975 DeepDiver1975 force-pushed the fix/configurable-max-image-dimensions branch from 11f853f to 8f6a51c Compare February 23, 2024 11:26
@sonarqubecloud
Copy link

@jnweiger jnweiger merged commit 536f0b4 into master Feb 23, 2024
@delete-merged-branch delete-merged-branch bot deleted the fix/configurable-max-image-dimensions branch February 23, 2024 12:30
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.

[QA] typical Photos from a smartphone are no longer rendered in the builtin image viewer

3 participants

X Tutup