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

Lightbox pan, zoom and display mode options #1708

Merged
merged 35 commits into from
Oct 1, 2021

Conversation

WithoutPants
Copy link
Collaborator

@WithoutPants WithoutPants commented Sep 8, 2021

Moves the slideshow delay options into a popover overlay always. Adds a display mode dropdown to allow fit to screen (default), fit horizontally, and original size.

I'm looking to add scroll to zoom and drag to scroll functionality.

Resolves #1492
Resolves #1151

@WithoutPants WithoutPants added feature Pull requests that add a new feature bounty This issue has a bounty on it in the OpenCollective labels Sep 8, 2021
@WithoutPants WithoutPants added this to the Version 0.10.0 milestone Sep 8, 2021
@WithoutPants WithoutPants marked this pull request as draft September 8, 2021 03:30
@WithoutPants
Copy link
Collaborator Author

This is now ready for testing.

The lightbox code has been extensively refactored. Includes the following changes:

  • can now zoom in and out by hovering the mouse over the image and scrolling the mouse wheel
  • zoom can be reset to its original state by clicking on the button that appears when scrolling
  • image can be panned by dragging the mouse over the image
  • touch gestures to pan and pinch to zoom are also added. Note that this changes the previous behaviour that moving between images by swiping. This is replaced with tapping on the image.
  • slideshow delay options have been moved permanently into an options popover. This popover also includes options to set the default zoom - fit to screen (default), fit horizontally, and original image size

@WithoutPants WithoutPants marked this pull request as ready for review September 9, 2021 04:05
@WithoutPants WithoutPants changed the title [WIP] Lightbox zoom and display mode options Lightbox pan, zoom and display mode options Sep 9, 2021
@ALonelyJuicebox
Copy link

First, love the work that's been done on this feature request, thank you! 🙏🏾

  • Testing the Display Modes
    One interesting bug I've encountered.
    For both the Fit to Screen mode as well as Fit Horizontally mode, images of a certain smaller size will not expand to fit the boundaries of the viewport. There's some magical ratio between the size of the view and the size of the image that causes this.
    What you end up seeing is just the image as though the mode was set to "original".

  • Mouse Scroll behavior
    Scroll to zoom is a great addition here!
    The only downside I see is that because zoom is controlled by the scroll wheel, the user is unable to scroll down in order to see the remainder of the image. This is reasonable for images of people (for example), but the click-and-hold-to-scroll is poor UX for desktop users who might be using this functionality to read a comic (again, as an example).
    Just a suggestion for what I would hope might be an easy fix solution, maybe a toggle button in the Options popup to disable scroll to zoom and instead allow the scrollbar to function as normal if the image exceeds the height/width of the view?

  • Next/Previous Image location logic
    This one is a little tricky. Is there some currently defined logic around where on the image the user will be sent to when the "next" button is clicked?
    For example, if the image is larger than the view, and display mode is set to anything but "fit to screen", is there intent around where on the image the user will be sent when they click Next?
    Observing the current behavior, it appears that if the image is taller than the view, the next image will be loaded dead center (vertically). Further, it also appears there's some location...caching as well? Ex. If I go back and then forward, it will recall exactly where I was on that first image.
    Mimicking how all the major image viewers (that I'm aware of) behave by default, ideally I'd like to request that the user be brought to the top of the image when the next button is clicked as it would make the browsing experience feel a bit more standardized, and it would address a usability issue for those using this feature for reading comics.

@ALonelyJuicebox
Copy link

- Fullscreen testing

Minor UX thing, but we should probably hide the gear icon when in full screen as clicking the button does not do anything (nor are any of the options applicable when in full screen)

@WithoutPants
Copy link
Collaborator Author

  • Testing the Display Modes
    One interesting bug I've encountered.
    For both the Fit to Screen mode as well as Fit Horizontally mode, images of a certain smaller size will not expand to fit the boundaries of the viewport. There's some magical ratio between the size of the view and the size of the image that causes this.
    What you end up seeing is just the image as though the mode was set to "original".

If I understand this correctly, you're expecting images smaller than the resolution of the viewport to expand to fit the viewport? That would be the opposite to what I would expect. The two fit options imo should be used to constrain the maximum size of the image; it shouldn't zoom in smaller images. I'd want to hear a few more opinions before I commit to changing this behaviour.

If I'm assuming incorrectly, then let me know what image size/viewport resolution you see the problem behaviour with.

  • Mouse Scroll behavior
    Scroll to zoom is a great addition here!
    The only downside I see is that because zoom is controlled by the scroll wheel, the user is unable to scroll down in order to see the remainder of the image. This is reasonable for images of people (for example), but the click-and-hold-to-scroll is poor UX for desktop users who might be using this functionality to read a comic (again, as an example).
    Just a suggestion for what I would hope might be an easy fix solution, maybe a toggle button in the Options popup to disable scroll to zoom and instead allow the scrollbar to function as normal if the image exceeds the height/width of the view?

Added an option for scroll mode - either zoom (default) or pan Y - with the option to temporarily switch modes while holding shift.

  • Next/Previous Image location logic
    This one is a little tricky. Is there some currently defined logic around where on the image the user will be sent to when the "next" button is clicked?
    For example, if the image is larger than the view, and display mode is set to anything but "fit to screen", is there intent around where on the image the user will be sent when they click Next?
    Observing the current behavior, it appears that if the image is taller than the view, the next image will be loaded dead center (vertically). Further, it also appears there's some location...caching as well? Ex. If I go back and then forward, it will recall exactly where I was on that first image.
    Mimicking how all the major image viewers (that I'm aware of) behave by default, ideally I'd like to request that the user be brought to the top of the image when the next button is clicked as it would make the browsing experience feel a bit more standardized, and it would address a usability issue for those using this feature for reading comics.

Fixed so that position and zoom are reset when navigating between images, and images are now shown from the top of the image for the "original" and "fit horizontally" display modes.

- Fullscreen testing

Minor UX thing, but we should probably hide the gear icon when in full screen as clicking the button does not do anything (nor are any of the options applicable when in full screen)

Fixed so that gear icon is not shown in fullscreen mode.

@gitgiggety
Copy link
Contributor

The two fit options imo should be used to constrain the maximum size of the image; it shouldn't zoom in smaller images. I'd want to hear a few more opinions before I commit to changing this behaviour.

I tend to agree. "Fit" should make the image fit in the viewport, but shouldn't stretch smaller images to "fill" the viewport. Also presuming you can still zoom in which will make the image bigger / cover a bigger area of the screen space. Such that if you do have a smaller image you can still zoom in after which it will fill the entire viewport.

Added an option for scroll mode - either zoom (default) or pan Y - with the option to temporarily switch modes while holding shift.

That sounds great. Also because of the option to temporarily switch by holding shift.

Fixed so that gear icon is not shown in fullscreen mode.

Previously did a quick test (so didn't retest now). But at least on mobile when I opened the settings / gear and then went fullscreen the settings "popup" / "dropdown" was still being shown. Did you happen to fix that as well? As it was a bit weird that the settings popup was shown, while the toolbar (with the gear, play button, ..) was hidden.

@ALonelyJuicebox
Copy link

Fixed so that position and zoom are reset when navigating between images, and images are now shown from the top of the image for the "original" and "fit horizontally" display modes.

Fantastic improvements! The auto scroll to top feature works great, love the pan feature, and very clever with the shift shortcut.
I'm curious-- Was resetting the zoom depth on navigation click a requirement in order to get the next image to be positioned from the top? My bias towards using these lightbox improvements for comics is showing again but if it is reasonably feasible to make it such that the zoom does not reset but the scroll to top is maintained through something toggleable that'd be fantastic.

I can imagine that for standard photos of people, resetting the zoom makes tons of sense and is far less jarring.

If I understand this correctly, you're expecting images smaller than the resolution of the viewport to expand to fit the viewport? That would be the opposite to what I would expect. The two fit options imo should be used to constrain the maximum size of the image; it shouldn't zoom in smaller images. I'd want to hear a few more opinions before I commit to changing this behaviour.

Totally logical question! I am indeed asking for exactly as you described. :)
Instead of debating what the behaviour of "Fit to Screen" should be, can I request that we just add an additional item to the dropdown?
Irfanview (wildly popular image viewer) sets good precedence for this. Using Irfanview's own scaling option verbiage here:

  • Original Size
  • Fit to Screen - Large Images only
  • Fit to Screen - All Images
  • Fit image width to screen width

The third option (Fit to Screen - All Images) would just be a version of "Fit to Screen - Large Images Only" but one where smaller images are expanded to the viewport, maintaining aspect ratio.

For some additional examples, the Adobe suite of products (Lightroom, Photoshop, etc) all share the same UX as well.
In those programs, "Fit to screen" zooms (for smaller images) or contracts (larger images) such that the entire image is fully visible in the viewport.

@SuperEllipsis
Copy link

I do not know if it has been noted yet, but there seems to be a bug where going back to the previous page in the wall mode fails to return to the last image from the previous page. It goes directly to index 0 on the previous page. It happens for both Chrome and Firefox.

@WithoutPants
Copy link
Collaborator Author

I've made the behaviour that resets the zoom during navigation an option, enabled by default. When disabled, it maintains the current zoom level and resets the position to the top of the image.

@SuperEllipsis
Copy link

SuperEllipsis commented Sep 21, 2021

hmm, strange.
For me it occurs in all scenarioes where I have to go back one page in the Wall mode. Fx selecting the last image on a page and then going back (Clicking on image or using arrow keys), I get different behaviour clicking the small arrows on the left and right of my screen (sometimes works correct, sometimes does not).

  • I have tried to create a clean install on my computer in folders with different permissions and a clean install on my raspberry pi.
    • Windows 10
    • WSL 2.0 (Ubuntu)
    • Raspberry pi 4
  • I have also tried to disable all browser plugins, and used both Firefox and Chrome plus Safari on IOS.
  • Each time I have created a new database and config file
    • Containing one local image folder and one network drive image folder
  • Below is a simple video of one of the tests (using a gallery of space)
    https://user-images.githubusercontent.com/82902785/134211228-258d1cf8-b46d-4e6c-811e-06692d47ade6.mp4

I get the same behaviour across all devices and browsers. What confuses me, is that I have an existing instance of stash on my raspberry pi 4 (v0.9.0-20-g565064b4, hash: 565064b) where it behaves as expected.

@ALonelyJuicebox
Copy link

ALonelyJuicebox commented Sep 21, 2021

@WithoutPants The new toggle works great with no issues, thank you!

As for the bug, @SuperEllipsis scratch what I said earlier, I wasn't going through the steps properly, that video helped and is exactly what I'm seeing as well. I can replicate as well on Windows 10 + Google Chrome, Firefox and Microsoft Edge.

EDIT: I think I narrowed it down a bit. This only occurs when clicking on the image itself to navigate rather than when using the navigation arrows on the sides, a feature added in this PR. This bug also doesn't seem to present itself in the current 0.9 build of Stash.

@SuperEllipsis
Copy link

SuperEllipsis commented Sep 21, 2021

For me it (index becomes 0) happens both when clicking the image, but also when using the arrow keys. Also though it might work sometimes with the navigation arrows, my experience is that it differs from gallery to gallery (It seems to go to the index on the previous page corresponding to the number of images on the next page, if that makes sense).

Example:

  • Page 1 has 20 images
  • Page 2 has 7 images
  • Click on the first image on the second page
  • Go one image backwards (using the navigation arrows)
  • End up at image index 7 on the previous page

To reproduce:

  • Open a gallery
  • Set a number of images per page such that the last page is less than full
  • Click on the first image on the last page
  • Click the left navigation arrow
  • see the incorrect image index

Can I ask you @ALonelyJuicebox to try the steps above (just to verify)

@WithoutPants
Copy link
Collaborator Author

Thanks for the reports. I've hopefully fixed the issues traversing pages.

@SuperEllipsis
Copy link

SuperEllipsis commented Sep 22, 2021

I have done a bunch of tests and it seems like the backwards navigation now works as expected, thank you. I did however stumble upon another bug, also regarding navigation, but not introduces in this RP (As it appers in atleast 0.7, 0.8, and 0.9 - In my testing). This bug is about page overflow on gallery pages.

How to reproduce

  1. Open a gallery
  2. Set the gallery to Wall mode
  3. Navigate to the last page of the gallery (Note: Select the last page in the page selector above the images)
  4. Open the last image on the last page
  5. Navigate one image forwards
  6. See that you are now on page (0 / [number of pages]) and that the shown image is the first image on the first page

Note it seems to work as expected (most of the time) if you start on (open) any image on the first page and then navigate all the way to the end (with the lightbox viewer open).

@WithoutPants
Copy link
Collaborator Author

I think the intent of the original code was to only wrap around when the slideshow was running. I've changed it so that it always wraps around and in both directions (ie previous on first image/page goes to last image/page), and I've fixed the page display as reported.

@SuperEllipsis
Copy link

I really like the idea that it wraps around from both ends of the Wall mode 👍 and it works well for all but one case, as far as I can tell.

If there is only a single page, then it simply shows a loading animation, but it wont wrap to the last image of the same page or the first image (depending on the direction). This scenario can be tested by opening any gallery and setting the number of images to be equal to or more than the total number of images (Such that there is only one page). Then trying to wrap around either ends of the page.

Aside from that I really like the improvements in this PR.

@WithoutPants
Copy link
Collaborator Author

Good catch. I also noticed an issue where if the last page only has a single image, then the nav buttons would not be shown. Should be fixed now.

@SuperEllipsis
Copy link

SuperEllipsis commented Sep 27, 2021

Navigation between pages and images works perfectly. I decided to do a bit of testing with the pan/zoom and display modes.
They work very well. There is only one rather funny bug (I assume it is a bug), with the zoom functionality and wall mode.
If you open an image and zoom in rather far, the previous and next images also zoom, and can be visible in the current viewport (The previous image goes behind the current, while the next goes above the current). Which with a combination of wide and tall images means one can pull the other images into the frame. In the attached picture below, it was possible for me to have three images on one screen by pulling the other two images into the current viewport.

billede

@WithoutPants
Copy link
Collaborator Author

  • Fixed an issue where if you dragged outside the window, subsequent dragging would zoom the image
  • Made option dialog close when clicking outside
  • Changed so that zoom is only applied to the current image, so it should no longer be possible to see multple images at once

@WithoutPants WithoutPants merged commit e348053 into stashapp:develop Oct 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bounty This issue has a bounty on it in the OpenCollective feature Pull requests that add a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] Image/Gallery Wall View - Scaling & Zoom Options [Feature] Multi touch zoom for lightbox images
4 participants