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

JS - Implement few possibilities with app.execMenuItem (bug 1724399) #14038

Merged
merged 1 commit into from
Sep 18, 2021

Conversation

calixteman
Copy link
Contributor

Can be tested with https://github.com/mozilla/pdf.js/blob/master/test/pdfs/evaljs.pdf
Enter app.execMenuItem("SaveAs") in the first field and then click on the button Execute.

@calixteman calixteman changed the title JS - Implement few possibilities with app.execMenuItem JS - Implement few possibilities with app.execMenuItem (bug 1724399) Sep 17, 2021
this._eventBus.dispatch("save", { source: this });
break;
case "FirstPage":
this._eventBus.dispatch("firstpage", { source: this });
Copy link
Collaborator

Choose a reason for hiding this comment

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

Events should only be used for things that the BaseViewer doesn't directly support, and which require the full viewer (such as e.g. saving).

For everything else, please use direct this._pdfViewer calls wherever possible to avoid breaking the standalone viewer components unnecessarily; note https://github.com/mozilla/pdf.js/tree/master/examples/components

Hence, for the FirstPage, LastPage, NextPage, and PrevPage event specifically: please revert to the previous version of this PR (with my two comments addressed).

Copy link
Collaborator

@Snuffleupagus Snuffleupagus left a comment

Choose a reason for hiding this comment

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

r=me with the final comments addressed; thank you!

Really sorry about the back-and-forth here!

case "PrevPage":
this._pdfViewer.previousPage();
break;
case "ZoomViewIn":
Copy link
Collaborator

Choose a reason for hiding this comment

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

Similar to the "zoom"-case in the existing code, please add the following check to prevent issues:

if (isInPresentationMode) {
  return;
}

case "ZoomViewIn":
this._eventBus.dispatch("zoomin", { source: this });
break;
case "ZoomViewOut":
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above.

Comment on lines 483 to 499
if (
[
"SaveAs",
"FirstPage",
"LastPage",
"NextPage",
"PrevPage",
"ZoomViewIn",
"ZoomViewOut",
].includes(item)
) {
this._send({ command: item });
return;
}

switch (item) {
case "FitPage":
this._send({ command: "zoom", value: "page-fit" });
break;
case "Print":
this._send({ command: "print" });
break;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Now that the switch is added, can't you simply handle all of the cases within that block instead?

    switch (item) {
      case "SaveAs":
      case "FirstPage":
      case "LastPage":
      case "NextPage":
      case "PrevPage":
      case "ZoomViewIn":
      case "ZoomViewOut":
        this._send({ command: item });
        break;

      case "FitPage":
        this._send({ command: "zoom", value: "page-fit" });
        break;
      case "Print":
        this._send({ command: "print" });
        break;
    }

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.

3 participants