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

Load hyperref before any \newtcolorbox #14

Merged
merged 1 commit into from
Feb 18, 2023

Conversation

muzimuzhi
Copy link
Contributor

From the testfile one can see the previous hyperref target names like tcb@cnt@pabox.1 are not unique throughout the document, for example the first pabox in section 1 and section 2 will use the same target name.

Loading hyperref before any \newtcolorbox and friends fixes this problem. Now the hyperref target are named like tcb@cnt@pabox.1.1 (<box name>.<sec num>.<box num>), which reflect the setting number within=section.

See a related issue in T-F-S/tcolorbox#135.

PS: Since \tcbuselibrary may load packages as well, just in case, loading of hyperref is moved after \tcbuselibrary.

@gusbrs
Copy link
Owner

gusbrs commented Feb 18, 2023

Hi @muzimuzhi , thank you. And you're right of course. I also have the habit of keeping setup for a package close to the package call, but that sometimes bites us. :-)

I'll merge, of course, and I'll take a look at the failing test, which considering your care in the PR is probably from the xetex engine. Btw, do you mind if, in the process, I drop the "section 2" in the test file? I usually keep the how-tos regression tests in sync with the actual How-to in the manual. I do get you were trying to make your point, and you did, but the effect of the change is already clear in the first section's anchors. WDYT?

@muzimuzhi
Copy link
Contributor Author

muzimuzhi commented Feb 18, 2023

Btw, do you mind if, in the process, I drop the "section 2" in the test file?

I'll delete it, update the testfiles and push a new commit.

BTW, a "section 2" test allows me to check the output by clicking links in the output PDF. 😄

@muzimuzhi
Copy link
Contributor Author

Commits and changes are messed. Will tidy them.

@muzimuzhi
Copy link
Contributor Author

Commits and changes are messed. Will tidy them.

done. Sorry it results in a force push.

@gusbrs gusbrs merged commit a50d6c6 into gusbrs:main Feb 18, 2023
@gusbrs
Copy link
Owner

gusbrs commented Feb 18, 2023

Merged. And I'll cut a release because, though this is a small change, I have nothing lined up at the moment, and I also see no reason to hold this for an indefinite amount of time (and keep spreading a bad practice in the meantime).

Thank you very much @muzimuzhi !

BTW, a "section 2" test allows me to check the output by clicking links in the output PDF. 😄

Well, yes, the proof of the pudding is in the eating. 😄
With time, I'm getting a little better at reading these logs, but whenever I create a new test file what I check first and foremost is the PDF. And I suspect I'll keep doing this for a long time. Sometimes, I'll create a dummy extra end page so that there's enough room to scroll, and I can "be sure the anchor is right". 😉

@muzimuzhi muzimuzhi deleted the postpone-newtcolorbox branch February 18, 2023 12:43
@muzimuzhi
Copy link
Contributor Author

muzimuzhi commented Feb 18, 2023

Sometimes, I'll create a dummy extra end page so that there's enough room to scroll, and I can "be sure the anchor is right". 😉

+1 🎉

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

2 participants