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

DocBook writer: output identifiers of figure blocks #8608

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

Conversation

Flupp
Copy link
Contributor

@Flupp Flupp commented Feb 9, 2023

@Reviewer: I am not sure if the id needs to be prefixed with writerIdentifierPrefix opts as done in line 190 (this is the only place where the prefix is added AFAICT).

@Flupp
Copy link
Contributor Author

Flupp commented Feb 9, 2023

Note: In contrast to the DocBook writer, the DocBook reader already supports the identifier.

Behavior before the change:

$ pandoc -f Markdown -t docbook <<<'![caption text](file.svg){#testid}' | pandoc -f docbook -t markdown 
![caption text](file.svg "fig:")

Behavior after the change:

$ pandoc -f Markdown -t docbook <<<'![caption text](file.svg){#testid}' | pandoc -f docbook -t markdown
![caption text](file.svg "fig:"){#testid}

@jgm
Copy link
Owner

jgm commented Feb 9, 2023

Docs for --id-prefix say "Specify a prefix to be added to all identifiers and internal links in HTML and DocBook output," so yes, I'd say it should be added in this case too.

@Flupp
Copy link
Contributor Author

Flupp commented Feb 9, 2023

I’ve added the prefix. However, now I seriously wonder if the prefix is maybe also missing at other places in the DocBook writer. The following

:::{#bar}
bar div
:::

:::{#sec .section}
sec div
:::

[foo link](#foo)

[span]{#spanid}

![caption text](file.svg){#testid}

translates via pandoc --id-prefix=prefix- -f Markdown -t docbook (including this change) to

<para xml:id="bar">
  bar div
</para>
<para xml:id="sec">
  sec div
</para>
<para>
  <link linkend="prefix-foo">foo link</link>
</para>
<para>
  <anchor xml:id="spanid" />span
</para>
<figure xml:id="prefix-testid">
  <title>caption text</title>
  <mediaobject>
    <imageobject>
      <imagedata fileref="file.svg" />
    </imageobject>
    <textobject><phrase>caption text</phrase></textobject>
  </mediaobject>
</figure>

Note that prefix- is not added to every id.

@jgm
Copy link
Owner

jgm commented Dec 6, 2023

Hi, I'm sorry I let this languish so long.

You're right that the prefix isn't added everywhere it should be. Perhaps that could be addressed in a separate PR. Or, feel free to address it here.

I'll add one substantive comment to your code above.

Comment on lines +347 to +349
version <- ask
let idAttr = [ (idName version, writerIdentifierPrefix opts <> id')
| not (T.null id') ]
Copy link
Owner

Choose a reason for hiding this comment

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

We have the idAndRole function, and I think it would make sense to use it here (and anywhere else where we'd manually be constructing an id attribute).

Note also that the idAndRole function doesn't use the version-dependent idName. That should be fixed.

@jgm
Copy link
Owner

jgm commented Dec 6, 2023

It would also be great to have a test. Simplest way is a "command test" called test/command/8608.md. See the other files in that directory for examples.

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.

2 participants