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

implement lof and lot for docx #10029

Merged
merged 1 commit into from
Sep 22, 2024
Merged

implement lof and lot for docx #10029

merged 1 commit into from
Sep 22, 2024

Conversation

acxz
Copy link
Contributor

@acxz acxz commented Jul 25, 2024

lof and lot have been implemented for LaTeX for a while now.
This PR adds support for lof and lot in docx, closing #8245

In doing so, lof and lot are also exposed to the command line and corresponding documentation is added.

Please if you could review any instances where I have missed documentation regarding lof and lot, I would much appreciate it, @jgm. I did my best, but I would appreciate another pair of eyes.

A specific question:
Should I be adding in lof/lot and lof-title/lot-title here based on the changes that this PR has?

pandoc/MANUAL.txt

Lines 3337 to 3344 in 2f36df6

`toc`
: non-null value if `--toc/--table-of-contents` was specified
`toc-title`
: title of table of contents (works only with EPUB,
HTML, revealjs, opendocument, odt, docx, pptx, beamer, LaTeX).
Note that in docx and pptx a custom `toc-title` will be
picked up from metadata, but cannot be set as a variable.

Note: the latex writer prob needs to be modified to use these command line args. This wasn't obvious for me to do. I'll need some help if this change needs to be done in this PR. See: 64c7a0a for more info.

Also note that the default.openxml template needs to be modified as well.
(Edit: has been modified)

Example:

Pandoc Markdown File:

---
title: hello
author: Jane Doe
toc: true
lof: true
lot: true
---

# A

Blah

![Image A](placeholder.png)

|a|b|
|-|-|
|c|d|

: Table A

# B

Blah

![Image B](placeholder.png)

|a|b|
|-|-|
|c|d|

: Table B

Output Docx File:
image
image

@acxz
Copy link
Contributor Author

acxz commented Jul 26, 2024

@iandol if you get the chance I would love for you to try this PR out.

@iandol
Copy link
Contributor

iandol commented Jul 27, 2024

I don't have a build environment for haskell, so once there is a build for macOS I can test...

Copy link
Owner

@jgm jgm left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good. I've made some minor suggestions.

src/Text/Pandoc/Writers/Docx.hs Outdated Show resolved Hide resolved
src/Text/Pandoc/Writers/Docx.hs Outdated Show resolved Hide resolved
src/Text/Pandoc/Writers/Docx/OpenXML.hs Show resolved Hide resolved
src/Text/Pandoc/Writers/Docx/OpenXML.hs Outdated Show resolved Hide resolved
@acxz acxz force-pushed the docx-lof-lot branch 6 times, most recently from 24eef74 to 7a3b5e9 Compare July 31, 2024 03:11
@jgm
Copy link
Owner

jgm commented Aug 1, 2024

lotTitle <- T.text <$> translateTerm Term.ListOfTables

is raising an error. translateTerm will return a Text. If you want lotTitle to be a Text, you can just do

lotTitle <- translateTerm Term.ListOfTables

If you want lotTitle to be an Inlines, you'll have to do

import qualified Text.Pandoc.Builder as B
...
lotTitle <- B.text <$> translateTerm Term.ListOfTables

But I think it will work for you if it's just a Text, because you just need to stuff it into an XML attribute.

@acxz acxz force-pushed the docx-lof-lot branch 5 times, most recently from 73fc6cf to 75d979c Compare August 11, 2024 18:50
@acxz
Copy link
Contributor Author

acxz commented Aug 12, 2024

@jgm Again thanks for your help on this.

Neither of those options will work as we don't want a Text or B.Inlines, we need an [Inline]. See the above conversation in your PR review for the kind of errors we are facing. Can you provide more feedback on how to get an [Inline]?

As mentioned in the review comments, the solution is to pass the B.Inlines to B.toList, i.e. ... B.toList <$> B.text <$> ...

@acxz acxz force-pushed the docx-lof-lot branch 3 times, most recently from 0646b2e to f9b0459 Compare September 7, 2024 16:55
@acxz
Copy link
Contributor Author

acxz commented Sep 7, 2024

Now that we've iterated on the suggestions. Is there anything else left in this PR, before we add the snippet mentioned in the OP regarding lof and lot in the template repo?

Also would like a confirmation if any more changes are required in the docs/manual due to this change?

There is also the latex issue (i.e. latex using the command line options) as well, but that is prob best handled in a follow up PR.

Again thanks for reviewing this work @jgm

@jgm
Copy link
Owner

jgm commented Sep 9, 2024

You've tested this and the lot and lof appear correctly in Word documents?

@jgm
Copy link
Owner

jgm commented Sep 10, 2024

I tried to check this out locally, but it seems it needs to be rebased on main.

@acxz
Copy link
Contributor Author

acxz commented Sep 15, 2024

@jgm rebased! The output you see in the OP is the result of this PR. I'd like for you to try it locally (if you can) before we merge it in however, just so we have another user as a datapoint.

@jgm
Copy link
Owner

jgm commented Sep 16, 2024

Thanks. Tested it locally and it works well. But we do need to make sure that it works the same for LaTeX and ConTeXt, too. Here's how to do it:

diff --git a/MANUAL.txt b/MANUAL.txt
index ba965cf37..2fd2ce152 100644
--- a/MANUAL.txt
+++ b/MANUAL.txt
@@ -872,18 +872,18 @@ header when requesting a document from a URL:
 `--lof[=true|false]`, `--list-of-figures[=true|false]`
 
 :   Include an automatically generated list of figures (or, in
-    the case of `latex` or `docx`, an instruction to create
-    one) in the output document. This option has no effect
-    unless `-s/--standalone` is used, and it only has an effect
-    on `latex` or `docx` output.
+    some formats, an instruction to create one) in the output
+    document. This option has no effect unless `-s/--standalone`
+    is used, and it only has an effect on `latex`, `context`, and
+    `docx` output.
 
 `--lot[=true|false]`, `--list-of-tables[=true|false]`
 
 :   Include an automatically generated list of tables (or, in
-    the case of `latex` or `docx`, an instruction to create
-    one) in the output document. This option has no effect
-    unless `-s/--standalone` is used, and it only has an effect
-    on `latex` or `docx` output.
+    some formats, an instruction to create one) in the output
+    document. This option has no effect unless `-s/--standalone`
+    is used, and it only has an effect on `latex`, `context`, and
+    `docx` output.
 
 `--strip-comments[=true|false]`
 
diff --git a/src/Text/Pandoc/Writers/ConTeXt.hs b/src/Text/Pandoc/Writers/ConTeXt.hs
index 6c4bab355..9e35c803b 100644
--- a/src/Text/Pandoc/Writers/ConTeXt.hs
+++ b/src/Text/Pandoc/Writers/ConTeXt.hs
@@ -104,6 +104,8 @@ pandocToConTeXt options (Pandoc meta blocks) = do
   mblang <- fromBCP47 (getLang options meta)
   st <- get
   let context =   defField "toc" (writerTableOfContents options)
+                $ defField "lof" (writerListOfFigures options)
+                $ defField "lot" (writerListOfTables options)
                 $ defField "placelist"
                    (mconcat . intersperse ("," :: Doc Text) $
                      take (writerTOCDepth options +
diff --git a/src/Text/Pandoc/Writers/LaTeX.hs b/src/Text/Pandoc/Writers/LaTeX.hs
index 04e0ab3db..edbc40aaf 100644
--- a/src/Text/Pandoc/Writers/LaTeX.hs
+++ b/src/Text/Pandoc/Writers/LaTeX.hs
@@ -194,6 +194,8 @@ pandocToLaTeX options (Pandoc meta blocks) = do
                     $ lookupMetaInlines "nocite" meta
 
   let context  =  defField "toc" (writerTableOfContents options) $
+                  defField "lof" (writerListOfFigures options) $
+                  defField "lot" (writerListOfTables options) $
                   defField "toc-depth" (tshow
                                         (writerTOCDepth options -
                                               if stHasChapters st

Also, I think that before merging it might make sense to squash everything into a single commit.

@acxz acxz force-pushed the docx-lof-lot branch 3 times, most recently from 188b4ba to 695c4f8 Compare September 21, 2024 22:28
@acxz
Copy link
Contributor Author

acxz commented Sep 21, 2024

@jgm done!

Copy link
Owner

@jgm jgm left a comment

Choose a reason for hiding this comment

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

I think it looks great! I'm sorry, one more request. Can you update your commit message so that it indicates clearly what has changed? I will need this later to add these items to the changelog. It is especially important to note the new command-line options and any API changes (new fields in WriterOptions and Opt for example).
API chages should be marked as [API change].

@acxz
Copy link
Contributor Author

acxz commented Sep 22, 2024

Of course, I want to make sure this PR is done properly.

Changed the commit message. Let me know if there is anything else.

use lof and lot options in latex and context
implement lof and lot for docx

[command line] add `--lof[=true|false]`, `--list-of-figures[=true|false]`
[command line] add `--lot[=true|false]`, `--list-of-tables[=true|false]`

[API change] `WriterOptions`: add property `list_of_figures`
[API change] `WriterOptions`: add property `list_of_tables`

[API change] `Options`: add `writerListOfFigures :: Bool`
[API change] `Options`: add `writerListOfTables :: Bool`

[API change] `App/Opt`: add `optListOfFigures :: Bool`
[API change] `App/Opt`: add `optListOfTables :: Bool`

Co-authored-by: John MacFarlane <jgm@berkeley.edu>
@jgm jgm merged commit ca34894 into jgm:main Sep 22, 2024
8 of 12 checks passed
@jgm
Copy link
Owner

jgm commented Sep 22, 2024

Thanks! I rewrote the commit message a bit to fit what I like for the changelog.

@acxz
Copy link
Contributor Author

acxz commented Sep 22, 2024

Tried looking at your previous commits to get a feel for it, but what you wrote is cleaner.

Thanks for taking the cycles working with me to get this merged in, @jgm! I really appreciate it.
Whenever I have a feature I'd like in pandoc, I'd love to come back and contribute again!

@acxz acxz deleted the docx-lof-lot branch September 22, 2024 22:26
@jgm
Copy link
Owner

jgm commented Sep 23, 2024

Whenever I have a feature I'd like in pandoc, I'd love to come back and contribute again!

I hope you will.

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.

3 participants