viewer icon indicating copy to clipboard operation
viewer copied to clipboard

Pinch to zoom

Open starypatyk opened this issue 2 years ago • 4 comments

Here is my initial pinch-to-zoom implementation. This should resolve #916 and #917.

I have not committed the compiled code yet, to make rebasing easier.

Some explanation of the changes:

In https://github.com/nextcloud/viewer/pull/1952/commits/003f31fe70550da3c8046e80124784a425335b4c I have switched from mouse events to more generic pointer events (see https://developer.mozilla.org/en-US/docs/Web/API/Pointer_events).

https://github.com/nextcloud/viewer/pull/1952/commits/e96662cc299e6d665b7b7d7112ee98fbe744b649 is the actual implementation.

  • I have re-used the original zoom handling code - a significant part of the old updateZoom method was extracted to updateZoomAndShift and I created an additional common method updateShift.
  • Co-ordinates of the two touches are kept in pointerCache.
  • Pinch-zoom calculations are explained in the comments.

I committed https://github.com/nextcloud/viewer/pull/1952/commits/aeab2bd8f159fae5553150b293d2c12fa7f353ca separately, to make it easier to follow the changes.

https://github.com/nextcloud/viewer/pull/1952/commits/1f0fcedc9dfa1ecc86a3ab59418c1e151425a1b7 was required after recent changes in master - as I removed the width/height: 100% from CSS.

I have the following questions:

  • Regarding https://github.com/nextcloud/viewer/pull/1952/commits/1f0fcedc9dfa1ecc86a3ab59418c1e151425a1b7 - an alternative solution would be to bring back width/height: 100%. I am not sure which one is better. Any suggestions?
  • The pointerMove and pointerUp handlers are now attached all the time. I am not sure what should be done here: https://github.com/nextcloud/viewer/blob/698afa42df0c8976e797a68dec8c8ee8f2dc9c89/src/components/Images.vue#L219 and here: https://github.com/nextcloud/viewer/blob/698afa42df0c8976e797a68dec8c8ee8f2dc9c89/src/components/Images.vue#L223 Probably also a permanent handler for pointerOut that would invoke resetZoom? I do not know how to test this case. Suggestions welcome.
  • I have tested on Android and iPad. Would be good to verify on an iPhone.
  • I have noticed that recent changes to NcModal (https://github.com/nextcloud-libraries/nextcloud-vue/pull/4350) affect zooming behaviour. Now, when the image is enlarged a bit from 100%, a scrollbar is shown, and this causes a slight shift in image position. This happens on Chrome and Chrome Mobile (for some reason Firefox does not show the scrollbar). This is not a major problem, but is ~~quite~~ a bit annoying. I wonder if we should try to have some workaround in this PR or leave it for a separate PR.

starypatyk avatar Aug 28 '23 21:08 starypatyk

Hello there, Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the reviewing process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR reviewing process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

github-actions[bot] avatar Sep 12 '23 01:09 github-actions[bot]

@starypatyk I don't have much time for Viewer lately, thank you for this PR (and your patience :see_no_evil:)

skjnldsv avatar Sep 19 '23 08:09 skjnldsv

@skjnldsv No worries. :wink: Thanks for letting me know. Waiting patiently then. :snail:

starypatyk avatar Sep 19 '23 20:09 starypatyk

@starypatyk I don't have much time for Viewer lately, thank you for this PR (and your patience 🙈)

Hi @skjnldsv,

Any chance to go forward with this PR?

FYI - I have rebased and re-checked the implementation. Seems to behave decently. Tested as previously on Android and iPad.

This still requires some clean-up - see my questions in the first comment (I have updated commit references after rebase in the original comment).

starypatyk avatar Mar 07 '24 23:03 starypatyk

/compile /

skjnldsv avatar Aug 22 '24 07:08 skjnldsv

Patience came to an end, let get this in! Sorry again @starypatyk!

Btw, if you're interested, I sketched an RFC for an improved Viewer version 3.0. If you have some commens or feedback, feel free to check #2395 :tada:

skjnldsv avatar Aug 22 '24 07:08 skjnldsv

cypress fix: https://github.com/nextcloud/viewer/pull/2436

skjnldsv avatar Aug 22 '24 11:08 skjnldsv

Fixing the public view rendering :)

skjnldsv avatar Aug 22 '24 13:08 skjnldsv

@skjnldsv - Great to see this merged finally! :wink: Thanks :+1:

starypatyk avatar Aug 22 '24 21:08 starypatyk