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

more ViewerPreferences #745

Merged
merged 22 commits into from
Jan 24, 2021
Merged

more ViewerPreferences #745

merged 22 commits into from
Jan 24, 2021

Conversation

mcshaz
Copy link
Contributor

@mcshaz mcshaz commented Jan 11, 2021

address issue #744 and complete the rest of the spec for ViewerPreferences (with the exception of all key-value pairs for which the spec states 'Most conforming readers disregard it.'). includes documentation and unit tests.

potential further tests not included (but I can write if you tell me your thoughts & where you would like them):

  1. Test that during serialization the document catalog is the root entry in the trailer.
  2. Test that serialization and then deserialization retains all the metadata and also viewerPreferences.

Also address #747 (minor). If you want to support back to ES5, I can alter the Rollup workflow (using babel) so that both a modern and ES5 version of the code is produced, but as per the comments in the issue, I think this should actually be the responsibility of the consuming code.

P.S. I tried a rebase but failed - If you would like to merge this code, can you please squash everything to a single commit during the merge- thanks.

@Hopding
Copy link
Owner

Hopding commented Jan 14, 2021

Hello @mcshaz! It would be awesome to have setters and getters for the ViewerPreferences. Thanks for working on this.

Over all the code looks good, but there are some spots that will need to be tweaked to match the patterns & conventions of this codebase. I'll leave some specific review comments in the next few days when I have some time. Though some things I'll just polish up myself after merging.

Regarding the rollup and tsconfig changes: If either of those things need to be changed, that should happen in a dedicated PR. I try not to fix code/features with config and compiler changes when possible. For this PR, please implement the logic under the current rollup and tsconfig setup. Please hold off for now on opening a separate PR for rollup/tsconfig changes, those things can impact consumers of the library in unexpected ways and may result in unintentional breaking changes, so they need to be considered very carefully.

@mcshaz
Copy link
Contributor Author

mcshaz commented Jan 14, 2021

OK I'll remove the target directive - I looked more closely at the TS spec and saw that target might not be a good one to play with as arrow functions will no longer be transpiled to function statements.

However, developing in visual studio code, a significant number of files within this library use functions and objects which are not part of the ES5 spec (listed in #747) and are therefore displayed as warnings within the debugger. What about including a lib directive to ES2016. The lib directive is described at https://www.typescriptlang.org/tsconfig#lib-config - obviously I'll put this as a separate pull request.

ensure it does not show up as a change in the diff
@mcshaz
Copy link
Contributor Author

mcshaz commented Jan 14, 2021

Another question to think about is how to get and set the array pairs to the PrintPageRange. currently it is an array of numbers and the setter checks the array is an even length. Therefore printing pages 1,3 and 5 to 7 would be [1, 1, 3, 3, 5, 7]. However it might be better for readability and also TypeScript type checking to be an array of tuples [number, number][] | [number, number] such that the values associated with the get & set method for the same page range is denoted by [[1, 1], [3, 3], [5, 7]].

Let me know your thoughts.

@Hopding Hopding mentioned this pull request Jan 16, 2021
Copy link
Owner

@Hopding Hopding left a comment

Choose a reason for hiding this comment

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

Thanks again for working on this @mcshaz! I've gone through the PR and made some change requests. Once they're addressed this should be good to merge and release. Let me know if you have any questions!

src/core/interactive/ViewerPreferences.ts Outdated Show resolved Hide resolved
src/core/interactive/ViewerPreferences.ts Outdated Show resolved Hide resolved
src/core/interactive/ViewerPreferences.ts Outdated Show resolved Hide resolved
src/core/interactive/ViewerPreferences.ts Outdated Show resolved Hide resolved
src/core/interactive/ViewerPreferences.ts Outdated Show resolved Hide resolved
src/core/interactive/ViewerPreferences.ts Outdated Show resolved Hide resolved
src/core/interactive/ViewerPreferences.ts Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/core/interactive/ViewerPreferences.ts Outdated Show resolved Hide resolved
tests/api/PDFDocument.spec.ts Show resolved Hide resolved
@Hopding Hopding changed the base branch from master to cleanup-745 January 24, 2021 23:08
Copy link
Owner

@Hopding Hopding left a comment

Choose a reason for hiding this comment

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

Thanks again for all your work on this @mcshaz! These new setters/getters will go out in the next release of pdf-lib 🎉

@Hopding Hopding merged commit 0813ccb into Hopding:cleanup-745 Jan 24, 2021
Hopding added a commit that referenced this pull request Jan 25, 2021
@Hopding Hopding mentioned this pull request Jan 25, 2021
Hopding pushed a commit that referenced this pull request Aug 30, 2021
* more ViewerPreferences

address issue #744

* PDFHexString ViewerPreferences

* Update ViewerPreferences.ts

* neaten ViewerPreferences

* type assertIsOneOf values

* clearer documentation of viewerPreferences

* Update tsconfig target

* README with viewer pref

* tests for viewer preferences

* more ViewerPreferences

address issue #744

* revert to ES5 target

* fix tsconfig es case

ensure it does not show up as a change in the diff

* address Hopding coments 1

* minor viewerpref doc changes

* doc viewerPref with pageMode

* set pagemode

* test existing viewerPrefs in doc

* document viewerPref enums

* lint viewerPreferences

* comment linting

* comments in 0 printpage index
Hopding added a commit that referenced this pull request Aug 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants