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

Tagging 809 dollardollar version #1696

Merged
merged 38 commits into from
Mar 10, 2025

Conversation

FrankMittelbach
Copy link
Member

@FrankMittelbach FrankMittelbach commented Mar 8, 2025

Internal housekeeping

Status of pull request

  • Feedback wanted
  • Under development
  • 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
  • Rollback provided (if necessary)?
  • ltnewsX.tex (and/or latexchanges.tex) updated

Version using \dollardollar@end, this is roughly how I would propose to implement it (of course, needs further documentation and rollback etc, but hopefully enough for feedback on code approach as such)

@davidcarlisle
Copy link
Member

davidcarlisle commented Mar 8, 2025

The code looks good in general, although with this in place how essential does it become to use \dollardollar@.. rather than $$ ?

grep -l   '[$][$]' */*.sty */*.cls 

shows a hundred top level packages and classes using $$ currently.

Plus of course an unknown number of documents.

ltnews should probably (again) tell people to use \[ \] not $$ $$

@FrankMittelbach
Copy link
Member Author

FrankMittelbach commented Mar 8, 2025

The code looks good in general, although with this in place how essential does it become to use \dollardollar@.. rather than $$ ?

grep -l   '[$][$]' */*.sty */*.cls 

shows a hundred top level packages and classes using $$ currently.

Plus of course an unknown number of documents.

ltnews should probably (again) tell people to use \[ \] not $$ $$

Not a problem as far as I can tell.

  • Top-level document usage of $$..$$even though officially not supported in LaTeX works both with and without tagging with this code (including the direct use of \eqno o \leqno).

  • If tagging is not used then $$..$$ in those package and classes work as before, the \dollardollar@end is only needed for tagging and these classes would need adjustments anyway for the math environments they define (they need registering and then you can at the same time replace $$with \dollardollar@end.

So bottom line I don't see any change to the current situation it might even improve the situation in one or the other case and with tagging it fixes a couple of limitations, e.g., you can now again specify a oneoff \belowdisplayskip inside a formula and it will get applied (even a negative one if for some reason you want to do that).

Copy link
Member

@josephwright josephwright left a comment

Choose a reason for hiding this comment

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

No major issues - just a couple of minor performance suggestions

@josephwright
Copy link
Member

ltnews should probably (again) tell people to use \[ \] not $$ $$

I see @FrankMittelbach has replied on the main thrust, but yes - we should make this abundantly clear

@FrankMittelbach
Copy link
Member Author

ltnews should probably (again) tell people to use \[ \] not $$ $$

I see @FrankMittelbach has replied on the main thrust, but yes - we should make this abundantly clear

maybe, but I'm close to calling it a defeat there. And we now fully support tagging it, so yes it has some odd side effects, but I'm not sure we get people to get rid of it and maybe it isn't really worth trying to.

What we can stress is that package and class files should not use it but use \dollardollar@begin ... \dollardollar@end, but whether that should go into ltnews I'm not sure. Anyway, if you want to write a para or two feel free.

# Conflicts:
#	required/latex-lab/changes.txt
#	required/latex-lab/testfiles-math-luatex/test-math-split-leqno-tbtags.tlg
#	required/latex-lab/testfiles-math-luatex/test-math-split-reqno-tbtags.tlg
#	required/latex-lab/update-math-all.sh
@FrankMittelbach
Copy link
Member Author

I have now added documentation of the code and rollback etc, so this is ready for a final review

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.

some minor comments inline

@FrankMittelbach FrankMittelbach mentioned this pull request Mar 9, 2025
6 tasks
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.

some minor comments inline

Comment on lines +775 to +776
\def\dollardollar@begin{$$}
\def\dollardollar@end{$$}
Copy link
Member

Choose a reason for hiding this comment

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

the names are OK or you could go for a name based on usage rather than the definition say \mathdisplay@begin \mathdisplay@end but really they have to be $$ not any morecomplec code you might want to start or end a display, so I think the current names are OK

@car222222
Copy link
Contributor

@FrankMittelbach wrote:
added another section in ltnews41 to discuss $$, please have a look.

Well, I can look at it, but it is not possible to comment directly on it in the current setup.

Do you, perhaps, need to request a review?
Any other possibilities?

@car222222
Copy link
Contributor

I now discovered that I can piggyback on David's more recent review.

@FrankMittelbach
Copy link
Member Author

@FrankMittelbach wrote: added another section in ltnews41 to discuss $$, please have a look.

Well, I can look at it, but it is not possible to comment directly on it in the current setup.

No idea if there are restrictions if you use the ipad app but on the website you can just start another review if you start from the "Files changed" view.

Do you, perhaps, need to request a review? Any other possibilities?

anyway, I requested another review from you in case that solves the problem.

@car222222
Copy link
Contributor

@FrankMittelbach wrote:
you can just start another review if you start from the "Files changed" view.

Yes, that seems to work as expected again now.
Not sure why it did not offer me a review opportunity before!

@FrankMittelbach FrankMittelbach merged commit 7bc038c into develop Mar 10, 2025
90 checks passed
@josephwright josephwright deleted the tagging-809-dollardollar-version branch March 10, 2025 12:13
Copy link
Contributor

@car222222 car222222 left a comment

Choose a reason for hiding this comment

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

Wow, What an enormous task!
Not sure if I found everything, but I tried.

@car222222
Copy link
Contributor

Sorry this took so long: 57 entries!

I have never seen so many tests in one place before. Well done, the testers.

@FrankMittelbach
Copy link
Member Author

Wow, What an enormous task! Not sure if I found everything, but I tried.

you found a lot; thanks. Added most of them, with a few questions/comments left.

@car222222
Copy link
Contributor

I have now tried to add some more ideas and reply to Frank's comments.

But I may not have been able to save them, so will need to start over some time,

@car222222
Copy link
Contributor

Looks OK: they are in the emails even though I cannot locate them within the GitHub PR itself: strange beast!

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.

5 participants