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

fix(files): conversion api simplification and conflict check #50208

Merged
merged 2 commits into from
Jan 16, 2025

Conversation

skjnldsv
Copy link
Member

@skjnldsv skjnldsv commented Jan 16, 2025

Followup #49922

  • Simplified the Conversion provider supported mime declaration
  • Added extension information
  • Restructured a bit the manager
  • Automatically detect file conflicts and adjust accordingly
  • Tests

Copy link
Member

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

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

Code looks good and works as expected now with and without passing a destination to the OCS endpoint

lib/private/Files/Conversion/ConversionManager.php Outdated Show resolved Hide resolved
lib/private/Files/Conversion/ConversionManager.php Outdated Show resolved Hide resolved
lib/private/Files/Conversion/ConversionManager.php Outdated Show resolved Hide resolved
Comment on lines +24 to +25
* @param string $extension The file extension for the target MIME type (e.g. 'png')
* @param string $displayName The human-readable name of the target MIME type (e.g. 'Image (.png)')
Copy link
Member

Choose a reason for hiding this comment

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

IMO the translations should omit the extension, as we can build this string ourselves using the provided extension.

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought about this, I don't know how flexible we want to make it.
Do we ALWAYS want to show the (.png) part ?

cc @marcoambrosini @nimishavijay
for context, this would be visible in the Save as submenu,
image

Entries would be like this

  • Image (.jpg)
  • Image (.png)
  • Image (.gif)

Maybe we should directly use more human text like this? Then we can omit the extension alltogether?

  • Jpeg image
  • Png image
  • Gif image

Copy link
Member

Choose a reason for hiding this comment

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

True, for example Windows doesn't even display the file extensions anymore, so they could be meaningless to many users.

Copy link
Member

Choose a reason for hiding this comment

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

From my perspective It makes little difference and I'd be fine with both approaches

@skjnldsv skjnldsv force-pushed the feat/conversion-adjusting branch from 3687728 to 85bea72 Compare January 16, 2025 12:59
@skjnldsv
Copy link
Member Author

@provokateurin maybe we can tackle the l10n on a followup once we have an answer from the design team? :)

@skjnldsv skjnldsv force-pushed the feat/conversion-adjusting branch from 85bea72 to fdd2b77 Compare January 16, 2025 13:01
Copy link
Member

@provokateurin provokateurin left a comment

Choose a reason for hiding this comment

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

Code looks good, but unfortunately you need to fix the depencencies 🙈

lib/composer/composer/autoload_psr4.php Outdated Show resolved Hide resolved
lib/private/Files/Conversion/ConversionManager.php Outdated Show resolved Hide resolved
@skjnldsv skjnldsv force-pushed the feat/conversion-adjusting branch from fdd2b77 to 55337af Compare January 16, 2025 13:55
@skjnldsv skjnldsv added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Jan 16, 2025
@skjnldsv skjnldsv force-pushed the feat/conversion-adjusting branch from 4271705 to 27d2fff Compare January 16, 2025 17:16
@skjnldsv skjnldsv mentioned this pull request Jan 16, 2025
@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 2. developing Work in progress labels Jan 16, 2025
@skjnldsv skjnldsv force-pushed the feat/conversion-adjusting branch 2 times, most recently from 1652349 to 8e6c399 Compare January 16, 2025 17:54
@skjnldsv skjnldsv force-pushed the feat/conversion-adjusting branch from 8e6c399 to 19ce362 Compare January 16, 2025 17:55
@skjnldsv skjnldsv merged commit 35db02c into master Jan 16, 2025
190 of 191 checks passed
@skjnldsv skjnldsv deleted the feat/conversion-adjusting branch January 16, 2025 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish bug feature: files technical debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants