fix: Improve handling of listoflistings commands in LaTeX metadata injection

[mcanouil]

Enhance the LaTeX metadata injection process by ensuring proper handling of the listoflistings command. This change prevents potential issues when the command is already defined, improving overall robustness.

Waiting for #13137 reprex that could be used as an unit test.

[posit-snyk-bot]

🎉 Snyk checks have passed. No issues have been found so far.

✅ security/snyk check is complete. No issues have been found. (View Details)

✅ license/snyk check is complete. No issues have been found. (View Details)

[cderv]

Just sharing thoughts about this kind of tweaks based on experience in LaTeX template for R Markdown world

[cderv]

IIUC, this would mean that we would not define the listoflistings function in Quarto if already available?

So \listoflistings would be the one from the package that provides it right?

I wonder if this is a good idea to rely on an external function if we really depend on this function. So I think we need to check if we use directly \listoflisting and if so how.

I guess I am wondering if we shoud do something like

\@ifundefined{listoflistings}
  {\newcommand*\listoflistings{\listof{codelisting}{...}}}
  {\renewcommand*\listoflistings{\listof{codelisting}{...}}}

I don't know the minted package used in the OP that this PR tries to solve. This is worth testing and understanding to avoid potentially more breakage by trying to solve one 😅 That happens to me sometimes and this is not ideal. 😓

[mcanouil]

If you want Quarto to ensure its definition is to be used, then you can't use newcommand, you need at the very least \renewcommand. If a package or user define the command, then Quarto will crash 100% of time with no possibility to work around.

Note that \@ifundefined is not a new thing, it's already used in many places, as \renewcommand is.
I believe here the use of \newcommand is an oversight based on the hypothesis that listoflistings is never defined.

The main goal of the PR is not to support the "minted" package but to ensure Quarto is not failing that hard either via \@ifundefined or \renewcommand.

[mcanouil]

None of the tests failed with / because of that change, but coverage is not 100%.

[cderv]

Thanks for the additional information. It seems I was not clear in my thoughts.

I agree with the change with undefined. I am wondering if we should use \renewcommand in the else case, and not rely on the existing function brought by another package.

I wonder what happens if a different function that ours is used, especially regarding code listing feature.

Just bringing ideas around the table - If you got this covered already, awesome!

None of the tests failed with / because of that change, but coverage is not 100%.

We need to check if we have tests that cover this. Indeed, having the reprex will help, but searching for an existing one that could trigger \listoflisting is a first step.

Some features like this one may not be well tested especially on edge case like interaction with other package for sure.

The main goal of the PR is not to support the "minted" package but to ensure Quarto is not failing that hard either via @ifundefined or \renewcommand.

Maybe I did not get that right. I'll try out with minted, and see if I can come up with a dummy example. It felt to me @ifundefined we then no set our own version, and so indeed to error hard if already defined, but possibly hidding with silent problem using another function.

I have been beaten by this kind of this with LaTeX in the past. Sharing experience.

[mcanouil]

I agree with the change with undefined. I am wondering if we should use \renewcommand in the else case, and not rely on the existing function brought by another package.

I wonder what happens if a different function that ours is used, especially regarding code listing feature.

I agree, that's why I'm waiting for the reproducible example that could serve to properly test this. I don't know "minted" at all.
I only reversed engineered the error by looking at where the command was defined.

[cderv]

Same here. It may be more applicable as this is for Float and this will make the crossreference system for code listing to work. If \listoflisting is not doing \\listof{codelisting} as we defined it, then maybe this will be more a problem than trying to support minted package.

So \renewcommand may be required to overwrite for crossref to work (and not opted-out)