Image attributes

[mb21]

Implemented image attributes as discussed in #261. (closes #261)

The changes to pandoc-types can be found in this pull request.

All writers except the following have been properly adjusted:

• Docx (don't know whether it's possible to add image widths that are a percentage of the page width, otherwise it works)
• ODT (might work but not tested, also same here about percentage-widths)
• OpenDocument (what's the difference between this and ODT?)
• Custom

Would be great if someone familiar with those formats could adjust the writers.

The README and markdown reader have been adjusted as well. I chose to avoid renaming an existing extension flag, but maybe it would be better to rename the existing link_attributes to mmd_link_attributes, and my new common_link_attributes to link_attributes (or if you know of a better name than common, the basic syntax is shared by Markdown Extra, Kramdown and now Pandoc).

[mpickering]

Thank you got doing this, this kind of work it always very messy. I noticed a few changes from a quick glance which didn't seem to fit into the proposed changes. It's probably too late to change that now but it would be better in future to split them into separate commits.

Also it seems like there were some errors on the build, can you see what's causing these?

[mb21]

Sorry about the two unrelated changes (the one in Pretty.hs and the clarification of --columns and --no-wrap in the README). I thought of putting them in a separate commit a bit late, but I could still rebase I guess.

The two ICML writer commits are already in a separate pull, but I needed to build on them for the image dimensions stuff. The build is failing only on those commits, but not the final one, or am I looking at the wrong place?

[mpickering]

> .cabal-sandbox/bin/pandoc 
[link](url){.class}
<p><a href="url">link</a></p>
> pandoc
[link](url){.class}
<p><a href="url">link</a>{.class}</p>

I know this is intentional but I don't think that this is desirable. How difficult would it be to change?

[mb21]

It's all about the regLink and referenceKey functions in the markdown reader. I guess we could add a flag to them or temporarily disable the Ext_common_link_attributes option if we are calling the function for a link as opposed to for an image, both ways are a bit ugly though.

The alternative would probably be to add attributes to links as well and doing so sooner rather than later, fixing #170.

[mb21]

So is this pull mainly waiting for the other writers, or the question on how to handle links in the markdown reader or something else?

[jgm]

+++ mb21 [Jan 28 15 12:59 ]:

So is this pull mainly waiting for the other writers, or the question
on how to handle links in the markdown reader or something else?

I haven't really had time to look at it yet, and of course it's a complex issue, but it hasn't been forgotten about.

[mpickering]

This is a delicate change and there is still the outstanding issue I mentioned above?

[mb21]

sure, just wanted to know whether this is still on track for 1.14.

[MetaMemoryT]

👍

[linquize]

+1

[ibutra]

+1

[jgm]

This was a somewhat messy patch, with several different changes mixed together. I've tried to separate them out a bit, changed writerDpi to an Int, fixed tests, and rebase on top of master -- the result is pushed to the new-image-attributes branch.

I'm tempted to merge this now. It does seem a bit incomplete though -- it would make sense to add the Attr field to Link as well, so links and images behave similarly.

[timtylin]

Does this mean that the next release is going to be a 2.0?

[jgm]

+++ Tim T.Y. Lin [Apr 02 15 21:59 ]:

Does this mean that the next release is going to be a 2.0?

Why would it? With this change we'd need to bump to 1.14, but not to 2,
according to the package version policy.

[timtylin]

@jgm Sorry, I was thinking pandoc-types when I wrote that. Is it the policy to increment major version on that package when existing filters (as well as custom writers) may no longer work? I presume matching of Image types will fail.

[jgm]

+++ Tim T.Y. Lin [Apr 02 15 22:48 ]:

@jgm Sorry, I was thinking pandoc-types when I wrote that. Is it the policy to increment major version on that package when existing filters may no longer work? I presume matching of Image types will fail.

Policy is to increment the minor version on breaking API changes:
https://wiki.haskell.org/Package_versioning_policy

We incremented the minor version, e.g., when header attributes were
added.

I agree that this change could break existing filters. So, we might
want to consider a more conservative option. One more conservative
option would be to abuse the combination

Span attr [Image target label]

and have the writers and readers treat it as an image with attributes.
This feels slightly hacky but would have some advantages.

[jgm]

Another conservative approach (not requiring changes to pandoc-types) would be to overload the title field (which we already do to some extent for figures -- see how the writers treat a fig: prefix).

So, for example:

![alt text](url "title here height=78px width=32px")

Pandoc could be trained to look for these attributes in the title and strip them out.

[timtylin]

@jgm I was actually toying with the overloaded title approach for Scholdoc, and I liked it quite a lot. Forgot why I didn't make it official yet; probably laziness on my part.

---

I dug out my notes, here's what I listed as pros:

• automatic backward compatibility, not just for the AST, but also a clear default behavior for existing reader and writers (without just dropping the attr)
• a de facto syntax for reference-style images "for free", without toying with the Link parser or its type
• looks good in all existing MD processors, no dangling attr blocks

The obvious downside is that it's hacky, and that parsing for attrs in the title may become a bit ambiguous. The latter might be alleviated if the attrs must be in MDExtra-style curly braces.

[jgm]

@timtylin - on reflection I'm against the hackish title overloading idea. This is Haskell after all; we should be making type information explicit. And think about how it would make it harder to write filters if the filters have to parse the title for image size attributes!

The span overloading idea might still be worth considering, though.

Note that the issue of markdown syntax and the issue of how this is represented in the Pandoc structure are separate issues. Some of your considerations are about the syntax, not the types (e.g. "looks good in existing processors"). Of course, it's entirely possible to overload the title in the MD syntax while representing the attributes separately in the Image type constructor. I don't think I'd favor that, though.

[timtylin]

@jgm Makes sense that I'm thinking more about the syntax, since I've already abandoned any notion of backwards-compatible AST in Scholdoc, so no need for hacks there :)

I guess I'm just very interested in your philosophy w.r.t. these filter/writer-breaking type changes, i.e., whether you want to lump together breaking changes as much as possible or spread them out slowly. On a broader scope, the Pandoc ecosystem already has quite a long chain of things dangling precariously on each other: types, filters (modules for other languages), custom writers, and templates, that all needs some sort of version orchestration which isn't very visible at the moment. I don't know if it's a big problem yet, but some community effort in orchestrating all this might be prudent sooner than later.

[aaren]

@jgm @timtylin I think the fact that this sort of change would break existing filters is more of an argument for working on the pandoc architecture than not making the change in the properly typed way.

The changes to be made to pandoc would be

1. More typing, so that every component of an element has a type
2. Changing [] to {} in the AST so that element components are accessible by type name

For example, the existing Image type is

Image [Inlines] Target

and this currently comes out as this json:

{"t":"Image","c":[ [ {"t":"Str","c":"alt"} ], ["source","title"] ] }

but we could create an AltText type and have

Image AltText Target

and then have this json:

{"t":"Image","c":{"alt": [ {"t":"Str","c":"alt"} ], "target": {"fname": "source","title": "title"} } }

This changes the way we access the content of the image from

alt, (fname, title) = image['c']

to

alt = image['c']['alt']
target = image['c']['target']
fname = target['fname']
title = target['title']

This way you avoid breaking filters when you make a change like this (adding attributes). It would also make writing filters easier and clearer as you'd no longer have to remember the order of the types, just their names.

Of course this in itself would be a massive breaking change to pandoc-types but I think it would make the AST more robust in the future.

[lierdakil]

Additional spans could change output rendering, and I believe in many cases, filters may need changing even when using span-wrapping approach.

Since it'll be 1.14 anyway, might as well go all the way, I think.

P.S. adding attributes to links sounds like a good idea. Images and links should handle pretty similarly, I believe.

[jgm]

I think we need to discuss this on pandoc-discuss. I'll open up
a thread there.

+++ Tim T.Y. Lin [Apr 03 15 00:38 ]:

@jgm Makes sense that I'm thinking more about the syntax, since I've already abandoned any notion of backwards-compatible AST in Scholdoc, so no need for hacks there :)

I guess I'm just very interested in your philosophy w.r.t. these filter/writer-breaking type changes, i.e., whether you want to lump together breaking changes as much as possible or spread them out slowly. On a broader scope, the Pandoc ecosystem already has quite a long chain of things dangling precariously on each other: types, filters (modules for other languages), custom writers, and templates, that all needs some sort of version orchestration which isn't very visible at the moment. I don't know if it's a big problem yet, but some community effort in orchestrating all this might be prudent sooner than later.

---

Reply to this email directly or view it on GitHub:
#1806 (comment)

[lierdakil]

A thought occurred to me, w.r.t. conservative extension using spans.

While spans have meaning for many writers, we could add a new Block type constructor, e.g. Attrs Attr Block. I think that would require smaller changes to existing code, and would be somewhat backwards-compatible with existing filters at the cost of more awkward pattern matches (especially for the cases when we actually want attributes),

This will also potentially allow to use attributes with any block element with little penalty (if writers actually honor this is another matter), and simplify attribute parsing code somewhat (i.e. we could possibly add attribute parser to block parser)

This would make Header/Div/CodeBlock attributes obsolete though, so there's an open question on how should we handle those.

[linquize]

Since this issue is closed, which issue can we also refer to ?

[mb21]

the new pull is at #2351