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

typst callouts are bracketed #9816

Closed
gordonwoodhull opened this issue May 31, 2024 · 7 comments · Fixed by #9827
Closed

typst callouts are bracketed #9816

gordonwoodhull opened this issue May 31, 2024 · 7 comments · Fixed by #9827
Assignees
Labels
bug Something isn't working callouts Issues with Callout Blocks. typst
Milestone

Comments

@gordonwoodhull
Copy link
Contributor

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 .. "()")}
@mcanouil mcanouil added bug Something isn't working callouts Issues with Callout Blocks. typst labels May 31, 2024
@cderv
Copy link
Collaborator

cderv commented May 31, 2024

@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

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", "]"))

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

So either

  • we revert for this part, not calling the function
  • or we tweak _quarto.format.typst.function_call to use _quarto.format.typst.as_typst_content because, in fact this is the same things
    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
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
Copy link
Collaborator

cscheid commented May 31, 2024

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

@cscheid cscheid self-assigned this May 31, 2024
@cderv cderv added this to the v1.5 milestone May 31, 2024
@cscheid
Copy link
Collaborator

cscheid commented May 31, 2024

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
Copy link
Contributor Author

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
Copy link
Collaborator

cscheid commented May 31, 2024

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
Copy link
Collaborator

cscheid commented May 31, 2024

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:

  • on OS X, brew install poppler
  • on Ubuntu (our CI), apt-get install poppler-utils
  • on Windows, ... ?

@cderv, what do you recommend?

@cscheid cscheid mentioned this issue May 31, 2024
@cderv
Copy link
Collaborator

cderv commented May 31, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working callouts Issues with Callout Blocks. typst
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants