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

[api-minor] Re-factor the basic textLayer-functionality #18104

Merged
merged 1 commit into from
May 21, 2024

Conversation

Snuffleupagus
Copy link
Collaborator

@Snuffleupagus Snuffleupagus commented May 16, 2024

This is very old code, and predates e.g. the introduction of JavaScript classes, which creates unnecessarily unwieldy code in the viewer.
By introducing a new TextLayer class in the API, similar to how e.g. the AnnotationLayer looks, we're able to keep most parameters on the class-instance itself. This removes the need to manually track them in the viewer, and simplifies the call-sites.

This also removes the numTextDivs parameter from the "textlayerrendered" event, since that's only added to support default-viewer functionality that no longer exists.

Finally we try, as far as possible, to polyfill the old renderTextLayer and updateTextLayer functions since they are exposed in the library API.
For simple invocations of renderTextLayer the behaviour should thus be the same, with only a warning printed in the console.

@Snuffleupagus Snuffleupagus force-pushed the TextLayer-class branch 8 times, most recently from 39108ff to 5f420d6 Compare May 17, 2024 07:30
@Snuffleupagus Snuffleupagus changed the title [api-major] Re-factor the basic textLayer-functionality [api-minor] Re-factor the basic textLayer-functionality May 17, 2024
@Snuffleupagus Snuffleupagus force-pushed the TextLayer-class branch 6 times, most recently from 434fc90 to 4bf0c98 Compare May 17, 2024 12:15
@Snuffleupagus
Copy link
Collaborator Author

/botio test

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.241.84.105:8877/6f5dff3be3cfc49/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Received

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

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

This is very old code, and predates e.g. the introduction of JavaScript classes, which creates unnecessarily unwieldy code in the viewer.
By introducing a new `TextLayer` class in the API, similar to how e.g. the `AnnotationLayer` looks, we're able to keep most parameters on the class-instance itself. This removes the need to manually track them in the viewer, and simplifies the call-sites.

This also removes the `numTextDivs` parameter from the "textlayerrendered" event, since that's only added to support default-viewer functionality that no longer exists.

Finally we try, as far as possible, to polyfill the old `renderTextLayer` and `updateTextLayer` functions since they are exposed in the library API.
For *simple* invocations of `renderTextLayer` the behaviour should thus be the same, with only a warning printed in the console.
@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Failed

Full output at http://54.241.84.105:8877/6f5dff3be3cfc49/output.txt

Total script time: 27.72 mins

  • Unit tests: Passed
  • Integration Tests: FAILED
  • Regression tests: FAILED
  different ref/snapshot: 18
  different first/second rendering: 2

Image differences available at: http://54.241.84.105:8877/6f5dff3be3cfc49/reftest-analyzer.html#web=eq.log

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Failed

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

Total script time: 43.31 mins

  • Unit tests: Passed
  • Integration Tests: FAILED
  • Regression tests: FAILED
  different ref/snapshot: 2

Image differences available at: http://54.193.163.58:8877/c19c5296ae9d6d4/reftest-analyzer.html#web=eq.log

@Snuffleupagus
Copy link
Collaborator Author

/botio integrationtest

@moz-tools-bot
Copy link
Collaborator

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/addb4a9f667b2f3/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Received

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

Live output at: http://54.193.163.58:8877/71b8377b4bd7745/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Success

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

Total script time: 7.31 mins

  • Integration Tests: Passed

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Success

Full output at http://54.193.163.58:8877/71b8377b4bd7745/output.txt

Total script time: 21.36 mins

  • Integration Tests: Passed

@Snuffleupagus Snuffleupagus marked this pull request as ready for review May 17, 2024 13:23
Copy link
Contributor

@calixteman calixteman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see anything obviously wrong.
Thank you for doing that.
If everything is fine for you, please merge it asap, I'll make a release in m-c after in order to test it irl in nightly.

@Snuffleupagus Snuffleupagus merged commit 9ee7c07 into mozilla:master May 21, 2024
9 checks passed
@Snuffleupagus Snuffleupagus deleted the TextLayer-class branch May 21, 2024 10:28
@timvandermeij timvandermeij removed their request for review May 21, 2024 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants