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

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

Open
muzimuzhi opened this issue Jun 10, 2019 · 5 comments
Open

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

muzimuzhi opened this issue Jun 10, 2019 · 5 comments

Comments

@muzimuzhi
Copy link
Contributor

muzimuzhi commented Jun 10, 2019

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 CTeX-org/forum#42.

@u-fischer
Copy link
Member

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
Copy link
Member

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

@u-fischer u-fischer reopened this Sep 28, 2019
@jfbu
Copy link
Contributor

jfbu commented Nov 30, 2019

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

hyperref/hyperref.dtx

Lines 10125 to 10128 in e3c8305

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

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:

hyperref/hyperref.dtx

Lines 14845 to 14846 in e3c8305

\Hy@DisableOption{pdfcreationdate}%
\Hy@DisableOption{pdfcreationdate}%

and

hyperref/hyperref.dtx

Lines 15351 to 15352 in e3c8305

\Hy@DisableOption{pdfcreationdate}%
\Hy@DisableOption{pdfcreationdate}%

and

hyperref/hyperref.dtx

Lines 16280 to 16281 in e3c8305

\Hy@DisableOption{pdfcreationdate}%
\Hy@DisableOption{pdfcreationdate}%

and

hyperref/hyperref.dtx

Lines 16619 to 16620 in e3c8305

\Hy@DisableOption{pdfcreationdate}%
\Hy@DisableOption{pdfcreationdate}%

and

hyperref/hyperref.dtx

Lines 17761 to 17762 in e3c8305

\Hy@DisableOption{pdfcreationdate}%
\Hy@DisableOption{pdfcreationdate}%

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

@jfbu
Copy link
Contributor

jfbu commented Nov 30, 2019

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
Copy link
Contributor

jfbu commented Nov 30, 2019

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
}

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

No branches or pull requests

3 participants