vincentb1 / fmtcount

Source for LaTeX fmtcount package
11 stars 5 forks source link

New definition of `\Numberstring` is now causing problems with glossaries.sty #27

Closed nlct closed 6 years ago

nlct commented 7 years ago

The original definition of \Numberstring was:

\newcommand*{\Numberstring}[1]{%
  \expandafter\protect\expandafter\Numberstringnum{%
    \expandafter\the\csname c@#1\endcsname}%
}

This expansion and use of \csname ensures the sample file samplexdy.tex works. The new definition of the command:

\newcommand*{\Numberstring}[1]{%
  \expandafter\Numberstringnum\expandafter{%
    \the\value{#1}}%
}

breaks the sample file.

vincentb1 commented 7 years ago

I will have a look. As far as I can remember, the discussion was that in the former definition, the two first \expandafter were useless because they accounted to expand { which is not expandable. The \protect was also useless, because \Numberstringnum is now declared with \newrobustcmd* of etoolbox. I let you know, when I understand what happens.

nlct commented 7 years ago

I'm sorry, I should've clarified. It's the \expandafter\the\csname c@#1\endcsname bit that's important from the glossaries.sty point of view, so samplexdy.tex works fine with:

\renewcommand*{\Numberstring}[1]{%
  \Numberstringnum{\expandafter\the\csname c@#1\endcsname}%
}

This problem is specific to glossaries with xindy because it has to do a bit of trickery to get around two conflicting requirements: \Numberstring{page} needs to expand to \\Numberstringnum{page number} in the glossary (note the escaped backslash) for xindy's benefit but \c@page can't expand until the actual write occurs (due to TeX's asynchronous output routine). So \Numberstring{page} needs to be expanded before the write occurs in order to escape \Numberstring but it can't expand before the write occurs because \c@page might not be correct.

I admit that this is a rather esoteric edge case :smile: so I'm trying to find a way to deal with this within glossaries.sty but it's a bit awkward that the sample file doesn't work any more.

vincentb1 commented 7 years ago

The issue is not really in fmtcount.sty, but it is that commands \the and \number are redefined by glossaries as follows:

#1->\ifx #1\c@page \gls@numberpage \else \orgthe #1\fi 

This is in the piece of code found in glossaries.sty:

    \ifglswrallowprimitivemods
      \def\the##1{%
        \ifx##1\c@page \gls@numberpage\else\orgthe##1\fi}%
      \def\number##1{%
        \ifx##1\c@page \gls@numberpage\else\orgnumber##1\fi}%
    \fi

So inherently they expects that the sequel is a macro containing some counter, while the original \the does not behave this way, and would be ok if the sequel is \value or \csname.

Maybe this could be fixed rather in glossaries.sty as folllows (not tested, all #x would be replaced by ##x):

\def\the{\gls@the\orgthe}
\def\number{\gls@the\orgnumber}
\def\gls@the#1#2{%
  \ifx#2\value\expandafter\gls@the@value
  \else\expandafter\gls@the@i\fi#1#2}
\def\gls@the@i#1#2{%
  \ifx#2\csname\expandafter\gls@the@csname
  \else\expandafter\gls@the@ii\fi#1#2}
\def\gls@the@value{\expandafter\gls@the@csname\expandafter}
\def\gls@the@csname{\expandafter\gls@the@ii\expandafter}
\def\gls@the@ii#1#2{\ifx#2\c@page \gls@numberpage\else#1#2\fi}%

Another option is to make a simple fix in fmtcount to replace the last \expandafter by \expandafter\expandafter\expandafter so as to get two expansions, or alternatively to replace \value{...} by \csname\c@...\endcsname.

Any opinion about what is best to do — my preference would be the fix in glossaries.sty because I don't think that the code of fmtcount.sty is, as such, faulty.

vincentb1 commented 7 years ago

Ooops… I had missed your prior post. Anyway, thank you for the additional explanation why you play this sort of black magics. BTW, \orgthe and \orgnumber are a bit ackwards names, IMHO \gls@orgthe and \gls@orgnumber would have been preferable.

vincentb1 commented 7 years ago

Here is the new definition of \@@do@wrglossary with my proposed fix. I made a test with the sample file, and it seems to work with the current fmtcount. Hopefully, I did not introduce any bug… There is no change from line containing \@wrglossarynumberhook.

\newcommand*{\@@do@wrglossary}[1]{%
  \begingroup
    \let\gls@orgthe\the
    \let\gls@orgnumber\number
    \let\gls@orgarabic\@arabic
    \let\gls@orgromannumeral\romannumeral
    \let\gls@orgalph\@alph
    \let\gls@orgAlph\@Alph
    \let\gls@orgRoman\@Roman
    \ifglswrallowprimitivemods
       \def\the{\gls@the\gls@orgthe}%
       \def\number{\gls@the\gls@orgnumber}%
    \fi
    \def\@arabic{\gls@the\gls@orgarabic}%
    \def\romannumeral{\gls@the\gls@orgromannumeral}%
    \def\@Roman{\gls@the\gls@orgRoman}%
    \def\@alph{\gls@the\gls@orgalph}%
    \def\@Alph{\gls@the\gls@orgAlph}%
    \def\gls@the##1##2{%
      \ifx##2\value\expandafter\gls@the@value
      \else\expandafter\gls@the@i\fi##1##2}%
    \def\gls@the@i##1##2{%
      \ifx##2\csname\expandafter\gls@the@csname
      \else\expandafter\gls@the@ii\fi##1##2}%
    \def\gls@the@value{\expandafter\gls@the@csname\expandafter}%
    \def\gls@the@csname{\expandafter\gls@the@ii\expandafter}%
    \def\gls@the@ii##1##2{\ifx##2\c@page \gls@numberpage\else##1##2\fi}%
  \@wrglossarynumberhook
    \gls@disablepagerefexpansion
    \protected@xdef\@glslocref{\theglsentrycounter}%
  \endgroup
  \@gls@checkmkidxchars\@glslocref
  \expandafter\ifx\theHglsentrycounter\theglsentrycounter\relax
    \def\@glo@counterprefix{}%
  \else
    \protected@edef\@glsHlocref{\theHglsentrycounter}%
    \@gls@checkmkidxchars\@glsHlocref
    \edef\@do@gls@getcounterprefix{\noexpand\@gls@getcounterprefix
      {\@glslocref}{\@glsHlocref}%
    }%
    \@do@gls@getcounterprefix
  \fi
  \edef\@gls@label{\glsdetoklabel{#1}}%
  \@@do@@wrglossary
}
nlct commented 7 years ago

Okay. It's your call about fmtcount. That bit of code in glossaries is very sensitive to alterations, but I'll have a look at your suggestion. (I didn't notice it when the change was first made as it turned out I had an old version of fmtcount.sty in ~/texmf so I didn't pick up on it until I removed it.)

vincentb1 commented 7 years ago

You are right, this piece of code in glossaries is not trivial. I realized that the code provided in my previous comment erroneously makes the same formatting for all cases. Here is a fix: glossaries.zip. It is a pitty that glossaries is not on some forge, I would have supplied a patch file.

Concerning fmtcount, I've made the fix this way:

\newrocommand*\foo[1]{\foonum{\expandafter\expandafter\expandafter
  \the\value{#1}}

Where \foo takes a counter as argument, and \foonum is robust and takes an expression as argument that expands to a number. So for the fmtcount purpose itself the \expandafter's are useless, they are for compatibility with current version of glossaries.

I have made this fix, and uploaded it to repo, it passes the non regression tests, but I need to do more testing, as I realized that not all macros (e.g. \aaalph) are tested.

vincentb1 commented 7 years ago

It seems that even with the fix made in fmtcount there remains some issue. See the samplexdy.pdf which I get: the glossary is incomplete. And I get warnings like that:

pdfTeX warning (dest): name{glo:gaussianint} has been referenced but does not exist, replaced by a fixed one

makeglossaries outputs the following warning:

**Warning:**

You may have forgotten to add a location 
class with \GlsAddXdyLocation or you may have 
the format incorrect.

and farther stuff like that:

Reading raw-index "C:\\Users\\Vincent\\AppData\\Local\\Temp\\bug-fmtcount\\s2BBnoZg1y"...WARNING: location-reference "{}{\\Numberstringnum {1}}" did not match any location-class! (ignored)
WARNING: location-reference "{}{\\Numberstringnum {2}}" did not match any location-class! (ignored)
nlct commented 7 years ago

Yes, I'd already noticed that, which is why I've come to the conclusion that it's best not to modify glossaries.sty. xindy is very sensitive to the location format and a change in the internal definition can break the style. In this case the problem stems from the missing \protect which originally was written to the glossary file, so the xindy style has to include it. When the \protect is removed, the style specification no longer matches.

The glossaries manual already mentions that a specific format is required for the location command. In the next version, I'll add a note using \Numberstring as an example of how the change in definition can upset xindy. Thank you for investigating, but it's not often that documents use such esoteric numbering schemes so it's not worth the risk of breaking glossaries.sty in order to accommodate xindy idiosyncrasies, especially now that bib2gls can be used as an alternative (and that doesn't require the internal fudging that xindy needs).

vincentb1 commented 7 years ago

Hello, Thank you for clarifying this. I noticed anyway that the .xdy file produced by glossaries seems to be Lisp code, however it is incorrect Lisp code, because for instance in

(define-location-class "Numberstring"
   (:sep "{}{" :sep "\protect \Numberstringnum {" "arabic-numbers" :sep "}" :sep "}")) 

the \ characters should be duplicated.

Isn't there some attribute like :optionalsep that could be used to tell xindy that string "\\protect " is optional ?

vincentb1 commented 7 years ago

Or, a better approach would be to generate automatically the \GlsAddXdyLocation calls w/o the user intervention, just by expanding every page the \thepage inside a group where \the etc... are redefined so that the result of expansion can be analysed and if such a format was not yet detected, output it to the .xdy file.

nlct commented 7 years ago

The backslashes shouldn't be escaped in the define-location-class definition in the .xdy file, but the :locref attribute in the .glo file requires it. Doubling the backslash in the .xdy file makes the location unrecognised (but it will match four backslashes in the location, but that would lead to a double backslash in the file read by LaTeX, which is incorrect). I don't know why this difference in interpreting the Lisp code occurs, but that's the way xindy's doing it.

The old and new formats can both be defined:

\GlsAddXdyLocation{Numberstring}{:sep "\string\Numberstringnum\space\glsopenbrace" 
  "arabic-numbers" :sep "\glsclosebrace"}

\GlsAddXdyLocation{pNumberstring}{:sep "\string\protect\space\string\Numberstringnum\space\glsopenbrace"
  "arabic-numbers" :sep "\glsclosebrace"}

However, with the samplexdy.tex sample file it's not just a matter of removing \\protect from \GlsAddXdyLocation but also the custom command \linkpagenumber needs to be changed to remove the first argument (or test if it's \protect).

Unfortunately the format can't be easily automatically generated (not without a highly sophisticated algorithm that would significantly slow the document build). xindy has location classes like arabic-numbers, roman-page-numbers, Alpha-page-numbers etc and the user may provide their own. There's no way of determining if the location will end up in the form :sep "\\somenumcmd {" "arabic-numbers" :sep "}" or in some other form, such as "Roman-page-numbers" :sep "[" "arabic-numbers" :sep "]", or if the location will completely expand to the user's customised location format (such as the bible-verses location class in the xindy tutorial).

Since most documents just use standard location formats, it's not really worth adding an extra level of complexity and slowing the document build for rare cases. (There are already people who grumble about the slow compilation time when glossaries is used.) It is worth highlighting the issue for those rare cases, which is why I think it's a good idea to have it flagged here as well as in the glossaries user manual, but I don't think it's easily soluble.

vincentb1 commented 7 years ago

OK, I tried this:

nlct commented 7 years ago

Thank you for looking at it, but since the sample file is just intended as an example of a non-standard numbering system I decided that it would be best to just provide a simple number formatting command that's defined in the sample file.

I think it's useful to have this issue logged here, in case anyone else investigates it, so your code will be useful for them :smile: but I think it's best not to overload sample files with internal commands or they end up propagating in template mashups :smile:

vincentb1 commented 7 years ago

A simpler fix in samplexdy.tex would certainly be to load fmtcount.sty with a required version like this:

\usepackage{fmtcount}[2017/09/14]

and then to define the stuff only for the no-\protect case. Well, for the time being this 2017/09/14 version is not yet on CTAN, for, as I wrote, I wish to do more testing.

nlct commented 7 years ago

Just a quick note: it seems that ~ is the escape character in the .xdy file but \ is the escape character in the glossary file input by xindy. So \ is just a backslash in the :sep attribute of define-location-class. For example, if you have a double quote " as a separator in the location then :sep "~"" uses ~" to make a literal " but the :locref attribute would need \" to indicate a literal " instead.

vincentb1 commented 7 years ago

It seems that xindy use a special Lisp dialect.

BTW, I have just uploaded fmtcount v3.04 to CTAN. The main change is compatibility with glossaries, but I have also made some code optimization in the \aaalph, \@octal and suchlikes. Mainly it is about using of \@ne, \m@ne and suchlikes, but also some code factorization between lower and uper case, and taking care to expand only once the argument of the \xxxxnum type of macros. That is why I needed so much testing: any change is a risk of regression.

Attached is an updated samplexdy.tex file which is working w/o the additional complexity of declaring two location classes, and testing whether or not \protect is there in \linkpagenumber. samplexdy.zip

So, for the sake of compatibility, in this file, fmtcount is now loaded with a version date requirement, like this:

\usepackage{fmtcount}[2017/09/16]

I have read in some former post of yours that you have just gone the way of using some less exotic page format than \Numberstring. Your motivation was to avoid making samplexdy.tex some mashup, with this update this objective is reached.

nlct commented 7 years ago

I've updated glossaries (v4.33). The \Numberstring example is now in samplexdy3.tex. It works fine with the new version. Thanks.

vincentb1 commented 6 years ago

You're welcome !