Skip to content

fix: Improve handling of listoflistings commands in LaTeX metadata injection #13139

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.

Sign up for GitHub

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

fix: Improve handling of listoflistings commands in LaTeX metadata injection #13139

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 7 additions & 8 deletions src/resources/filters/crossref/meta.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ function crossrefMetaInject()
return trim(pandoc.write(pandoc.Pandoc(quarto.utils.as_blocks(inlines)), "latex"))
end
metaInjectLatex(meta, function(inject)

inject(usePackage("caption"))

inject(
Expand All @@ -21,10 +20,10 @@ function crossrefMetaInject()
maybeRenewCommand("tablename", as_latex(title("tbl", "Table"))) ..
"}\n"
)

if param("listings", false) then
inject(
"\\newcommand*\\listoflistings\\lstlistoflistings\n" ..
"\\@ifundefined{listoflistings}{\\newcommand*\\listoflistings\\lstlistoflistings}{}\n" ..
Comment on lines -27 to +26
Copy link
Collaborator

Choose a reason for hiding this comment

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

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. 😓

Copy link
Collaborator Author

@mcanouil mcanouil Jul 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@cderv cderv Jul 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

"\\AtBeginDocument{%\n" ..
"\\renewcommand*\\lstlistlistingname{" .. listOfTitle("lol", "List of Listings") .. "}\n" ..
"}\n"
Expand All @@ -38,7 +37,8 @@ function crossrefMetaInject()
)

inject(
"\\newcommand*\\listoflistings{\\listof{codelisting}{" .. listOfTitle("lol", "List of Listings") .. "}}\n"
"\\@ifundefined{listoflistings}{\\newcommand*\\listoflistings{\\listof{codelisting}{" ..
listOfTitle("lol", "List of Listings") .. "}}}{}\n"
Copy link
Collaborator

Choose a reason for hiding this comment

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

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)

)
end

Expand All @@ -61,24 +61,23 @@ function crossrefMetaInject()
"\n'', 'none', ':', 'colon', '.', 'period', ' ', 'space', and 'quad'")
end
end

local theoremIncludes = theoremLatexIncludes()
if theoremIncludes then
inject(theoremIncludes)
end
end)

return meta
end
}
end

function maybeRenewCommand(command, arg)
function maybeRenewCommand(command, arg)
local commandWithArg = command .. "{" .. arg .. "}"
return "\\ifdefined\\" .. command .. "\n " .. "\\renewcommand*\\" .. commandWithArg .. "\n\\else\n " .. "\\newcommand\\" .. commandWithArg .. "\n\\fi\n"
end


-- latex 'listof' title for type
function listOfTitle(type, default)
local title = crossrefOption(type .. "-title")
Expand Down
Loading