Skip to content

Commit

Permalink
Add better support for AppOptions with multiple types (PR 18465 follo…
Browse files Browse the repository at this point in the history
…w-up)

For options with varying types, see `useSystemFonts`, we're not sufficiently validating the type when setting a new value. This means that for an option that uses `OptionKind.UNDEF_ALLOWED` we'd allow *any* value, which is obviously not the intention.

Hence we instead introduce a new and *optional* `type`-field that allows specifying exactly which types are valid when multiple ones are supported.

*Note:* This obviously didn't occur to me until after PR 18465 landed, sorry about that!
  • Loading branch information
Snuffleupagus committed Jul 22, 2024
1 parent deaac28 commit b3ea789
Showing 1 changed file with 18 additions and 11 deletions.
29 changes: 18 additions & 11 deletions web/app_options.js
Original file line number Diff line number Diff line change
Expand Up @@ -55,10 +55,18 @@ const OptionKind = {
API: 0x04,
WORKER: 0x08,
EVENT_DISPATCH: 0x10,
UNDEF_ALLOWED: 0x20,
PREFERENCE: 0x80,
};

// Should only be used with options that allow multiple types.
const Type = {
BOOLEAN: 0x01,
NUMBER: 0x02,
OBJECT: 0x04,
STRING: 0x08,
UNDEFINED: 0x10,
};

/**
* NOTE: These options are used to generate the `default_preferences.json` file,
* see `OptionKind.PREFERENCE`, hence the values below must use only
Expand Down Expand Up @@ -405,7 +413,8 @@ const defaultOptions = {
)
? false
: undefined,
kind: OptionKind.API + OptionKind.UNDEF_ALLOWED,
kind: OptionKind.API,
type: Type.BOOLEAN + Type.UNDEFINED,
},
verbosity: {
/** @type {number} */
Expand Down Expand Up @@ -484,7 +493,7 @@ if (typeof PDFJSDev === "undefined" || PDFJSDev.test("GENERIC")) {
if (typeof PDFJSDev === "undefined" || PDFJSDev.test("TESTING || LIB")) {
// Ensure that the `defaultOptions` are correctly specified.
for (const name in defaultOptions) {
const { value, kind } = defaultOptions[name];
const { value, kind, type } = defaultOptions[name];

if (kind & OptionKind.PREFERENCE) {
if (kind === OptionKind.PREFERENCE) {
Expand All @@ -493,9 +502,9 @@ if (typeof PDFJSDev === "undefined" || PDFJSDev.test("TESTING || LIB")) {
if (kind & OptionKind.BROWSER) {
throw new Error(`Cannot mix "PREFERENCE" and "BROWSER" kind: ${name}`);
}
if (kind & OptionKind.UNDEF_ALLOWED) {
if (type !== undefined) {
throw new Error(
`Cannot allow \`undefined\` value for "PREFERENCE" kind: ${name}`
`Cannot have \`type\`-field for "PREFERENCE" kind: ${name}`
);
}
if (typeof compatParams === "object" && compatParams.has(name)) {
Expand All @@ -512,9 +521,9 @@ if (typeof PDFJSDev === "undefined" || PDFJSDev.test("TESTING || LIB")) {
throw new Error(`Invalid value for "PREFERENCE" kind: ${name}`);
}
} else if (kind & OptionKind.BROWSER) {
if (kind & OptionKind.UNDEF_ALLOWED) {
if (type !== undefined) {
throw new Error(
`Cannot allow \`undefined\` value for "BROWSER" kind: ${name}`
`Cannot have \`type\`-field for "BROWSER" kind: ${name}`
);
}
if (typeof compatParams === "object" && compatParams.has(name)) {
Expand Down Expand Up @@ -565,8 +574,7 @@ class AppOptions {
!defaultOpt ||
!(
typeof value === typeof defaultOpt.value ||
(defaultOpt.kind & OptionKind.UNDEF_ALLOWED &&
(value === undefined || defaultOpt.value === undefined))
Type[(typeof value).toUpperCase()] & defaultOpt.type
)
) {
return;
Expand All @@ -585,8 +593,7 @@ class AppOptions {
!defaultOpt ||
!(
typeof userOpt === typeof defaultOpt.value ||
(defaultOpt.kind & OptionKind.UNDEF_ALLOWED &&
(userOpt === undefined || defaultOpt.value === undefined))
Type[(typeof userOpt).toUpperCase()] & defaultOpt.type
)
) {
continue;
Expand Down

0 comments on commit b3ea789

Please sign in to comment.