From 64fd9fc7b61699bb2c5b0968aa67847b3e418d96 Mon Sep 17 00:00:00 2001 From: Phuc Nguyen <16951671+phucnht@users.noreply.github.com> Date: Tue, 25 Aug 2026 11:13:41 +0700 Subject: [PATCH] fix: survive replay contexts and boxed comments A change command is protected, so its tokens travel unexecuted into anything LaTeX reads twice: a table of contents, this package's own report file, the running header and footer. Every one of those replays was measured to fail, not assumed: - an id inside a heading was a fatal duplicate-ID error on the second pass, because validation ran again for the replayed copy - one change recorded once per echo in the change report: a heading with one change and a two-page document produced four report lines for it, not one - a heading change's hyperref bookmark carried the raw command tokens concatenated, "OLDHNEWH", because hyperref's string expansion has no definition for \txreplace and drops the command while keeping every argument - a margin or todo comment inside a float, a footnote, or a minipage was a fatal "Float(s) lost", because \marginpar is illegal in a box The fix is a replay depth counter, incremented and decremented by kernel hooks on \@starttoc and \@outputpage, so a change command renders its resolved text instead of markup wherever it replays and skips validation and counting entirely there. Depth rather than a flag, because a page can break inside a long contents listing and nest one replay context inside the other. hyperref gets its own expandable stand-ins, since \pdfstringdefDisableCommands runs the disable list itself rather than executing the document's, and the disable list only \let-remaps onto them, avoiding the hash-doubling that macro parameters inside that list would cause. The margin fallback measures four signals against the actual kernel contexts rather than guessing: \l_tx_boxed_bool set by the same kernel hooks around \@makefntext and \@floatboxreset, \@captype existing only inside a float environment, \@mpfn reading mpfootnote only inside a minipage, and \mode_if_inner: for a plain restricted box. Falls back to an inline comment with a warning that fires once per document. Report text is scanned for tokens that only work in the context they were written in (math shift, alignment, sub/superscript) and shown as literal source only when one is present; detokenizing unconditionally was tried first and mangles UTF-8 prose under pdfTeX, where accented characters are active and arrive as raw bytes. tests/robust.tex exercises all of it together: a heading with an id, a rejected heading change, inline and display math, a float with a removed span and a boxed comment, a caption, a running header across a page break, and a galley-mode comment that must remain a real margin note. case_robust in scripts/test.sh pins one assertion per finding above. Verified each guard is not vacuous by disabling it in turn: no replay hooks brings back the duplicate-ID failure, no margin-context detection brings back "Float(s) lost", no report-text scan brings back "Missing $ inserted". Restoring each passes again. Full suite green at 23 cases. --- CHANGELOG.md | 4 ++ scripts/test.sh | 37 ++++++++++ tests/robust.tex | 38 ++++++++++ texchanges.sty | 177 +++++++++++++++++++++++++++++++++++++++++++++-- 4 files changed, 250 insertions(+), 6 deletions(-) create mode 100644 tests/robust.tex diff --git a/CHANGELOG.md b/CHANGELOG.md index 5a46121..555f131 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,10 @@ All notable changes to this project are documented in this file. The format is b ### Fixed +- Change commands survive the contexts a document actually moves them through. A change with an `id` inside a heading, a caption, or anywhere else that gets read a second time (a table of contents, a running header) no longer fails compilation with a duplicate-ID error, no longer records the change once per echo in the change report, and no longer leaves struck-through markup in a heading's PDF bookmark; hyperref's bookmark now shows the same resolved text a table of contents does. A margin or todo comment inside a float, a footnote, or a minipage no longer aborts the document with "Float(s) lost"; it falls back to an inline comment with a one-time warning, since `\marginpar` cannot be placed there. Report entries for a change written in math mode, or containing any token that only works in the context it came from, are shown as literal source text instead of being retypeset outside that context. + +### Fixed + - The documentation site stops widening past its design width. The sidebar, the content, the table of contents and the header bar now share one pair of outer edges on a large monitor instead of leaving an empty gutter on the right. - The theme and language pickers show one caret arrow rather than two. diff --git a/scripts/test.sh b/scripts/test.sh index c2890f5..0bb2e19 100755 --- a/scripts/test.sh +++ b/scripts/test.sh @@ -333,6 +333,42 @@ case_editor_files() { assert_contains "$PROJECT_ROOT/editors/texstudio/texchanges.cwl" '\replaced{new}{old}' } +case_robust() { + # Moving arguments and boxed contexts. Before the replay guard existed, a + # heading change with an id was a fatal duplicate-ID error on the second + # pass, one heading change produced one report line per page header plus + # one per contents listing, and a margin comment inside a float aborted + # with "Float(s) lost". Each assertion below pins one of those against the + # fixture in tests/robust.tex. + compile_named robust 2 + + # One report line and a count of exactly 1 per change, however many times + # the heading echoes through the contents and the running headers. + test "$(grep -c txreportline "$TASK_TMP_DIR/robust.txc")" = 8 + assert_contains "$TASK_TMP_DIR/robust.txs" "{rob/replaced/pending}{2}" + assert_contains "$TASK_TMP_DIR/robust.txs" "{rob/replaced/rejected}{1}" + assert_contains "$TASK_TMP_DIR/robust.txs" "{rob/added/pending}{2}" + assert_contains "$TASK_TMP_DIR/robust.txs" "{rob/removed/pending}{1}" + assert_contains "$TASK_TMP_DIR/robust.txs" "{rob/commented/pending}{2}" + + # The contents resolve markup: the pending heading shows only its new text + # there, so the old token appears exactly once, in the marked-up body. + test "$(grep -o HOLDTOKEN "$TASK_TMP_DIR/robust.txt" | wc -l | tr -d ' ')" = 1 + # A rejected heading change resolves to its original text in the contents, + # so the discarded new text never reaches the contents line. + test "$(grep -o HDROPNEW "$TASK_TMP_DIR/robust.txt" | wc -l | tr -d ' ')" = 1 + + # hyperref bookmarks carry the resolved title, not the concatenated pair. + assert_contains "$TASK_TMP_DIR/robust.out" "H\000N\000E\000W" + assert_not_contains "$TASK_TMP_DIR/robust.out" "H\000O\000L\000D" + + # The boxed margin comment fell back inline with a warning instead of + # losing the float; the galley one stayed a real margin note. + assert_contains "$TASK_TMP_DIR/robust.log" "Falling back to an inline comment" + assert_contains "$TASK_TMP_DIR/robust.txt" "BOXNOTE" + assert_contains "$TASK_TMP_DIR/robust.txt" "GALLEYNOTE" +} + case_playground() { # The website playground reimplements the package's mode semantics in # JavaScript so a visitor sees them without a TeX installation. Hold that @@ -418,6 +454,7 @@ run_case manpage run_case style_matrix run_case compat_prefixes run_case editor_files +run_case robust run_case playground run_case l3build run_case latexdiff diff --git a/tests/robust.tex b/tests/robust.tex new file mode 100644 index 0000000..b65a632 --- /dev/null +++ b/tests/robust.tex @@ -0,0 +1,38 @@ +% Moving-argument and boxed contexts: headings with IDs, captions, running +% headers, margin comments inside boxes, and simple math. Each was measured +% to fail before the replay guard and the margin fallback existed; the +% assertions in scripts/test.sh case_robust pin the corrected behaviour. +\documentclass{article} +\usepackage[review,commentmarkup=margin]{texchanges} +\usepackage{hyperref} +\pagestyle{headings} +\txdefineauthor[name={Robust Reviewer}, color=orange]{rob} +\begin{document} +\tableofcontents +\listoffigures + +\section{Alpha \txreplace[author=rob, id=RH1]{HOLDTOKEN}{HNEWTOKEN} title} +Body text mentions the heading change once. + +\section{Beta \txreplace[author=rob, id=RH2, status=rejected]{HKEPTOLD}{HDROPNEW} kept} +A rejected heading change resolves to its original text in the contents. + +Inline math $a + \txreplace[author=rob, id=RM1]{x^2}{y^2} = b$ and display: +\begin{equation} +E = \txadd[author=rob, id=RM2]{mc^2} +\end{equation} + +\begin{figure}[h] +\centering +Float body \txremove[author=rob, id=RF1]{FLOATCUT} here, +with a boxed margin comment \txcomment[author=rob, id=RC1]{BOXNOTE}. +\caption{Caption \txadd[author=rob, id=RCAP1]{CAPTOKEN} text} +\end{figure} + +Galley margin comment \txcomment[author=rob, id=RC2]{GALLEYNOTE} stays in the margin. + +\newpage +Second page exercises the running header. + +\txlistofchanges +\end{document} diff --git a/texchanges.sty b/texchanges.sty index 31deda6..63f9729 100644 --- a/texchanges.sty +++ b/texchanges.sty @@ -37,6 +37,22 @@ \bool_new:N \g_tx_color_bool \bool_new:N \g_tx_pending_bool \bool_new:N \g_tx_todonotes_loaded_bool +% Depth of replay contexts: reading a contents file back, or building the +% running header and footer. Change commands re-execute there because their +% tokens travel through moving arguments and marks, so side effects (ID +% validation, counting, report lines, labels) must fire only at depth zero, +% and markup renders as resolved text. An integer rather than a boolean so a +% page break inside a long contents listing cannot clear the outer state. +\int_new:N \g_tx_replay_int +\bool_new:N \g_tx_margin_warned_bool +% True while typesetting inside a footnote or a float box. Set locally by the +% kernel hooks near the end of this file, so the group closing the box also +% clears the flag. Mode conditionals cannot detect these contexts: a +% paragraph inside a \vbox is unrestricted horizontal mode, so \ifinner is +% false exactly where \marginpar silently loses the float. +\bool_new:N \l_tx_boxed_bool +\prg_new_conditional:Npnn \tx_if_replay: {T,F,TF} + {\int_compare:nNnTF {\g_tx_replay_int} > {0} {\prg_return_true:} {\prg_return_false:}} \dim_new:N \g_tx_truncate_width_dim \dim_new:N \g_tx_summary_width_dim @@ -189,10 +205,22 @@ \tl_if_blank:nF {#1} { \tl_if_in:nnTF {#1}{=} - {\keys_set:nn {tx/change}{#1}} + { + % article's \ps@headings passes section marks through + % \MakeUppercase, so by the time a change replays in the running + % header its keys read AUTHOR=ROB, STATUS=REJECTED. Lowercase the + % whole list there before parsing: the status value is what the + % resolved rendering needs, and everything else in a replay is + % unused. Outside replay the list is parsed exactly as written. + \tx_if_replay:TF + {\exp_args:Nne \keys_set:nn {tx/change}{\str_lowercase:n{#1}}} + {\keys_set:nn {tx/change}{#1}} + } {\tl_set:Nn \l_tx_label_tl {#1}\bool_set_true:N \l_tx_legacy_label_bool} } - \tx_change_validate: + % A replayed change was already validated where it was written; checking + % again would register its ID a second time and fail as a duplicate. + \tx_if_replay:F {\tx_change_validate:} } \cs_new_protected:Npn \tx_change_validate: { @@ -320,15 +348,50 @@ } \cs_new_protected:Npn \tx_author_at:n #1 {\str_if_eq:VnT \g_tx_author_position_tl {#1}{\tl_if_blank:eF {\tx_author_name:}{{\color{txCurrentColor}\tx_author_markup:n{\tx_author_name:}}}}} +% A margin note is a \marginpar underneath, and \marginpar is illegal inside +% a box: a float, a footnote, a minipage. TeX is in internal mode there, so +% the note falls back to an inline comment instead of losing the float. The +% warning fires once per document, naming the fix. +\cs_new_protected:Npn \tx_inner_fallback_warn: + { + \bool_if:NF \g_tx_margin_warned_bool + { + \bool_gset_true:N \g_tx_margin_warned_bool + \PackageWarning{texchanges}{Margin~and~todo~comments~cannot~be~placed~inside~a~float,~footnote,~or~minipage.~Falling~back~to~an~inline~comment~here.~Use~commentmarkup=inline~to~silence~this~warning} + } + } +% \marginpar is legal only in the main galley. The blocked contexts are a +% float box, a footnote, a minipage, and any smaller box; each is detected by +% a signal measured to be present exactly there: the kernel hooks set +% \l_tx_boxed_bool for floats and footnotes, \@captype exists only inside a +% float environment, \@mpfn reads mpfootnote only inside a minipage, and +% \mode_if_inner: covers restricted horizontal boxes such as \mbox. +\prg_new_conditional:Npnn \tx_if_margin_blocked: {TF} + { + \bool_lazy_any:nTF + { + {\l_tx_boxed_bool} + {\cs_if_exist_p:N \@captype} + {\str_if_eq_p:on {\@mpfn} {mpfootnote}} + {\mode_if_inner_p:} + } + {\prg_return_true:} {\prg_return_false:} + } +\cs_new_protected:Npn \tx_margin_or_inline:n #1 + { + \tx_if_margin_blocked:TF + {\tx_inner_fallback_warn:{\color{texchangesComment}\textbf{[\tx_author_name:\space}#1\textbf{]}}} + {\marginpar{{\color{txCurrentColor}\textbf{[\tx_author_name:]}~#1}}} + } \cs_new_protected:Npn \tx_comment_markup:n #1 { \str_case:VnF \g_tx_comment_style_tl { {inline}{{\color{texchangesComment}\textbf{[\tx_author_name:\space}#1\textbf{]}}} - {margin}{\marginpar{{\color{txCurrentColor}\textbf{[\tx_author_name:]}~#1}}} + {margin}{\tx_margin_or_inline:n {#1}} {footnote}{\footnote{\textbf{[\tx_author_name:]}~#1}} {uwave}{{\color{txCurrentColor}\uwave{\textbf{[\tx_author_name:]}~#1}}} - {todo}{\bool_if:NTF \g_tx_todonotes_loaded_bool {\todo[color=txCurrentColor!10,bordercolor=txCurrentColor,nolist]{#1}}{{\color{texchangesComment}[#1]}}} + {todo}{\bool_if:NTF \g_tx_todonotes_loaded_bool {\tx_if_margin_blocked:TF {\tx_inner_fallback_warn:{\color{texchangesComment}[#1]}} {\todo[color=txCurrentColor!10,bordercolor=txCurrentColor,nolist]{#1}}}{{\color{texchangesComment}[#1]}}} }{{\color{texchangesComment}[#1]}} } \NewDocumentCommand \txsetaddedmarkup { m } {\cs_gset:Npn \tx_added_markup:n ##1 {#1}} @@ -358,6 +421,10 @@ } \cs_new_protected:Npn \tx_record:nn #1#2 { + % Counting, the report line, and the anchor belong to the change where it + % was written, not to its echo in a contents line or a page header. + \tx_if_replay:F + { \str_if_eq:VnT \g_tx_mode_tl {review} { \tx_count:n {#1} @@ -366,12 +433,14 @@ \exp_args:NV \addtocontents \g_tx_loc_ext_tl {\protect\txreportline{#1}{\l_tx_status_tl}{\l_tx_author_tl}{\l_tx_id_tl}{#2}} } + } } \cs_new_protected:Npn \tx_change_begin:nn #1#2 { \tx_change_parse:n {#2} \tl_set:Nn \l_tx_type_tl {#1} - \tx_set_color: + % Resolved replay text carries no colour, so skip the lookup entirely. + \tx_if_replay:F {\tx_set_color:} } \cs_new_protected:Npn \tx_pending_markup:nn #1#2 { @@ -387,14 +456,31 @@ \tx_author_at:n {right} \tl_if_blank:VF \l_tx_comment_tl {\tx_comment_markup:n{\l_tx_comment_tl}} } +% What a change contributes inside a contents line or a page header: its +% resolved text, never markup. Struck-through ink has no place in a table of +% contents, and ulem's boxes are fragile there. Pending and accepted read as +% the proposed text, rejected keeps the original, and original mode keeps the +% original throughout, which matches how final and original mode already +% resolve. +\cs_new_protected:Npn \tx_render_resolved:nn #1#2 + { + \bool_lazy_or:nnTF + {\str_if_eq_p:Vn \g_tx_mode_tl {original}} + {\str_if_eq_p:Vn \l_tx_status_tl {rejected}} + {\str_case:Vn \l_tx_type_tl {{removed}{#1}{replaced}{#1}{highlighted}{#2}}} + {\str_case:Vn \l_tx_type_tl {{added}{#2}{replaced}{#2}{highlighted}{#2}}} + } \cs_new_protected:Npn \tx_render:nn #1#2 { + \tx_if_replay:TF {\tx_render_resolved:nn{#1}{#2}} + { \str_case:Vn \g_tx_mode_tl { {original}{\str_case:Vn \l_tx_type_tl {{removed}{#1}{replaced}{#1}{highlighted}{#2}}} {final}{\str_if_eq:VnTF \l_tx_status_tl {rejected}{\str_case:Vn \l_tx_type_tl {{removed}{#1}{replaced}{#1}{highlighted}{#2}}}{\str_case:Vn \l_tx_type_tl {{added}{#2}{replaced}{#2}{highlighted}{#2}}}} {review}{\str_if_eq:VnTF \l_tx_status_tl {pending}{\tx_pending_markup:nn{#1}{#2}}{\str_if_eq:VnTF \l_tx_status_tl {accepted}{\str_case:Vn \l_tx_type_tl {{added}{#2}{replaced}{#2}{highlighted}{#2}}}{\str_case:Vn \l_tx_type_tl {{removed}{#1}{replaced}{#1}{highlighted}{#2}}}}} } + } } \cs_new_protected:Npn \tx_change:nnnn #1#2#3#4 {\group_begin:\tx_change_begin:nn{#1}{#2}\tx_record:nn{#1}{#4}\tx_render:nn{#3}{#4}\group_end:} @@ -433,12 +519,37 @@ status .clist_set:N = \l_tx_list_status_clist, author .clist_set:N = \l_tx_list_author_clist } +% A change written in math mode arrives in the report as {y^2}, and +% typesetting that outside math mode is a fatal "Missing $ inserted"; an +% alignment tab is a fatal "Misplaced alignment tab" the same way. Scan the +% recorded text at every brace depth for tokens that only work in the context +% they came from (math shift, alignment, superscript, subscript) and show +% such an entry as literal source text instead. Everything else, including +% non-ASCII prose, keeps typesetting as before; blanket detokenizing was +% measured to mangle UTF-8 text under pdfTeX, where accented characters are +% active and arrive as raw bytes. +\bool_new:N \l_tx_report_detok_bool +\cs_new_protected:Npn \tx_report_text:n #1 + { + \bool_set_false:N \l_tx_report_detok_bool + \tl_analysis_map_inline:nn {#1} + { + \int_case:nn {"##3} + { + {3}{\bool_set_true:N \l_tx_report_detok_bool} + {4}{\bool_set_true:N \l_tx_report_detok_bool} + {7}{\bool_set_true:N \l_tx_report_detok_bool} + {8}{\bool_set_true:N \l_tx_report_detok_bool} + } + } + \bool_if:NTF \l_tx_report_detok_bool {\detokenize{#1}} {#1} + } \cs_new:Npn \txreportline #1#2#3#4#5 { \clist_if_in:NnT \l_tx_list_show_clist {#1} {\clist_if_in:NnT \l_tx_list_status_clist {#2} {\clist_if_in:NnT \l_tx_list_author_clist {#3} - {\par\noindent\textbf{\tx_type_name:n{#1}}\tl_if_blank:nF{#4}{~(#4)}:~\truncate{\g_tx_truncate_width_dim}{#5}\tl_if_blank:nF{#4}{\dotfill\pageref{tx:#4}}\par}}} + {\par\noindent\textbf{\tx_type_name:n{#1}}\tl_if_blank:nF{#4}{~(#4)}:~\truncate{\g_tx_truncate_width_dim}{\tx_report_text:n{#5}}\tl_if_blank:nF{#4}{\dotfill\pageref{tx:#4}}\par}}} } \cs_new:Npn \tx_type_name:n #1 {\str_case:nnF{#1}{{added}{\txaddedname}{removed}{\txremovedname}{replaced}{\txreplacedname}{highlighted}{\txhighlightedname}{commented}{\txcommentedname}}{#1}} \cs_new:Npn \tx_status_name:n #1 {\str_case:nnF{#1}{{pending}{\txpendingname}{accepted}{\txacceptedname}{rejected}{\txrejectedname}}{#1}} @@ -547,6 +658,60 @@ } \cs_new:Npn \texchangesmode {\g_tx_mode_tl} + +% -------------------------------------------------------------------------- +% Replay contexts +% +% Change commands are protected, so their tokens survive into .toc-family +% files and into page marks unexecuted, and run again when those are read +% back or when the header is built. \@starttoc covers every contents file, +% including this package's own report files; \@outputpage covers the running +% header and footer. A depth counter rather than a set/clear pair, because a +% page can break inside a long contents listing, which nests one context +% inside the other. +% -------------------------------------------------------------------------- +\AddToHook{cmd/@starttoc/before}{\int_gincr:N \g_tx_replay_int} +\AddToHook{cmd/@starttoc/after}{\int_gdecr:N \g_tx_replay_int} +\AddToHook{cmd/@outputpage/before}{\int_gincr:N \g_tx_replay_int} +\AddToHook{cmd/@outputpage/after}{\int_gdecr:N \g_tx_replay_int} + +% Both hooks run inside the box group they mark, so the local flag clears +% itself when the group ends. \@makefntext is the kernel's footnote-text +% wrapper; \@floatboxreset runs at the top of every float box. +\AddToHook{cmd/@makefntext/before}{\bool_set_true:N \l_tx_boxed_bool} +\AddToHook{cmd/@floatboxreset/before}{\bool_set_true:N \l_tx_boxed_bool} + +% hyperref builds PDF bookmark strings by expanding the heading text with all +% robust commands disabled; without help it drops the change command and +% keeps every argument, so a replaced heading bookmarks as "OLDNEW". These +% expandable stand-ins resolve each change the same way the replay renderer +% does for the document's mode, minus colour, which a bookmark cannot carry. +% Bookmarks cannot inspect per-change status (the key list is unexpandable +% there), so they resolve by document mode alone: original keeps old text, +% review and final show the proposed text. These are fully expandable, which +% is what survives hyperref's string expansion. The disable list below only +% \let-remaps the public names onto them; storing macro parameters inside +% that list is a hash-doubling trap, so no parameter tokens appear there. +\NewExpandableDocumentCommand \txbookmarkadd { O{} m } + {\str_if_eq:VnTF \g_tx_mode_tl {original} {} {#2}} +\NewExpandableDocumentCommand \txbookmarkremove { O{} m } + {\str_if_eq:VnTF \g_tx_mode_tl {original} {#2} {}} +\NewExpandableDocumentCommand \txbookmarkreplace { O{} m m } + {\str_if_eq:VnTF \g_tx_mode_tl {original} {#2} {#3}} +\NewExpandableDocumentCommand \txbookmarkkeep { O{} m } {#2} +\NewExpandableDocumentCommand \txbookmarkdrop { O{} m } {} +\AddToHook{package/hyperref/after} + { + \pdfstringdefDisableCommands + { + \let \txadd \txbookmarkadd + \let \txremove \txbookmarkremove + \let \txreplace \txbookmarkreplace + \let \txhighlight \txbookmarkkeep + \let \txcomment \txbookmarkdrop + } + } + \PackageInfo{texchanges}{Rendering~changes~in~\g_tx_mode_tl~mode} \ExplSyntaxOff \endinput