quarto-dev / quarto-cli

Open-source scientific and technical publishing system built on Pandoc.
https://quarto.org
Other
3.85k stars 312 forks source link

typst callouts are bracketed #9816

Closed gordonwoodhull closed 4 months ago

gordonwoodhull commented 4 months ago

Somehow #9690 caused Typst callouts to have an extra level of square brackets, which are rendered as text

image

Sample of Typst output for dev-docs/feature-format-matrix/qmd-files/callout/document.qmd:

#block[
#callout(
body: 
[
[
A callout does not need a title.

]
]
, 
title: 
[
[
Warning
]
]
, 
background_color: 
rgb("#fcefdc")
, 
icon_color: 
rgb("#EB9113")
, 
icon: 
fa-exclamation-triangle()
)
]

Likely incorrect fix - I ran out of time for now and couldn't pinpoint why this is happening... I'm sure there is a better way to to fix this!

--- a/src/resources/filters/customnodes/callout.lua
+++ b/src/resources/filters/customnodes/callout.lua
@@ -391,8 +391,8 @@ function _callout_main()
     end

     local typst_callout = _quarto.format.typst.function_call("callout", { 
-      { "body", _quarto.format.typst.as_typst_content(callout.content) },
-      { "title", _quarto.format.typst.as_typst_content(title) },
+      { "body", callout.content },
+      { "title", title },
       { "background_color", pandoc.RawInline("typst", background_color) },
       { "icon_color", pandoc.RawInline("typst", icon_color) },
       { "icon", pandoc.RawInline("typst", "" .. icon .. "()")}
cderv commented 4 months ago

@gordonwoodhull I believe this happens because we already add the wrapping square brackets [ ... ] when we call _quarto.format.typst.function_call() with values that are tables (like callout.content here https://github.com/quarto-dev/quarto-cli/blob/92bfd31ce0c65afab38a0f7cc6f318d0d8ea858f/src/resources/filters/modules/typst.lua#L18-L21

So wrapping again in _quarto.format.typst.as_typst_content() creates too many square brackets

So either

diff --git a/src/resources/filters/customnodes/callout.lua b/src/resources/filters/customnodes/callout.lua
index e109802ce..2feda4752 100644
--- a/src/resources/filters/customnodes/callout.lua
+++ b/src/resources/filters/customnodes/callout.lua
@@ -389,10 +389,10 @@ function _callout_main()
     if title == nil then
       title = pandoc.Plain(_quarto.modules.callouts.displayName(callout.type))
     end
     local typst_callout = _quarto.format.typst.function_call("callout", {
-      { "body", _quarto.format.typst.as_typst_content(callout.content) },
-      { "title", _quarto.format.typst.as_typst_content(title) },
+      { "body", callout.content },
+      { "title", title },
       { "background_color", pandoc.RawInline("typst", background_color) },
       { "icon_color", pandoc.RawInline("typst", icon_color) },
       { "icon", pandoc.RawInline("typst", "" .. icon .. "()")}
diff --git a/src/resources/filters/modules/typst.lua b/src/resources/filters/modules/typst.lua
index 09a22e950..dde20b3cb 100644
--- a/src/resources/filters/modules/typst.lua
+++ b/src/resources/filters/modules/typst.lua
@@ -5,6 +5,14 @@
 -- this module is exposed as quarto.format.typst

 local function _main()
+  local function as_typst_content(content)
+    local result = pandoc.Blocks({})
+    result:insert(pandoc.RawInline("typst", "[\n"))
+    result:extend(quarto.utils.as_blocks(content) or {})
+    result:insert(pandoc.RawInline("typst", "]\n"))
+    return result
+  end
+
   local function typst_function_call(name, params, keep_scaffold)
     local result = pandoc.Blocks({})
     result:insert(pandoc.RawInline("typst", "#" .. name .. "("))
@@ -16,9 +24,7 @@ local function _main()
       elseif v.t == "RawInline" or v.t == "RawBlock" then
         result:insert(v)
       elseif type(v) == "userdata" or type(v) == "table" then
-        result:insert(pandoc.RawInline("typst", "["))
-        result:extend(quarto.utils.as_blocks(v) or {})
-        result:insert(pandoc.RawInline("typst", "]"))
+        result:extend(as_typst_content(v))
       else
         result:extend(quarto.utils.as_blocks({pandoc.utils.stringify(v)}) or {})
       end
@@ -55,14 +61,6 @@ local function _main()
     end
   end

-  local function as_typst_content(content)
-    local result = pandoc.Blocks({})
-    result:insert(pandoc.RawInline("typst", "[\n"))
-    result:extend(quarto.utils.as_blocks(content) or {})
-    result:insert(pandoc.RawInline("typst", "]\n"))
-    return result
-  end
-
   return {
     function_call = typst_function_call,
     as_typst_content = as_typst_content
cscheid commented 4 months ago

I'm sorry :( (I'm back as of today, so I'll fix this)

cscheid commented 4 months ago

This is annoying. What I would really like to have is some indication to whether a parameter that was passed to typst_function_call is "typst code" (which doesn't need brackets) or "typst content" (which does need brackets). But both of those can be represented as Pandoc data structures, and so we fundamentally can't know which is which in typst_function_call.

I think that means that callers of typst_function_call need to know the difference, and themselves handle the converting of "raw pandoc content" to "typst content" by adding brackets only when necessary.

gordonwoodhull commented 4 months ago

Interesting, I have been wondering what conventions we should use to know which "mode" of typst (code/content/math) is appropriate for a parameter.

I agree, expecting code mode for function parameters is probably apt.

It's a very clever system, not something I recall seeing in any other language.

cscheid commented 4 months ago

It's a very clever system, not something I recall seeing in any other language.

Yes, it indeed is!

We could have custom Quarto AST nodes for TypstCode and TypstContent, but I don't think the juice is worth the squeeze in this case. Those are really most useful if the nodes are going to reside in the document for a while, and I don't think that's going to be the case for us.

cscheid commented 4 months ago

I'd like to add regression tests here that aren't a snapshot test. This requires finding the text content in a .pdf file.

I first tested the Unix strings command, but it's (predictably) bad in this case, because the PDF format uses [ in non-text contexts.

I then tested pdftotext, which comes from poppler. That seems to work well enough. I'm going to create a verification function ensurePdfTextMatches to test against the output of pdftotext.

The question, then, is how to install this dependency in our test suite:

@cderv, what do you recommend?

cderv commented 4 months ago

For windows poppler is a good choice as it is available.

Best I know is to use Scoop manager to install this

scoop install poppler

then the pdftotext command will be on PATH.

We just need a specific action to install scoop and the tool.

I can do this.

PS: the windows version I know is here. https://github.com/oschwartz10612/poppler-windows

Another solution is to use R which is already available on runners with pdftools library which has functions based in libpoppler. https://docs.ropensci.org/pdftools/reference/pdftools.html

But this would mean running from Deno Rscript pdftotext.R with a custom script of ours to do the extraction. This would be a cross OS way to do it. but runs with R.