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

extend label to five data and add hook #956

Merged
merged 25 commits into from May 16, 2023
Merged

extend label to five data and add hook #956

merged 25 commits into from May 16, 2023

Conversation

u-fischer
Copy link
Member

@u-fischer u-fischer commented Nov 14, 2022

Internal housekeeping

Status of pull request

  • Feedback wanted
  • Ready to merge

Checklist of required changes before merge will be approved

  • Test file(s) added
  • Version and date string updated in changed source files
  • Relevant \changes entries in source included
  • Relevant changes.txt updated
  • ltnewsX.tex (and/or latexchanges.tex) updated

Summary

This pull request extends the \label command to store as with hyperref five
data. It also adds a hook similar to the one added by nameref. This allows to
remove some patches from hyperref, nameref and showkeys and to store
structure data needed for example for the tagging of footnotes.

Open questions

  • Name of the hook ok? (currently label)
  • Name of the command which stores the current argument of \label ok?
    (currently \label@name)
  • Are the names for the additional data ok?
  • Is the rollback code correct??

Notes about the code

  • As third argument it stores \@currentlabelname. An alternative would be
    to use a new name, e.g. \@currenttitle. But while this wouldn't be a problem for
    nameref as it could do \def\@currenttitle{\@currentlabelname} or could be changed
    to use the new command, it would be difficult for classes and other packages to support this, as they would have to fill
    either \@currenttitle or \@currentlabelname depending on the age of the format and
    depending on if nameref is loaded or not.

  • The fourth argument is \@currentHref. A better name would be
    \@currenttarget but \@currentHref is set by various classes and packages
    and so it can't be dropped for similar reasons.

  • The fifth argument should be reserved for the kernel, but currently no
    concrete use is planed or implemented.
    One option is to store an id for the current structure.

  • The change of the \setref command avoids that documents compiled
    with an older format and where the aux-file contains old
    „2-data-\newlabel's“ error with "runaway argument".

  • The name of the label is saved into \label@name. This is the
    command name also used in nameref. But this could be changed, no package using
    it (apart from hyperref and nameref) could be found.

Example of uses for the hook

  • showkeys redefinition of \label can be replaced by
\AddToHook{label}{\SK@\SK@@label{\label@name}} 
  • nameref uses its hook to strip a period from the title so would
    have to do
\AddToHook{label}{\NR@sanitize@labelname} 
  • a zref user can do
\AddToHook{label}{\zlabel{\label@name}}

Compability with external packages.

  • The testsuite only showed test failures due to the changed write command.
  • cleveref redefines \label to take an optional argument but makes
    use of the original command and seems to work fine.
  • No problems found with varioref, smartref, showlabels either.
  • the thref option of ntheorem is incompatible. It doesn't error but
    overwrites the changes again. (It is also incompatible with hyperref and cleveref
    and should better not be used anyway.)
  • xr-hyper/hyperref use currently the fifth argument and must be adapted if the
    kernel wants to use it too.

Copy link
Member

@FrankMittelbach FrankMittelbach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments added, I think this needs some separate discussions.

\subsection{Extension of the \cs{label} command}

Up to now the \cs{label} command stored in standard \LaTeX{} into the
\cs{newlabel} command in the \texttt{.aux} file two data:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would use "values" not "data" even if data is correct in its plural

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or "data items"


Starting with this release the number of arguments have be unified.
\cs{label} now writes a
\cs{newlabel} command containing five data: \cs{@currentlabel},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with five arguments

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No! They are not (and in ltxref never become) arguments of anything --

Suggestion: ... command that stores, in its second argument, the following five items of data:

Copy link
Contributor

@car222222 car222222 Dec 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

container ==> item


Additionally a hook with the name \texttt{label} has been added. It is
executed before the writing to the \texttt{.aux} file. Code using
the hook can refer to the label argument with \cs{label@name}.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if @currentlabelname etc why \label@name and not @labelargument, ie the @ always in the same position?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from a general perspective I wonder if there shouldn't actually be 6 arguments (the last 2 key/value lists)
with

  • arg5 reserved for packages to pass stuff around
  • arg6 reserved for the kernel to pass extra stuff around

shold probably be discussed elsewhere, not in this pull.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if @currentlabelname etc why \label@name and not @labelargument, ie the @ always in the same position?

\label@name is the command currently used by nameref. But `@labelargument would be fine with me.

6 arguments (the last 2 key/value lists)

perhaps, but at first the patches from hyperref and nameref should disappear.

Copy link
Contributor

@car222222 car222222 Dec 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If changing to 6 items is too difficult, item 6 could contain two sub-items(?), one for holding the user-information (in a well-defined and consistent format) and the other for the kernel.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed yesterday, we can do with one argument as long as we define that datastructure in a way that packages (or users) can add to it in a controlled way. But if not we can indeed use 2 brace groups to separate them if that seems advisable in the end.

Copy link
Contributor

@car222222 car222222 Dec 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

argument ==> (data) item

base/ltxref.dtx Outdated
@@ -108,7 +108,8 @@
% incremented by the command \refstepcounter{CNT} , which sets
% \@currentlabel == {CNT}{eval(\p@cnt\theCNT)}. The command
% \label{FOO} then writes the following on file \@auxout :
% \newlabel{FOO}{{eval(\@currentlabel)}{eval(\thepage)}}
% \newlabel{FOO}{{eval(\@currentlabel)}{eval(\thepage)}%
% {eval(\@currentlabelname)}{eval(\@currentHref)}{eval(\@kernel@currentdata)}}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kernel@currentdata is a very strange name ... for starters it is not at all linked to label/ref from the name

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kernel@currentdata is a very strange name ... for starters it is not at all linked to label/ref from the name

well currently it doesn't hold anything. I didn't want to leave it empty so that one can test in latex-lab what is needed or useful here and added some name that reserves that for us. Do you a have idea?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe \ref@kernel@data
or \ref@kernel@currentdata

base/ltxref.dtx Outdated
Comment on lines 399 to 401
\protected@write\@auxout{}%
{\string\newlabel{#1}{{\@currentlabel}{\thepage}}}%
{\string\newlabel{#1}{{\@currentlabel}{\thepage}%
{\@currentlabelname}{\@currentHref}{\@kernel@currentdata}}}%
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is a full expansion (or any expansion for that matter) really desired for the fifth argument, or shouldn't tjat be passed unchanges (after one expansion through the .aux mechanism?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed: full expansion may be very dangerous, but one-level seems to be necessary.

Copy link
Member

@davidcarlisle davidcarlisle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reworded ltnews, pulling in Frank's comments at same time

@car222222
Copy link
Contributor

I think that the ltnews37 entry will need further revision as I spotted a remaining, possibly wrong, use of "argument"; but not essential right now.

@car222222
Copy link
Contributor

car222222 commented Dec 5, 2022

Ooops! It seems that I got misled about the data items never being used as arguments: the version of ltxref in this pull does seem to do so, for choosing one of them from the list.

I have seen a version of ltxref that avoids using them as arguments, by using some l3 commands for handling the data list as l3 items in an l3 token list (tl).
Good stuff! But where is that version?
Note that using this methodology, the number if data items can be increased with no need to change any the code for accessing existing items (there is no need for the fixed (currently three) number of @empty args).

@car222222
Copy link
Contributor

No, I did see the no-arg versions, they are staring at me in the current ltxref.dtx.
But it seems they are not currently used! It says:
"As the commands are now protected we also need expandable versions for use in \ifthenelse:"
These 'expandable versions' do not use the data as arguments to anything.

Therefore my suggestion is to always use (something like) these versions: at least, use their method for extracting the data, rather than using @firstoffive etc., with arguments).

I am unsure if it is worth making this change to ltxref at this stage.

@u-fischer
Copy link
Member Author

Therefore my suggestion is to always use (something like) these versions: at least, use their method for extracting the data, rather than using @firstoffive etc., with arguments).

The expandable version is a bit slower than \@setref, so what would be the gain to use it always?

@car222222
Copy link
Contributor

Yes, it would be a bit slower.

The main advantage is that it removes all of the dependence of the current code on the number of data items. More adaptable code; packages do not need to know how many data items must be or have been been stored.
But maybe saving time is more important.

@car222222
Copy link
Contributor

I still believe that we should not use the term "argument" in the documentation as, even if the implementation does not change and thus they do sometimes get used as arguments, they should be thought of as data items!

@u-fischer
Copy link
Member Author

packages do not need to know how many data items must be or have been been stored.

You are right, that is a gain. On the other side, after the unification, hopefully it will be always five and so packages don't need to know it either.

@FrankMittelbach
Copy link
Member

I still believe that we should not use the term "argument" in the documentation as, even if the implementation does not change and thus they do sometimes get used as arguments, they should be thought of as data items!

What is written to the aux file is \newlabel{..}{..}{..}{..}{..} that is a command with X number of arguments and it is processed by picking up the arguments. I don't see why you want to call them anything else at this stage.

@u-fischer
Copy link
Member Author

u-fischer commented Dec 5, 2022

What is written to the aux file is \newlabel{..}{..}{..}{..}{..}

öh no. \newlabel{key}{{1}{1}{}{}{}} is written. So we have always two arguments, but with a (until now) varying numbers of values in the second argument. (I have locally changed the text to use "argument" only for the outer things, and "values" for the things in the second argument, will push when I checked and updated testfiles)

@FrankMittelbach
Copy link
Member

ok, looking instead of incorrectly remembering would help ... I see. Yes then value is better as arguments. I seem to always confuse that the way toc data is passed around.

@car222222
Copy link
Contributor

Yes, I too get confused every time I get back to looking at it again!

Thanks, @u-fischer, for sorting out the language.

PS: I have now started looking at l3ref, which is even more brain-twisting!!

@zauguin zauguin force-pushed the develop branch 3 times, most recently from 1b35011 to 945edf6 Compare February 28, 2023 02:19
@car222222
Copy link
Contributor

Not sure if this the best place to put this comment?

AFAICT, the latest version of ltxref.dtx in branch newlabel still has this line (here, the date is definitely wrong, not sure about the version number?):

\ProvidesFile{ltxref.dtx} [2022/11/12 v1.1q LaTeX Kernel (Cross Referencing)]

@car222222
Copy link
Contributor

These three new slots/attributes(!) need some explanation
in this file:
{@currentlabelname}{@currentHref}{@kernel@reserved@label@data}

For the first two:
How are these used, where/how do they get values?
Where is their use explained (and they are defined)!?
Do they need, or have, accessor functions, like \ref \pageref: and where are these?
For the third, . . . ??? but say something, please!

Please also explain “something more about the hook”!
Why is it there?
Maybe give some example contents!
Or at least a reference to where its use is fully explained.

@u-fischer
Copy link
Member Author

For the first two:
How are these used, where/how do they get values?

the first has been introduced by nameref and contains typically the title. nameref patches sectioning commands to set it, but beamer and e.g. tabularray set it too. The second contains the destination/target name. It has been introduced by hyperref, but the latex-lab code sets it in various places too.

Both is explained in the ltnews entry.

Do they need, or have, accessor functions, like \ref \pageref: and where are these?

The first can be accessed by \nameref, the second has only internal accessor commands as it is typically used in links.

For the third, . . . ??? but say something, please!

That was discussed on the team list.

Please also explain “something more about the hook”!
Why is it there?

Because nameref added a similar hook, and hyperref needs it to change in certain cases the destination names. Beside this, if the PR is merged the showkeys package will use it. It also allows users to add more label commands, e.g. to record the position or the absolute page.

@car222222
Copy link
Contributor

These explanations all need to be in the file!

@u-fischer
Copy link
Member Author

These explanations all need to be in the file!

I added a note in ltxref.

@car222222
Copy link
Contributor

Good, thanks!

@u-fischer u-fischer merged commit c620e1a into develop May 16, 2023
60 checks passed
@car222222
Copy link
Contributor

car222222 commented May 17, 2023

These latest changes to ltnews37 by @FrankMittelbach are OK (and mostly good:-). But much of this should at some stage also be added to ltxref.
Explicit examples of current uses for the hook would also be useful in ltxref.

@car222222
Copy link
Contributor

This probably, at some stage, will need some test files.

@FrankMittelbach
Copy link
Member

This probably, at some stage, will need some test files.

I was briefly wondering about that but given that it affects quite a number of test files arealdy I thought that extra ones aren't needed. What should the test in addition?

@car222222
Copy link
Contributor

Yes, I can understand that the new 5-slot version may well be covered by existing tests. So, do some .tlg files need to be updated?

But the new hook is less likely to be covered. Also, and related, we anyway need examples of its use.

Copy link
Contributor

@muzimuzhi muzimuzhi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whether the new hook label is used inside a group or not is inconsistent among ltnews37 and ltxref.dtx.

It seems to me the functionality of showkeys package won't be influenced by the extra group since \SK@@label works globally.

Comment on lines +386 to +387
The hook is executed directly before the \cs{label} command writes to the \texttt{.aux} file
but \emph{after} the \cs{@bsphack} command has done its spacing magic and it is located \emph{inside} a group, thus its code only affects the write operation.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here in ltnews37 it reads new hook label is executed inside a group.

Comment on lines 404 to 411
\def\label#1{\@bsphack
%\begingroup
\UseHookWithArguments{label}{1}{#1}%
\protected@write\@auxout{}%
{\string\newlabel{#1}{{\@currentlabel}{\thepage}}}%
{\string\newlabel{#1}{{\@currentlabel}{\thepage}%
{\@currentlabelname}{\@currentHref}{\@kernel@reserved@label@data}}}%
%\endgroup
\@esphack}
Copy link
Contributor

@muzimuzhi muzimuzhi May 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But in new \label in ltxref.dtx, both \begingroup and \endgroup are commented out starting from commit 1494dd4 (revert hook name to label, 2023-05-12), hence \UseHookWithArguments{label}{1}{#1} is not executed inside a group.

Update: Resolved by #1067.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ups, good catch. I will add it (it is no longer strictly needed to protect the @bshack value but we decided to keep it).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, it would be good to change the wording in ltnews37 as follows, to be more precise:

. . . thus its code can be made local, so as to affect only the write operation.

@u-fischer u-fischer deleted the newlabel branch May 23, 2023 17:58
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

Successfully merging this pull request may close these issues.

None yet

5 participants