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

Print the document after having ran the WillPrint callback #15032

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

calixteman
Copy link
Contributor

  • wait for the WillPrint callback:
    • this one could change the annotationStorage hence it needs
      to be frozen in order to be used later when the page is
      rendered for the printer.
  • update the colors in the annotationStorage and use them when
    saving or printing the document.
  • it fixes the printing of 160F-2019.pdf.

- wait for the WillPrint callback:
  * this one could change the annotationStorage hence it needs
    to be frozen in order to be used later when the page is
    rendered for the printer.
- update the colors in the annotationStorage and use them when
  saving or printing the document.
- it fixes the printing of 160F-2019.pdf.
@Snuffleupagus Snuffleupagus self-requested a review June 12, 2022 18:03
Comment on lines -496 to -499
return (
!this._hasFlag(flags, AnnotationFlag.INVISIBLE) &&
!this._hasFlag(flags, AnnotationFlag.NOVIEW)
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Historically we've had bugs related to Annotation-visiblity and various flags.

Note that this code applies to all Annotations, and the code below (i.e. the one setting e.g. data.display) only applies to WidgetAnnotations, hence please explain why these changes are generally correct/safe!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yes you're right: I was bit FieldAnnotation-focused...

@@ -505,26 +502,19 @@ class Annotation {
_isPrintable(flags) {
return (
this._hasFlag(flags, AnnotationFlag.PRINT) &&
!this._hasFlag(flags, AnnotationFlag.HIDDEN) &&
Copy link
Collaborator

Choose a reason for hiding this comment

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

Historically we've had bugs related to Annotation-visiblity and various flags.

Note that this code applies to all Annotations, and the code below (i.e. the one setting e.g. data.display) only applies to WidgetAnnotations, hence please explain why these changes are generally correct/safe!

if (storageEntry && storageEntry.hidden !== undefined) {
return !storageEntry.hidden;
}
return this.viewable && !this._hasFlag(this.flags, AnnotationFlag.HIDDEN);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Historically we've had bugs related to Annotation-visiblity and various flags.

Note that this code applies to all Annotations, and the code below (i.e. the one setting e.g. data.display) only applies to WidgetAnnotations, hence please explain why these changes are generally correct/safe!

Comment on lines +1439 to +1441
const print = this._hasFlag(data.annotationFlags, AnnotationFlag.PRINT);
const hidden = this._hasFlag(data.annotationFlags, AnnotationFlag.HIDDEN);
const noView = this._hasFlag(data.annotationFlags, AnnotationFlag.NOVIEW);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we not use the shorter this.hasFlag(AnnotationFlag.WHATEVER) format instead here?

const hidden = this._hasFlag(data.annotationFlags, AnnotationFlag.HIDDEN);
const noView = this._hasFlag(data.annotationFlags, AnnotationFlag.NOVIEW);

if (hidden) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where do all these magical values, that are used below, come from?
My guess would be the scripting-API, hence a comment just above here explaining their origin would be a very good idea.

Comment on lines +158 to +163
get frozen() {
if (this.#frozen) {
const results = this.#frozen;
this.#frozen = null;
return results;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

How does this work when printing a multi-page document, since it seems to me that the very first getOperatorlist-call will remove the internal frozen-state?

Generally speaking, I'm sorry to say that I'm also finding this implementation a bit cumbersome and unfortunate.
If you don't mind, I'd really like to experiment a little bit and maybe propose a slightly different solution here (hopefully later today or tomorrow).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please note that I'm not saying that this is perfect by any means, and it feels a bit annoying having to extend the public API, but what about something like the following?
https://github.com/mozilla/pdf.js/compare/master...Snuffleupagus:PrintAnnotationStorage?w=1

Comment on lines +297 to +300
static async dispatchAsyncEventInSandbox(event) {
return FirefoxCom.requestAsync("dispatchAsyncEventInSandbox", event);
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we please just modify the method above as follows instead (and similarly in the web/generic_scripting.js file).

static async dispatchEventInSandbox(event, awaitResponse = false) {
  if (awaitResponse) {
    return FirefoxCom.requestAsync("dispatchAsyncEventInSandbox", event);
  }
  return FirefoxCom.request("dispatchEventInSandbox", event);
}

(Since I somewhat doubt that the run-time overhead will even be measurable, and adding essentially duplicated code never feels great)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants