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

[AnnotationEditorLayerBuilder] Inline the destroy code in the cancel method #15800

Conversation

Snuffleupagus
Copy link
Collaborator

It doesn't seem necessary to have a separate destroy method given that the cancel method always invokes it unconditionally. In the PDFPageView.reset method we currently attempt to call destroy directly, however that'll never actually happen since either:

  • We're keeping the annotationEditorLayer, in which case we're just hiding the layer and nothing more (and the relevant branch is never entered).
  • We're removing the annotationEditorLayer, in which case the PDFPageView.cancelRendering method has already cancelled and nulled it (and there's thus nothing left to destroy here).

Please note: Hopefully I'm not overlooking something obvious here, since both reading through the code and also adding console.log(this.annotationEditorLayer); before this line suggests that it's indeed unnecessary.

…el` method

It doesn't seem necessary to have a *separate* `destroy` method given that the `cancel` method always invokes it unconditionally.
In the `PDFPageView.reset` method we currently attempt to call `destroy` directly, however that'll never actually happen since either:
 - We're keeping the annotationEditorLayer, in which case we're just hiding the layer and nothing more (and the relevant branch is never entered).
 - We're removing the annotationEditorLayer, in which case the `PDFPageView.cancelRendering` method has already cancelled *and* nulled it (and there's thus nothing left to `destroy` here).

*Please note:* Hopefully I'm not overlooking something obvious here, since both reading through the code *and* also adding `console.log(this.annotationEditorLayer);` [before this line](https://github.com/mozilla/pdf.js/blob/9d4aadbf7a32c8a994818e848fb2ec883f257702/web/pdf_page_view.js#L438) suggests that it's indeed unnecessary.
@Snuffleupagus
Copy link
Collaborator Author

/botio-linux preview

@pdfjsbot
Copy link

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

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/8591ce8c8b006a8/output.txt

Total script time: 1.26 mins

Published

@Snuffleupagus
Copy link
Collaborator Author

/botio integrationtest

@pdfjsbot
Copy link

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

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

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

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

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/928ce68b6cdb992/output.txt

Total script time: 4.21 mins

  • Integration Tests: Passed

@pdfjsbot
Copy link

From: Bot.io (Windows)


Failed

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

Total script time: 25.37 mins

  • Integration Tests: FAILED

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.

LGTM.

@Snuffleupagus Snuffleupagus merged commit 879a743 into mozilla:master Dec 10, 2022
@Snuffleupagus Snuffleupagus deleted the AnnotationEditorLayerBuilder-destroy branch December 10, 2022 14:02
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.

None yet

3 participants