hyperref icon indicating copy to clipboard operation
hyperref copied to clipboard

\Hy@DisableOption in \PDF@FinishDoc is local, hence do nothing

Open muzimuzhi opened this issue 6 years ago • 5 comments

Currently, hyperref, at the time of first page break, uses \PDF@FinishDoc to set PDF info fields like pdftitle and make related options disable. After that, any key-val setting to options like pdftitle will not influence the corresponding field in the output PDF, and should raise warning \Hy@WarnOptionDisabled.

But, since

  • the \PDF@FinishDoc is executed inside commands of atbegshi package, and
  • the \define@key (provided by keyval package) used by \Hy@DisableOption only locally (re)defines key,

options like pdftitle are never disabled, hence usages after first page break won't raise any warning.

Following is a quick fix (with test by showing the meaning of \KV@Hyp@pdftitle) by providing global variant of \define@key and using it inside \PDF@FinishDoc.

  • Without fix, \KV@Hyp@pdftitle is not redefined after the first page break.
  • With the fix, \KV@Hyp@pdftitle is redefined as desired and warning \Hy@WarnOptionDisabled is raised after the first page break.
\documentclass{article}
\usepackage{hyperref}
\hypersetup{pdftitle={preamble}}

\makeatletter
%% redifined to use \define@key@g
\def\Hy@DisableOption#1{%
  \ltx@ifundefined{KV@Hyp@#1@default}{%
    \define@key@g{Hyp}{#1}%
  }{%
    \define@key@g{Hyp}{#1}[]%
  }%
  {\Hy@WarnOptionDisabled{#1}}%
}

%% add \global version of \defin@key
\def\define@key@g#1#2{%
  \@ifnextchar[{\KV@def@g{#1}{#2}}{\global\long\@namedef{KV@#1@#2}####1}}

%% add \global version of \KV@def
\def\KV@def@g#1#2[#3]{%
  \global\long\@namedef{KV@#1@#2@default\expandafter}\expandafter
  {\csname KV@#1@#2\endcsname{#3}}%
  \global\long\@namedef{KV@#1@#2}##1}
\makeatother
\listfiles


\begin{document}
%% this one succeeds in changing the Title field
\hypersetup{pdftitle={before first newpage}}
a
\newpage

%% with fix, this one raises warning
\hypersetup{pdftitle={after first newpage}}

%% check meaing of \KV@Hyp@pdftitle
\makeatletter
\ttfamily
\meaning\KV@Hyp@pdftitle
\makeatother
\end{document}

This issue is first reported, in Chinese, in https://github.com/CTeX-org/forum/issues/42.

muzimuzhi avatar Jun 10 '19 20:06 muzimuzhi

Thank you for the report. That's clearly a bug. But I don't think that a global \define@key is the solution. One shouldn't assign commands sometimes locally and sometimes globally. We will look for a solution with \aftergroup to allow \Hy@DisableOption to escape the groups created by the atbegshi command.

u-fischer avatar Jun 11 '19 12:06 u-fischer

I add to revert the change to resolve issue #103 and so reopen this one.

u-fischer avatar Sep 28 '19 22:09 u-fischer

Rather than re-defining \Hy@DisableOption, I propose adding \Hy@GloballyDisableOption (basically the code as proposed by @muzimuzhi but with some \Hy@ prefixes) then do this modification

\g@addto@macro\Hy@FirstPageHook{%
  \let\Hy@DisableOption\Hy@GloballyDisableOption
  \PDF@FinishDoc
  \global\let\PDF@FinishDoc\ltx@empty
}

to current https://github.com/latex3/hyperref/blob/e3c83053558efde7acf8527bdafecf384d592d63/hyperref.dtx#L10125-L10128

the effect of \let will be limited to the enclosing group. Globally acting on the kvoptions and keyval keys looks like the things to do actually for

  \Hy@DisableOption{pdfauthor}%
  \Hy@DisableOption{pdftitle}%
  \Hy@DisableOption{pdfsubject}%
  \Hy@DisableOption{pdfcreator}%
  \Hy@DisableOption{addtopdfcreator}%
  \Hy@DisableOption{pdfcreationdate}%
  \Hy@DisableOption{pdfmoddate}%
  \Hy@DisableOption{pdfproducer}%
  \Hy@DisableOption{pdfkeywords}%
  \Hy@DisableOption{pdftrapped}%
  \Hy@DisableOption{pdfinfo}%

as encountered in \PDF@FinishDoc.

(I thought about \globaldefs1 but the expansion leads to LaTeX's \@PackageWarning from \Hy@Warning hence \GenericWarning and we don't want to fiddle with its actions as it does \def \MessageBreak {#1}\set@display@protect in a group).

Side note, these duplicated lines look weird:

https://github.com/latex3/hyperref/blob/e3c83053558efde7acf8527bdafecf384d592d63/hyperref.dtx#L14845-L14846

and

https://github.com/latex3/hyperref/blob/e3c83053558efde7acf8527bdafecf384d592d63/hyperref.dtx#L15351-L15352

and

https://github.com/latex3/hyperref/blob/e3c83053558efde7acf8527bdafecf384d592d63/hyperref.dtx#L16280-L16281

and

https://github.com/latex3/hyperref/blob/e3c83053558efde7acf8527bdafecf384d592d63/hyperref.dtx#L16619-L16620

and

https://github.com/latex3/hyperref/blob/e3c83053558efde7acf8527bdafecf384d592d63/hyperref.dtx#L17761-L17762

(but not at the first encountered definition of \PDF@FinishDoc). Or there is some reason escaping me?

jfbu avatar Nov 30 '19 09:11 jfbu

by the way, the definition could be simply:

\def\Hy@GloballyDisableOption#1{%
   \DisableKeyvalOption[package={hyperref},action={warning}]{Hyp}{#1}%
}

because this acts by default globally. The warning message looks like this:

Package hyperref Warning: Option `pdftitle' is already consumed
(hyperref)                and has no effect on input line 21.

which differs slightly from current warning message, but conveys same meaning.

Current code of \Hy@DisableOption looks like a hack into kvoptions and on brief look appears like it could be replaced by simpler code:

\def\Hy@DisableOption#1{%
   \DisableKeyvalOption[local,package={hyperref},action={warning}]{Hyp}{#1}%
}

which (untested) promises to be an equivalent replacement of current code. And the advantage now is that both cases are handled the same. But this obsoletes \Hy@WarnOptionDisabled hence ignores people hacks into it (or \Hy@Warning).

(naturally such a higher level definition of \Hy@DisableOption means its expansion is more costly)

jfbu avatar Nov 30 '19 10:11 jfbu

Alternative to all of the above (after checking at how \DisableKeyvalOption execute its by default global option)

\def\Hy@FirstPageHook{%
  \PDF@FinishDoc
  \Hy@GlobalizeOption{pdfauthor}%
  \Hy@GlobalizeOption{pdftitle}%
  \Hy@GlobalizeOption{pdfsubject}%
  \Hy@GlobalizeOption{pdfcreator}%
  \Hy@GlobalizeOption{addtopdfcreator}%
  \Hy@GlobalizeOption{pdfcreationdate}%
  \Hy@GlobalizeOption{pdfmoddate}%
  \Hy@GlobalizeOption{pdfproducer}%
  \Hy@GlobalizeOption{pdfkeywords}%
  \Hy@GlobalizeOption{pdftrapped}%
  \Hy@GlobalizeOption{pdfinfo}%
  \global\let\PDF@FinishDoc\ltx@empty
}%
\def\Hy@GlobalizeOption#1{%
  \global\expandafter\let\csname KV@Hyp@#1\expandafter\endcsname
                         \csname KV@Hyp@#1\endcsname
  \global\expandafter\let\csname KV@Hyp@#1@default\expandafter\endcsname
                         \csname KV@Hyp@#1@default\endcsname
}

jfbu avatar Nov 30 '19 11:11 jfbu