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

Introduce new Reviewer zoom mode for images #2591

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

glutanimate
Copy link
Contributor

@glutanimate glutanimate commented Jul 25, 2023

Inspired by #2588, but hoping to address image zoom in a more general capacity as I think this has been a long-standing issue for quite a few users (even predating native zoom, see e.g. the reviews on zoom add-ons from as far back as 2.0).

In its current iteration, this PR introduces a new JS-based zoom system that works in parallel to the Qt-native system. When the Reviewer is active, any user-triggered zoom actions can be switched over to this alternate mode by holding down Shift. In alternate mode, zooming is performed by scale-transforming document.body, preserving the layout sizing and thus allowing users to freely zoom into images. Quick comparison between the mode (native first, JS second):

reviewer-zoom-rework.mp4

The downside to this approach, and why I ended up implementing it as an optional mode, is that text does not reflow when the layout sizing is preserved, so long-ish paragraphs will end up extending past the viewport. This makes the mode ill-suited for use cases where users will want to adjust the zoom level in order to boost text readability – which I assume are quite common, e.g. when using Anki at larger screen distances.

I played around with this a lot, going back-and-forth between different approaches, and wasn't able to find a full-page zoom mode that works well for both scenarios – thus the two modes, even if not particularly ideal from a discoverability standpoint.

On the upside, this PR also introduces a couple of zoom features in the alternate mode which I think are quite neat to have:

  • the zoom level is now communicated via a small info box in the top right
  • zoom levels in alternate mode are persisted across Anki sessions – not sure if this is 100% desirable for everyone, could switch it back to just being persistent within a session

Some thoughts on alternatives

If we are happy to handle this on a less generic level, then I think it could be interesting to take @krmanik's approach and extend it to all images, i.e. see if we can't equip all imgs with a ctrl+scroll handler that would allow users to zoom into individual images. The problem I would see here is that we don't have full control over user-created note types, or at least not as much as we do with a default note type like IO, and so we could run into scenarios where card layout or styling interferes with image zooming or creates unexpected behavior of some sort.

Introduces a custom JS-based zoom system for the reviewer that enables zooming into viewport-constrained images.

Additionally adds zoom persistence across Anki sessions.

Fixes Image occlusion doesn't scale when page zoomed in ankitects#2588

(but also addresses general image zoom issues across all note types)
Preset factors follow example of Chromium implementation
While scale-transforming the document body works well for images, it does not cover text use cases well: Scale-transform zooming preserves the document layout and thus increases font sizes without triggering text reflow. This makes it ill-suited for common use cases where users might want to boost the readability of a card at larger distances.

With this commit both zoom modes are now active in parallel, with users being able to trigger the scale-transform mode for any existing zoom action by holding down Ctrl+Shift.
@glutanimate
Copy link
Contributor Author

glutanimate commented Jul 25, 2023

One idea on how we could tackle the discoverability issue slightly: Instead of having two menu options that act differently depending on whether Shift is depressed, we could add another set for the new zoom mode and relabel the old ones. I.e., I would imagine the menu to look something like this in that case:

Increase text size (Ctrl++)
Decrease text size (Ctrl+-)
Zoom in (Ctrl+Shift++)
Zoom out (Ctrl+Shift+-)
Reset zoom (Ctrl+0)

This is somewhat (though not fully) similar to how e.g. VS code allows you to individually adjust the interface zoom level (Ctrl & +) vs text zoom level (Ctrl & scroll wheel).

@krmanik
Copy link
Contributor

krmanik commented Jul 26, 2023

I have tested this PR and it works as expected but we should have similar zoom option as before so user will not be confused with new changes to zoom option. Also I think we should switch the Ctrl+Shift+(+/-) for text and Ctrl+(+/-) for window zoom.

@dae
Copy link
Member

dae commented Jul 27, 2023

Initially thought it was broken, then realised I needed to be holding down ctrl as well. :-)

The scrolling in and out and the feedback on zoom level seems to work well.

For reference, AnkiMobile has separate options for zooming (pinch) and text size as well, with some differences:

  • Zooming state is not persisted, and resets when the user answers the card. My thinking here was that the user is probably zooming into an image where the text is hard to read, and may not want all images shown that big. Zooming out also ensures text is not cut off on the left and right of the screen, so it seemed like a better default. WDYT?
  • Text size adjustments work by dynamically rewriting all font references on the page to be relative (in rems), and then adjusting the body font. This causes the text to scale, while leaving images untouched (they don't confusingly shrink when zooming in due to margins).

Only the text size is displayed as a menu option in the UI; zooming is only accessible via pinch or double-tap.

Not suggesting we just copy the AnkiMobile approach here, but throwing it out as a possible alternative for discussion.

Regarding shortcut keys, we held off on adding them in the past because they don't work in some keyboard layouts, and were conflicting with the editor: #1668 (comment)

@dae
Copy link
Member

dae commented Aug 9, 2023

Here we have a user who doesn't want the zoom to change as they go through cards: https://forums.ankiweb.net/t/image-occlusion-improvement-on-anki-mobile/32908

@krmanik
Copy link
Contributor

krmanik commented Aug 23, 2023

@glutanimate
If you are busy then I should complete this PR?

@dae
Copy link
Member

dae commented Apr 10, 2024

@glutanimate ping

@glutanimate
Copy link
Contributor Author

@dae I should have some time to revisit this over the next couple of weeks, thanks for the slight nudge :)

@dae
Copy link
Member

dae commented Oct 2, 2024

Another gentle nudge. No judgement - people in glass houses... ;-)

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.

3 participants