search

mozilla / pdf.js

PDF Reader in JavaScript
https://mozilla.github.io/pdf.js/
Apache License 2.0
47.23k stars 9.83k forks source link

[api-minor] Simplify API to implement zoom in custom viewers #18179

Closed nicolo-ribaudo closed 1 month ago

nicolo-ribaudo commented 1 month ago

Closes #18076

This PR extends the zoom/scale API to simplify the logic in custom viewer wrappers:

Note that this PR increases the net number of lines because it adds a test, but ignoring the new test it would be +56 -95.

@Snuffleupagus In https://github.com/mozilla/pdf.js/pull/18098#issuecomment-2112821398 you said that you probably do not want updateScale, but I still uploaded it since it de-duplicates a significant amount of code. What do you think about it? (the second commit could be done independently from the first one)

Commits:

Unify increaseScale/decreaseScale logic as updateScale

updateScale receives a drawingDelay, a scaleFactor and/or a number of steps. If scaleFactor is a positive number different from 1 the current scale is multiplied by that number. Otherwise, if steps if a positive integer the current scale is multiplied by DEFAULT_SCALE_DELTA steps times. Finally, if steps is a negative integer, the current scale is divided by DEFAULT_SCALE_DELTA abs(steps) times.

Add origin parameter to updateScale

This parameter allows defining which point should remain fixed while scaling the document. It can be used, for example, to implement "zoom around the cursor" or "zoom around pinch center".

The logic was previously implemented in web/app.js, but moving it to the viewer scaling utilities themselves makes it easier to implement similar zooming functionalities in other embedders.

Snuffleupagus commented 1 month ago

In #18098 (comment) you said that you probably do not want updateScale, but I still uploaded it since it de-duplicates a significant amount of code. What do you think about it?

Looking at a working patch, written by an actual human, I'm happy to retract that statement since it's clear that it avoids a bunch of code duplication :-)

Snuffleupagus commented 1 month ago

/botio integrationtest

moz-tools-bot commented 1 month ago

From: Bot.io (Linux m4)

Received

Command cmd_integrationtest from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.241.84.105:8877/41d5c1423b09441/output.txt

moz-tools-bot commented 1 month ago

From: Bot.io (Windows)

Received

Command cmd_integrationtest from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.193.163.58:8877/c713b44aab198d2/output.txt

moz-tools-bot commented 1 month ago

From: Bot.io (Linux m4)

Success

Full output at http://54.241.84.105:8877/41d5c1423b09441/output.txt

Total script time: 7.85 mins

moz-tools-bot commented 1 month ago

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/c713b44aab198d2/output.txt

Total script time: 16.62 mins

nicolo-ribaudo commented 1 month ago

This is the test failing on windows (line 727 is throwing): https://github.com/mozilla/pdf.js/blob/95a7de9f986c449cb949d39af948dbe97f60948e/test/integration/stamp_editor_spec.mjs#L713-L730

It is failing because the .move() on line 727 is moving to outside of the viewport. It seems unrelated, because that test doesn't change zoom in any way, however looking at recent CI bot I couldn't find any similar failures 🤔

Snuffleupagus commented 1 month ago

/botio integrationtest

moz-tools-bot commented 1 month ago

From: Bot.io (Linux m4)

Received

Command cmd_integrationtest from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.241.84.105:8877/1a80676073ff22b/output.txt

moz-tools-bot commented 1 month ago

From: Bot.io (Windows)

Received

Command cmd_integrationtest from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.193.163.58:8877/aa3933c8fd7da6b/output.txt

moz-tools-bot commented 1 month ago

From: Bot.io (Linux m4)

Success

Full output at http://54.241.84.105:8877/1a80676073ff22b/output.txt

Total script time: 7.82 mins

moz-tools-bot commented 1 month ago

From: Bot.io (Windows)

Success

Full output at http://54.193.163.58:8877/aa3933c8fd7da6b/output.txt

Total script time: 19.04 mins

Snuffleupagus commented 1 month ago

/botio-linux preview

moz-tools-bot commented 1 month ago

From: Bot.io (Linux m4)

Received

Command cmd_preview from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.241.84.105:8877/55892e089776b04/output.txt

moz-tools-bot commented 1 month ago

From: Bot.io (Linux m4)

Success

Full output at http://54.241.84.105:8877/55892e089776b04/output.txt

Total script time: 1.17 mins

Published

Snuffleupagus commented 1 month ago

/botio integrationtest

moz-tools-bot commented 1 month ago

From: Bot.io (Linux m4)

Received

Command cmd_integrationtest from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.241.84.105:8877/8b3e402acee8dcf/output.txt

moz-tools-bot commented 1 month ago

From: Bot.io (Windows)

Received

Command cmd_integrationtest from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.193.163.58:8877/1724685ff947a26/output.txt

moz-tools-bot commented 1 month ago

From: Bot.io (Linux m4)

Success

Full output at http://54.241.84.105:8877/8b3e402acee8dcf/output.txt

Total script time: 7.75 mins

moz-tools-bot commented 1 month ago

From: Bot.io (Windows)

Success

Full output at http://54.193.163.58:8877/1724685ff947a26/output.txt

Total script time: 18.98 mins

Snuffleupagus commented 1 month ago

/botio-linux integrationtest

moz-tools-bot commented 1 month ago

From: Bot.io (Linux m4)

Received

Command cmd_integrationtest from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.241.84.105:8877/d288c01ae5dfbfa/output.txt

moz-tools-bot commented 1 month ago

From: Bot.io (Linux m4)

Success

Full output at http://54.241.84.105:8877/d288c01ae5dfbfa/output.txt

Total script time: 7.82 mins

nicolo-ribaudo commented 1 month ago

Thanks for the quick review!