davidcarlisle / dpctex

Assorted TeX packages
93 stars 30 forks source link

Add \rowcolors from xcolor #33

Closed u-fischer closed 2 years ago

u-fischer commented 2 years ago

Not ready to merge yet!

This moves the \rowcolors code from xcolor.

It misses currently this part of the xcolor code:

% \begin{macro}{\CT@extract}
% This is a fix for active `!' character to enable color expressions;
% it is apparently only necessary for |\columncolor| commands within |\multicolumn|.
%    \begin{macrocode}
 \def\CT@extract#1\columncolor#2#3\@nil
  {\if!#2%
     \let\CT@column@color\@empty
   \else
     \if[#2%]
       \expandafter\CT@extractb
     \else
       \XC@edef\XC@@tmp{#2}%
       \edef\CT@column@color{\noexpand\CT@color{\XC@@tmp}}%
       \expandafter\CT@extractd
     \fi
     {#1}#3\@nil
   \fi}
%    \end{macrocode}
% \end{macro}

\XC@edef\XC@@tmp{#2} rescans the color expression to remove active chars like !.

It is for example needed to avoid errors with

\documentclass{book}

\usepackage[french]{babel}
\usepackage{colortbl} % load first to suppress xcolor patch
\usepackage{xcolor}   % no table option 

\begin{document}
\begin{tabular}{ll}
\multicolumn{2}{>{\columncolor{red!50!white}}l}{abc} \\
a & b
\end{tabular}
\end{document}

which would give

! Package xcolor Error: Undefined color `red\penalty \@M \hskip .5\fontdimen 2\
font \relax '.

without the rescan.

(I don't know if the documentation actually compiles ...)

lvjr commented 2 years ago

Hi, I saw the following message from @davidcarlisle on TeX.SX chat and want to add a comment here since it is very important for my tabularray package.

@UlrikeFischer tempted to to change \newcount\rownum to \newcounter{rownum}\let\rownum\c@rownum so you can use \arabic{rownum}

Please DON'T make this change because it will break tabularray package. When I was writing tabularray, I checked the code of xcolor and found it didn't use rownum counter, and I was sure it would be safe to use it in tabularray. Therefore the planned change in colortbl will conflict with tabularray.

And it will make things worse if you write \newcounter{rownum}, since it will make counter library in tabularray break the whole package (see issue https://github.com/lvjr/tabularray/issues/89 for more details).

lvjr commented 2 years ago

And using a different counter name (e.g. RowNum) would be another choice, I think.

davidcarlisle commented 2 years ago

@lvjr thanks for the heads up, Just to be clear, you are saying that importing the xcolor \rownum count register here would be OK, but a latex2e counter \c@rownum would not?

As you note, we could not add the latex alias or give it another name, I'll think about it. In any case, before the merge I'll add an l3build test file using tabularray to check we don't break it. Possibly not today though. I'll ping you here to check it's testing the right thing once it is there (or you could suggest a test)

lvjr commented 2 years ago

@davidcarlisle I am not quite sure where you are planning to add the LaTeX counter: to the kernel or to colortbl package? From this pull request, I can only see it is just some simple moving of code from xcolor to colortbl. I don't know the background for this change, so I can not understand why it is necessary to add LaTeX counter while the old code runs well in general for many years? And also I can not understand why "give it another name" (other than rownum) is infeasible if you really need to add a LaTeX counter?

lvjr commented 2 years ago

you are saying that importing the xcolor \rownum count register here would be OK, but a latex2e counter \c@rownum would not?

@davidcarlisle Yes, \newcount\rownum is OK as before, but \newcount\c@rownum would cause problems, and \newcounter{rownum} would cause disasters to my package.

The LaTeX counter has been used all over the package code and has been documented as a public interface to users since the first public release of tabularray (version 2021H):

image

u-fischer commented 2 years ago

@lvr it is not so good that xcolor ask the user to type \number\rownum to print the counter, that is not LaTeX syntax. It would be better if it were a LaTeX counter and users could use \arabic{rownum} or \roman{rownum} as usual with other counters.

davidcarlisle commented 2 years ago

@lvjr this is just about this colortbl code, not the format.

the PR is simply moving a few lines from xcolor to here, my comment about adding a latex counter interface was just a comment looking at the diff as, as Ulrike comments, only providing a primitive counter means users need to use TeX primitives such as \number or \the to access the value.

As you may know expl3 already has an l3color module, and longer term we may want it to be easy to switch between xcolor and l3color, but the table option of xcolor is really an unrelated patch to colortbl and it makes more sense to have that code directly here, espcially if documents using l3color have to load xcolor just for this option and not for the actual color code.

I am not toally against using a different latex counter name but really users are used to the rownum name and use such as \arabic{rowum} would be natural to existing xcolor users intead of \the\rownum and also users of tabularray as your image shows \arabic{rownum} we know that internally tabularray and array/colortbl are very different but as an end user interface a table row counter is a table row counter....

So...

If I were to add something like \ifx\c@rownum\@undefined\let\c@rownum\rownum \fi

at package load time, and also inserted a save/restore at \begin{tabular} ... \end{tabular} so a table did not change the global value outside the table,

But I will add some tabularray tests here before doing anything else.

lvjr commented 2 years ago

At this time I can not make \rownum work well in nesting tabular tables (nesting tabularray tables are correct with rownum counter):

\documentclass{article}
\usepackage[table]{xcolor}
\begin{document}

\rowcolors*{1}{}{}
\begin{tabular}{ll}
\hline
  \the\rownum & \the\rownum \\
\hline
  \the\rownum &
  \begin{tabular}{ll}
    \the\rownum & \the\rownum \\
  \hline
    \the\rownum & \the\rownum \\
  \hline
    \the\rownum & \the\rownum \\
  \end{tabular} \\
\hline
  \the\rownum & \the\rownum \\
\hline
\end{tabular}

\bigskip

\rowcolors{1}{}{}
\begin{tabular}{ll}
\hline
  \the\rownum & \the\rownum \\
\hline
  \the\rownum &
  \rowcolors{1}{}{}
  \begin{tabular}{ll}
    \the\rownum & \the\rownum \\
  \hline
    \the\rownum & \the\rownum \\
  \hline
    \the\rownum & \the\rownum \\
  \end{tabular} \\
\hline
  \the\rownum & \the\rownum \\
\hline
\end{tabular}

\end{document}

image

To make nesting work, do you need to use LaTeX counters rownumi, rownumii, rownumiii, just like enumi, enumii, enumiii in LaTeX kernel?

davidcarlisle commented 2 years ago

@lvjr I think that would be fixed if I add the save restore i mentioned

davidcarlisle commented 2 years ago

I'm offline for rest of day

lvjr commented 2 years ago

this is just about this colortbl code, not the format.

Any reasons not to add the new counter code to array package? I guess some users would need row numbers even if they don't need colors.

lvjr commented 2 years ago

If I were to add something like \ifx\c@rownum\@undefined\let\c@rownum\rownum \fi

at package load time, and also inserted a save/restore at \begin{tabular} ... \end{tabular} so a table did not change the global value outside the table,

  • Would that break existing tabularray code and if so would it require much of a change in your code to guard against that and keep your current interface?

As long as no \newcounter is used, there will be no much code need to be changed, I guess. I will update my package on Saturday.

davidcarlisle commented 2 years ago

@lvjr thanks.

I'll merge this PR (which just moves \rownum from xcolor to here), then look and see if I can fix the nesting issue you show above. Then add a \c@rownum alias as a separate commit but I'll certainly give you time to review before releasing to ctan.

If adding \c@rownum means you just need to add a couple of lines to check if it is already defined, that would be OK I think. If it turns out to be hard and adding \c@rownum here breaks end user documents using your package, I can back out that part and release without it, Then investigate if there is some safe way to share this counter betwen the packages in a later release.

davidcarlisle commented 2 years ago

@u-fischer

Not ready to merge yet!

I can merge now?

lvjr commented 2 years ago

@davidcarlisle I am still believing that array package would be a better place for \c@rownum (see my above comment).

u-fischer commented 2 years ago

@u-fischer

Not ready to merge yet!

I can merge now?

Don't know. Did you do something about the missing \CT@extract fix? (I can't look now).

davidcarlisle commented 2 years ago

I have done nothing yet. I may merge anyway then look to fix things here, there is no urgency to release to ctan and it simplifies asking for review if It's all in one place.

davidcarlisle commented 2 years ago

@lvjr it's not a bad suggestion although complicates the timing a bit as there are then more packages involved and array is part of the core tools release. It may be easier to move it here first. @FrankMittelbach may have a view on that

FrankMittelbach commented 2 years ago

As long as no \newcounter is used, there will be no much code need to be changed, I guess. I will update my package on Saturday.

I'm not sure I get it why \newcounter is a problem but setting up \c@rownumis not. All that would be needed is for a package that uses \rownum and or \newcounter{rownum} to check if it is already defined and alter the allocation scheme accordingly. Am I missing something?

My preference would be not to put it into array, but instead put

\newcounter{rownum}

into the kernel so that it is always there. For \newcount\rownum in packages, either they could \let to \c@rownum or we could even do that too in the format, but I would prefer to keep that out of the kernel, because it is a not really a LaTeX concept.

But in either way, if we plan that for the 2022-11 release there is ample time to have the right code in (the few?) packages that either use \c@rownum or \rownum at least that would be my thinking.

lvjr commented 2 years ago

I'm not sure I get it why \newcounter is a problem but setting up \c@rownumis not. All that would be needed is for a package that uses \rownum and or \newcounter{rownum} to check if it is already defined and alter the allocation scheme accordingly. Am I missing something?

@FrankMittelbach See https://github.com/lvjr/tabularray/issues/89. The difference is that \newcounter will add counter name to \cl@@ckpt. And tabularray (like tabularx) need to save and restore all counters in the \cl@@ckpt list in building tables. But \c@rownum has been used all over the code of tabularray. Therefore \newcounter will break the package totally.

davidcarlisle commented 2 years ago

@FrankMittelbach as @lvjr says you can blame me for that:-) you need to make latex counters act somewhat normally inside code doing multiple trials, tabularx uses the top level counter reset list, as used for \include to save and restore state but tabularray does a lot more trials at a finer grain (ie per-cell rather than per-alignment). Defining \c@whatever as a name of the register allows latex syntax \roman{whatever} but \newcounter{whatever} would (without fairly extensive changes to the existing code, result in the counter being restored whenever a trial typesetting of a cell or row or ... happened and basically things would break.

@lvjr please correct me if I got that wrong, I haven't followed all the details of the tabularray code, but I have an idea about what it is doing...

FrankMittelbach commented 2 years ago

hmm, so we have the unfortunate situation that on one hand we want LaTeX like syntax for the formatting part but not that users use the LaTeX syntax for setting the counters. No good immediate idea on how that could be improved.

Well what about a lightweight interface to mark a counter as readonly on the LaTeX level? E.g.,

\def\setcounter#1#2{%
  \@ifundefined{c@#1}%
    {\@nocounterr{#1}}%
    {\ifcsname ronly@c@#1\endcsname \@ronlycounterr{#1}\else
       \global\csname c@#1\endcsname#2\relax
    \fi}}

and the same for \addtocounterand also in calc.sty? (and some \DeclareReadonlyCounteror some such name). That would mean one test per counter setting which is probably affordable.

davidcarlisle commented 2 years ago

@FrankMittelbach not quite: you want to allow setting but you want \stepcounter{foo} to increment the counter by 1, not by however many times this cell had a trial typesetting, so you need to run the counter reset list every trial to reset all counters at the start of each trial. But \rownum here is being incremented at a different time by the alignment code, not by user code in a cell, and without tracing exactly what happens it's not surprising if adding this to the restart list would require chaging the package code in interesting ways.

FrankMittelbach commented 2 years ago

Don't think so @davidcarlisle. rownum being a local counter should not be stepped at all through \setcounter or \stepcounter. it should be only manipulated internally by the code, but from a user perspective it is a readonly counter. I'm not talking about how to handle multiple executions, I'm talking about how to restrict the document-level interface so that you can look at the counter or typeset it in different ways but not update it. Such counters exist and right now the only way in LaTeX is to document that they shouldn't be updated.

lvjr commented 2 years ago

@FrankMittelbach I think as a solution \DeclareLocalCounter is better than \DeclareReadonlyCounter. Local counters will not be added to \cl@@ckpt, and can only be stepped locally. Also, local counters are requested by some users before. Moreover, local variables are more familiar to users than readonly variables.

FrankMittelbach commented 2 years ago

@lvjr maybe, but somehow to me those are orthogonal concepts. Some counter (whether local or global) should not be arbitrarily set by the user as it would create havoc isn't that part of the issue with rownum? The other problem that I see is that right now the LaTeX document level is simple: counters are always global, length registers are always local. If you provide local and global counters then the user needs to know which counter acts how which is more complicated than being told: this counter can only be read not set.

lvjr commented 2 years ago

Some counter (whether local or global) should not be arbitrarily set by the user as it would create havoc isn't that part of the issue with rownum?

@FrankMittelbach I think there are no problems with local counter rownum since every cell is a group.

\documentclass{article}
\usepackage{tabularray}
\begin{document}
\makeatletter
\begin{tblr}{hlines}
\advance\c@rownum by 4 row=\therownum & cell(\therownum,\thecolnum) \\
\advance\c@colnum by 7 col=\thecolnum & cell(\therownum,\thecolnum) \\
\end{tblr}
\makeatother
\end{document}

image

davidcarlisle commented 2 years ago

@lvjr maybe, but somehow to me those are orthogonal concepts. Some counter (whether local or global) should not be arbitrarily set by the user as it would create havoc isn't that part of the issue with rownum? The other problem that I see is that right now the LaTeX document level is simple: counters are always global, length registers are always local. If you provide local and global counters then the user needs to know which counter acts how which is more complicated than being told: this counter can only be read not set.

Currently the main issue is the reset list though, not end users chaging the value locally or globally. Adding rownum to the reset list breaks the package even if users do not set the value at all, as the list is used to do a global save/restore and rownum gets restored at points the code was not expecting and no longer counts the rows correctly. But I'll get a version here updated then we can test against tabularray and check we've not broken it before releasing anything, even if we end up moving rownum to the kernel in the end.

u-fischer commented 2 years ago

@FrankMittelbach I don't think readonly is the problem here. The problem is the resetting of the counters. Code that does trial typesetting like tabularx or tabularray have to restore user counters. So they typically do at some point

store all counters in \cl@@ckpt typesetting restore all counters in \cl@@ckpt.

But that fails if \cl@@ckpt contains a counter like rownum that the code doesn't want to restore at this point of the code for one reason or the other. One option is then not to add the counter to \cl@@ckpt (which is was @lvjr suggests by not using \newcounter), an other option would be to do

store rownum - restore all counters - restore rownum.

A third option could be to extend \stepcounter by a switch which would allow to suppress the stepping. Then such trial code could do e.g.

first typesetting \stepcountersfalse second typesetting \stepcounterstrue

and so avoid all this storing and restoring.

davidcarlisle commented 2 years ago

@u-fischer

A third option could be to extend \stepcounter by a switch which would allow to suppress the stepping. Then such trial code could do e.g.

I acually tried that originally in tabularx back then but it proved rather fragile. Consider an X column with equations if you don't step then label/ref has invalid values, you could suppress the errors during trials but they also have incorrect values so the measurements can be out (specially for non-numeric counters where the value affects the width more often), but even with numeric values, you do not want a trial to optimise for setting (9) if the real typesetting run is going to produce (10)

u-fischer commented 2 years ago

I acually tried that originally in tabularx back then but it proved rather fragile. Consider an X column with equations if you don't step then label/ref has invalid values,

well I would step in the first trial typesetting and suppress then in the next steps. That would then handle the case where the result of the trial is used (e.g. in captions).

\documentclass{article}
\newif\ifstepcounters
\stepcounterstrue
\makeatletter
\def\stepcounter#1{%
  \ifstepcounters
  \addtocounter{#1}\@ne
  \begingroup
    \let\@elt\@stpelt
    \csname cl@#1\endcsname
  \endgroup
  \fi}
\begin{document}

aaaa \stepcounter{section}\thesection\label{abc}\ref{abc}
aaaa \stepcountersfalse \let\label\@gobble
     \stepcounter{section}\thesection\label{abc}\ref{abc}

\end{document}

But I don't claim that it works for all trial-typesetting implementations. But as option it could be useful, or not?

davidcarlisle commented 2 years ago

@u-fischer I couldn't make it work reliaby enough, too many cases where you need multiple values in the same trial. Don't think caption, think of a biology paper that has at least 20 footnotes per paragraph, you need each trial setting to have the right foonote marks, and I couldn't get that to work without resetting the counter each trial and letting it increment again.

u-fischer commented 2 years ago

@davidcarlisle Oh you mean the case that a counter is stepped more than once in a cell. Yes that is flaw in the plan ;-(. Pity it looks a bit wasteful to store and restore all counters all the time, but probably unvoidable.