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 sgi Previews #37758

Merged
merged 1 commit into from
Aug 5, 2020
Merged

Add sgi Previews #37758

merged 1 commit into from
Aug 5, 2020

Conversation

micbar
Copy link
Contributor

@micbar micbar commented Jul 31, 2020

Description

This is adding support for SGI images
https://en.wikipedia.org/wiki/Silicon_Graphics_Image

The new supported file extensions are

  • .sgi or .rgb
    3 colour channels
  • .rgba
    3 colour cha-nnels and alpha
  • .bw or .int
    black and white
  • .inta
    black and white and alpha

These files will be associated with the mimetype image/sgi.
This mimetype is not officially registered. https://www.iana.org/assignments/media-types/media-types.xhtml

Dependency

  • This needs the imagick php extension to be installed.

Related Issue

https://github.com/owncloud/enterprise/issues/4131

Motivation and Context

How Has This Been Tested?

  • Manual Test using demo files
  • Manual Test in the details view
  • Manual Test with files_mediaviewer

Demo Files

RGB.zip

Screenshot File List

Screenshot_2020-07-31 RGB - Files - ownCloud

Screenshot Details View

Screenshot_2020-07-31 Files - ownCloud

Screenshot Mediaviewer

Screenshot_2020-07-31 RGB - Files - ownCloud(1)

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:

@mmattel
Copy link
Contributor

mmattel commented Jul 31, 2020

Maybe to add, This format is supported by ImageMagick, see Wikipedia (https://en.wikipedia.org/wiki/Silicon_Graphics_Image)

Please add a changelog 😅

@micbar
Copy link
Contributor Author

micbar commented Jul 31, 2020

@mmattel Raised a docs ticket owncloud/docs#2735

Copy link
Contributor

@ChrisEdS ChrisEdS left a comment

Choose a reason for hiding this comment

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

Works as expected. <3

@mmattel
Copy link
Contributor

mmattel commented Jul 31, 2020

Will this go when merged into 10.5 or in a subsequent release ?

@micbar
Copy link
Contributor Author

micbar commented Jul 31, 2020

Will this go when merged into 10.5 or in a subsequent release ?

10.5.0 is closed.

@micbar micbar requested a review from jvillafanez July 31, 2020 11:49
@owncloud owncloud deleted a comment from update-docs bot Jul 31, 2020
@codecov
Copy link

codecov bot commented Jul 31, 2020

Codecov Report

Merging #37758 into master will decrease coverage by 0.00%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #37758      +/-   ##
============================================
- Coverage     64.71%   64.71%   -0.01%     
- Complexity    19387    19388       +1     
============================================
  Files          1283     1284       +1     
  Lines         75737    75739       +2     
  Branches       1333     1333              
============================================
  Hits          49013    49013              
- Misses        26332    26334       +2     
  Partials        392      392              
Flag Coverage Δ Complexity Δ
#javascript 54.03% <ø> (ø) 0.00 <ø> (ø)
#phpunit 65.89% <0.00%> (-0.01%) 19388.00 <1.00> (+1.00) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ Complexity Δ
lib/private/Preview/SGI.php 0.00% <0.00%> (ø) 1.00 <1.00> (?)
lib/private/PreviewManager.php 71.85% <ø> (ø) 50.00 <0.00> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7a7d638...3e1e919. Read the comment docs.

1 similar comment
@codecov
Copy link

codecov bot commented Jul 31, 2020

Codecov Report

Merging #37758 into master will decrease coverage by 0.00%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #37758      +/-   ##
============================================
- Coverage     64.71%   64.71%   -0.01%     
- Complexity    19387    19388       +1     
============================================
  Files          1283     1284       +1     
  Lines         75737    75739       +2     
  Branches       1333     1333              
============================================
  Hits          49013    49013              
- Misses        26332    26334       +2     
  Partials        392      392              
Flag Coverage Δ Complexity Δ
#javascript 54.03% <ø> (ø) 0.00 <ø> (ø)
#phpunit 65.89% <0.00%> (-0.01%) 19388.00 <1.00> (+1.00) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ Complexity Δ
lib/private/Preview/SGI.php 0.00% <0.00%> (ø) 1.00 <1.00> (?)
lib/private/PreviewManager.php 71.85% <ø> (ø) 50.00 <0.00> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7a7d638...3e1e919. Read the comment docs.

@micbar
Copy link
Contributor Author

micbar commented Jul 31, 2020

@DeepDiver1975 I am not sure about that mimetypes.

Changed it.

@micbar micbar force-pushed the add-sgi-previews branch 2 times, most recently from 925db6f to 3e1e919 Compare July 31, 2020 13:26
@micbar
Copy link
Contributor Author

micbar commented Jul 31, 2020

Reduced it again to the simple set.

@mmattel
Copy link
Contributor

mmattel commented Aug 5, 2020

Are there tests needed ? or merging without (already approved...)

@micbar
Copy link
Contributor Author

micbar commented Aug 5, 2020

basic unit tests are already done for the parent class

@mmattel
Copy link
Contributor

mmattel commented Aug 5, 2020

Maybe asking @phil-davis for help?
I can not assists, tests are my weak point, sorry.

@micbar
Copy link
Contributor Author

micbar commented Aug 5, 2020

I can also write tests 😄 , IMO the existing tests are enough.

This pr is ready to merge

@phil-davis
Copy link
Contributor

We do not currently have specific automated end-to-end acceptance tests for preview generation of each of the supported file types. There are just "generic" test scenarios for "ordinary" text files.

Making such tests in a useful way (i.e. that know what preview result to really expect for each file type) would need to be a separate issue that can be done according to priority.

@micbar
Copy link
Contributor Author

micbar commented Aug 5, 2020

ok, merging now.

@micbar micbar closed this Aug 5, 2020
@micbar micbar reopened this Aug 5, 2020
@micbar micbar merged commit afa3fe3 into master Aug 5, 2020
@delete-merged-branch delete-merged-branch bot deleted the add-sgi-previews branch August 5, 2020 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants