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 heic image support #485

Closed
wants to merge 126 commits into from
Closed

Add heic image support #485

wants to merge 126 commits into from

Conversation

steiny2k
Copy link
Contributor

This is to add the viewer support for heic images. Given the backend is able to provide previews (jpegs) of the heic file through ImageMagick and libheif, this will enable the in-browser viewing of the files.

This addresses #4

Signed-off-by: Sebastian Steinmetz [email protected]

Signed-off-by: Sebastian Steinmetz <[email protected]>
@skjnldsv skjnldsv added 3. to review Waiting for reviews enhancement New feature or request labels Apr 29, 2020
@skjnldsv skjnldsv self-requested a review April 29, 2020 08:38
@skjnldsv
Copy link
Member

Hey!
Thanks for this! This would be an incomplete support though, right?
Heic can contains a video inside or various other images, which won't be displayed here?

@steiny2k
Copy link
Contributor Author

steiny2k commented Apr 29, 2020

This would be an incomplete support though, right?

Exactly. It relies on ImageMagick and the core preview handling for doing the heavy lifting.

Heic can contains a video inside or various other images, which won't be displayed here?

Technically speaking you are right. As the examples by nokiatech show, there are a lot of things a HEIF container may contain: https://nokiatech.github.io/heif/examples.html.
However, my use-case (and the use-case of most of the people using nextcloud I would guess) is photos being shot with an Apple iPhone (since iPhone 7). And for that showing the jpeg preview would be good enough I would say.
If a user wants to see all the sub-images of the file it is necessary to open the file with a suitable piece of software (Apple Photos app could do it).

Of course, long term goal could be to use the JavaScript based heif decoder (https://github.com/nokiatech/heif/wiki/IV.-HEIF-Reader-JavaScript-Implementation) to show directly the heif files. But I don't know if this would be a better user experience in the end. And to be honest, I think it should be the operating systems' or the browsers' duty to support these new image file types natively. So we wouldn't have to worry about any of this.

@skjnldsv
Copy link
Member

And to be honest, I think it should be the operating systems' or the browsers' duty to support these new image file types natively. So we wouldn't have to worry about any of this.

I concurr!

@jancborchardt you ok with this?

nextcloud-bot and others added 10 commits April 30, 2020 03:08
@jancborchardt
Copy link
Member

However, my use-case (and the use-case of most of the people using nextcloud I would guess) is photos being shot with an Apple iPhone (since iPhone 7). And for that showing the jpeg preview would be good enough I would say.

Agree with this, yes. This is a good improvement which will already improve the experience a lot for most people using that format.

nickvergessen and others added 2 commits May 7, 2020 08:00
Signed-off-by: Joas Schilling <[email protected]>
Signed-off-by: npmbuildbot[bot] <npmbuildbot[bot]@users.noreply.github.com>
@steiny2k
Copy link
Contributor Author

steiny2k commented May 7, 2020

Okay, given the input from Jan, how do we proceed? I see several of the checks didn't pass. Could someone point me in the direction, what I have to adjust in order provide a mergeable pull request?

@skjnldsv
Copy link
Member

skjnldsv commented May 8, 2020

Okay, given the input from Jan, how do we proceed? I see several of the checks didn't pass. Could someone point me in the direction, what I have to adjust in order provide a mergeable pull request?

Just rebase and we'll compile the files again :)
It's just because other changes were merged.
You can rebase to master and drop all the /js changes

skjnldsv and others added 19 commits August 2, 2020 14:15
Props of type array need a factory function for their defaults.

Otherwise when reusing the component
all might end up with a shared default array.

Signed-off-by: Azul <[email protected]>
Signed-off-by: npmbuildbot[bot] <npmbuildbot[bot]@users.noreply.github.com>
…rn/babel/core-7.11.1

Bump @babel/core from 7.11.0 to 7.11.1
Signed-off-by: Morris Jobke <[email protected]>
Bumps [@nextcloud/vue](https://github.com/nextcloud/nextcloud-vue) from 2.3.0 to 2.6.0.
- [Release notes](https://github.com/nextcloud/nextcloud-vue/releases)
- [Commits](nextcloud-libraries/nextcloud-vue@v2.3.0...v2.6.0)

Signed-off-by: dependabot-preview[bot] <[email protected]>
Signed-off-by: npmbuildbot[bot] <npmbuildbot[bot]@users.noreply.github.com>
…rn/nextcloud/vue-2.6.0

Bump @nextcloud/vue from 2.3.0 to 2.6.0
…rn/vue-async-computed-3.9.0

Bump vue-async-computed from 3.8.3 to 3.9.0
@skjnldsv
Copy link
Member

@steiny2k please rebase :)

dependabot-preview bot and others added 5 commits August 22, 2020 07:04
Bumps [vue](https://github.com/vuejs/vue) and [vue-template-compiler](https://github.com/vuejs/vue). These dependencies needed to be updated together.

Updates `vue` from 2.6.11 to 2.6.12
- [Release notes](https://github.com/vuejs/vue/releases)
- [Commits](vuejs/vue@v2.6.11...v2.6.12)

Updates `vue-template-compiler` from 2.6.11 to 2.6.12
- [Release notes](https://github.com/vuejs/vue/releases)
- [Commits](vuejs/vue@v2.6.11...v2.6.12)

Signed-off-by: dependabot-preview[bot] <[email protected]>
Signed-off-by: npmbuildbot[bot] <npmbuildbot[bot]@users.noreply.github.com>
Signed-off-by: Sebastian Steinmetz <[email protected]>
Signed-off-by: Sebastian Steinmetz <[email protected]>
@steiny2k
Copy link
Contributor Author

I'm not so sure, I should just merge this here, since I think I did something wrong with the rebasing. Github mentions a lot of files which would have been changed. So I've created a new PR #577. If thinks it's cleaner to merge that one.

@steiny2k steiny2k mentioned this pull request Aug 23, 2020
@skjnldsv
Copy link
Member

I'm not so sure, I should just merge this here, since I think I did something wrong with the rebasing. Github mentions a lot of files which would have been changed. So I've created a new PR #577. If thinks it's cleaner to merge that one.

What command did you run?

@MorrisJobke
Copy link
Member

Let's close this here in favor of #577

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants