Rotate functionality

[tomasz-grobelny]

Adds functionality to rotate images. Next in queue is functionality to add rating, favourite, tags, flip h/v, etc. Please let me know if anything needs to be fixed before this can be merged.

[tomasz-grobelny]

Any comments on the functionality/code? Of course I am willing to fix the tests if you think overall the functionality is ok.

[MorrisJobke]

@tomasz-grobelny As a general rule we try to only self-assign on tickets. Mentioning people should be enough.

[tomasz-grobelny]

@MorrisJobke, I don't see a lot of activity from @oparoz in nextcloud/gallery recently. I have added @juliushaertl as he committed to this repo relatively recently. Still, I think there is a general problem with getting timely feedback about proposed changes to nextcloud codebase...

[juliushaertl]

Thanks for your pull request and sorry I took a while to have a look at this @tomasz-grobelny I really appreciate your contribution, but I have some concerns regarding how this is implemented.

Some bigger issue is that the rotation won't be reflected in the original images. Therefore previews won't be rotated as well as opening the image on other plattforms.

I would actually prefer to have the modification stored right in the file by setting the exif orientation. We of course would need to check if the previews are properly generated with this information.

For other modifications we need to think more about how we want to store and distribute modifications or metadata (like rating).

Regarding the user interface we should probably hide modification actions in a 3 dots menu, since the list of icons in the slideshow is already becoming quite huge:

cc @nextcloud/designers @nextcloud/gallery also for the bigger vision of the gallery app in the future.

[jancborchardt]

Regarding the user interface we should probably hide modification actions in a 3 dots menu, since the list of icons in the slideshow is already becoming quite huge:

Yep, also ref Standardized file/image/document viewer https://github.com/nextcloud/server/issues/12382.

---

Also, I’d say before we pile up more functionality into Gallery, we should update it to Vue.js as early as possible, otherwise we need to just port more afterwards. What do you think?

[tomasz-grobelny]

Thanks for your pull request and sorry I took a while to have a look at this @tomasz-grobelny I really appreciate your contribution, but I have some concerns regarding how this is implemented.

Ok. So I see two concerns here:

1. UI - IMO a minor issue. For me we can have it in submenu. We can get back to it after we sort out the remaining issues.
2. Storage. For me there are two possible approaches:

1. As it is implemented now - store in application specific table. The benefits are: performance, file is not modified so everything is easily reversible, easy to store any modification. Drawbacks: not reflected on previews or when file is downloaded.
1. Direct file modification - for rotation we can store it in file EXIF data, but crop might not be so easy. All in all benefits are: previews are ok, all changes are reflected in on clients and when downloaded. Drawbacks: performance (file needs to be rewritten, all previews need to be regenerated), operation modifies file metadata such as modification time, operation may not be easily reversible (consider crop functionality).

I just mention that because storing the modifications in file should not be viewed as the "obviously best" option.

I can imagine that we could have a mix of both worlds - file modifications could be stored in table as it is now but then have a way to write all modification that can be written from this table to files (eg. Gwenview uses such idea). What do you think?

[tomasz-grobelny]

Regarding the user interface we should probably hide modification actions in a 3 dots menu, since the list of icons in the slideshow is already becoming quite huge:

Yep, also ref Standardized file/image/document viewer nextcloud/server#12382.

I think we should make a clear distinction here: this functionality is all about file editing pictures (specific file type), not about viewing generic files. In my option these are two very distinct use cases. IMO a generic file viewer might be a completely new application, while gallery could get basic picture editing functionality.

[jancborchardt]

IMO a generic file viewer might be a completely new application, while gallery could get basic picture editing functionality.

The image viewer and editor should be one and the same thing. At least for simple stuff like rotation there’s no reason to separate this. We need to keep it simple.

That currently the image viewer is linked to the Gallery app is just implementation detail.

[tomasz-grobelny]

IMO a generic file viewer might be a completely new application, while gallery could get basic picture editing functionality.

The image viewer and editor should be one and the same thing. At least for simple stuff like rotation there’s no reason to separate this. We need to keep it simple.

Yes, image viewer and editor should be the same thing. My point is that generic document viewer should be something different so we shouldn't consider it in this context.

[jancborchardt]

Ah sorry – I was just referring to the issue because as @juliushaertl said:

Regarding the user interface we should probably hide modification actions in a 3 dots menu, since the list of icons in the slideshow is already becoming quite huge:

as this is also talked about in the issue.

So before we add any more interface elements, the file actions need to go into a menu. Documenation at https://docs.nextcloud.com/server/15/developer_manual/design/popovermenu.html

[tomasz-grobelny]

I can imagine that we could have a mix of both worlds - file modifications could be stored in table as it is now but then have a way to write all modification that can be written from this table to files (eg. Gwenview uses such idea). What do you think?

@nextcloud/gallery, @juliushaertl - do you have any thoughts on this? Are you ok with the mixed approach (temp storage in table as currently + ability to write to files as a separate operation)?

[stefan-niedermann]

i don't like the idea. if one is able to rotate imaged from within the app, the image should be rotatet in files, too. A seperate table creates inconsistences between the different apps / sync clients.

[tomasz-grobelny]

i don't like the idea. if one is able to rotate imaged from within the app, the image should be rotatet in files, too. A seperate table creates inconsistences between the different apps / sync clients.

Understood. Do I understand correctly that you are ok with drawbacks of the other approach (mainly performance - eg. regenerating previews when not needed (applying a tag does not change preview), multiple modifications for one picture)? Or do you propose some other approach?

If you are ok with the drawbacks, I will proceed to implement the direct file modification approach.

[tomasz-grobelny]

If you are ok with the drawbacks, I will proceed to implement the direct file modification approach.

Now, given the emoji approval I started having a look at how can metadata manipulation be done. And it seems that PHP is not the language of choice for people writing dedicated tools/libs. Options I found to get it done:

1. Exiftool - very popular tool with wide image format support and r/w capabilities regarding metadata (Exif, XMP, etc.). The drawback is that it requires Perl and is a command line tool, not a library (there is a C++ "library" that wraps command line tool).
2. PHP library PEL - pros: native PHP library, cons: very limited support for file formats (jpg+tiff), does not seem to support XMP (likely needed for tags, description, favourites, rating, etc.), not actively developed (one commit in 2018).
3. There is a library/command line tool called Exiv2 which supports multiple metadata formats, multiple file formats, is actively developed (several commits just yesterday). The drawback is that it does not have PHP interface (forget about https://github.com/joelalejandro/php-exiv2/ - it is just a quick hack).

My suggestion would be to either:

1. use Exiv2 command line tool. Pros: easy to grasp and implement, simple to deploy. Cons: I don't like the idea of running command line tools from web for security considerations (our code + Exiv2 code which is quite vast and has history of security issues).
2. expose the Exiv2 functionality we need through REST API in C++ and wrap it all in a docker container. Pros: well specified interface, all image processing code is inside a container so we limit attack surface, gives us more flexibility in the future (ability to use other C/C++ libraries, eg. opencv - how about face detection on photos?), we already depend on other services for some nextcloud functionality (consider editing office files). Deployment is not necessarily any more complicated - you just need to set up a docker container from repository and point nextcloud/gallery to it. Cons: more effort needed to implement (but that's on me anyway).

@juliushaertl, @stefan-niedermann, @nextcloud/gallery - any thoughts on the above? Any other option that comes to your mind? My preferred option would be dockerized Exiv2 with REST interface.

[jancborchardt]

Just for info: It would probably be best if e.g. @rullzer @MorrisJobke @skjnldsv @juliushaertl have a look at the proposals before you dive into an implementation which might not work out with possible architectural plans. (And I guess some of them are on vacation at the moment.)

[tomasz-grobelny]

Just for info: It would probably be best if e.g. @rullzer @MorrisJobke @skjnldsv @juliushaertl have a look at the proposals before you dive into an implementation which might not work out with possible architectural plans. (And I guess some of them are on vacation at the moment.)

Hope you had great vacation :-) Any thoughts on the proposed design?

[stratege1401]

Hello to all.

Wondering if beta tester are needed ?

[nickvergessen]

The gallery app has been replaced by the beautiful new app: Nextcloud Photos - :camera_flash: Your memories under your control

Please checkout if your Pull request is still necessary there, and in case create it there or raise an issue for others to copy the change from here.

[geiseri]

For those of us on the stable channel, how long will this wait? Are there hopes to allow us to use this "beautiful new app"?

[nickvergessen]

No, it is available in 18 already, check the stable18 branch