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

[feature] Add =image to perl pod #18169

Open
dk opened this issue Sep 26, 2020 · 27 comments
Open

[feature] Add =image to perl pod #18169

dk opened this issue Sep 26, 2020 · 27 comments

Comments

@dk
Copy link

dk commented Sep 26, 2020

I propose an extension that, in spirit of "easy things easy", makes embedding of images in pod, well, easy.
Currently it is possible with a lots of ugly hacks, but the worst is especially knowing the formatted beforehand - f.ex. to show such a pod in github or metacpan requires magic like

=for html <p><img src="...png">

where you need to know that it is html, for example.

(see https://metacpan.org/pod/distribution/Prima/pod/Prima/Drawable.pod#Font-ABC-metrics as an example. It works but it is ugly).

In my experience an image tag should be able to do the following:

  1. display a locally installed image
  2. display a remote image (as in "img src")
  3. display a text fallback (as in "img alt" or similar)
  4. display nothing

An =image tag, in full form, would be thus used like this, for example:

=image begin

=image local "lib/graphic.png" (get searched in @INC for example)

=image remote "http://github.com/.../graphic.png"

^ y
|       *
| *
|------------> x

graphic for man pages

=image end

and also a short syntax for 90% of cases just as

=image "http://github.com/.../graphic.png"

This will break backward compatibility, but I'm decidedly not proposing a weaker but compatible option, such as f.ex.

=begin image 

=for image 

=end image

because I think that images should no longer be hacks but rather 1st class citizens.

Let's discuss this

@Grinnz
Copy link
Contributor

Grinnz commented Sep 26, 2020

I appreciate the design proposed, and =for html is awkward to use for this (though in practice HTML is the only medium that actually renders images), but I would much rather convince Pod::Simple::XHTML to recognize =for image which does not need to change the spec or break compatibility, and only would affect formatters that care about it.

@xsawyerx
Copy link
Member

xsawyerx commented Sep 26, 2020

General comments:

  • I wouldn't separate local and remote. I think these create confusion. Instead, the parsers should use a defined location for it. (For example, the current directory where this is rendered, as in HTML.)
  • I wouldn't put images in @INC. I absolutely abhor that. (I won't be making the call on this at the moment, just saying I personally really dislike this idea.) Then again, we might use __FILE__ to make this deterministic. But I hate putting non-Perl in a library directory.
  • Perhaps we should somehow connect this with some mechanism like File-ShareDir which tries to "correctly" (for different values of "correct") detect the location of files for a module.

I have a lot of questions that come up:

  • Do we care about width and height attributes for images?
  • Do we care about the title attribute for images?
  • HTML5 provides figcaption and figure. What about them?
  • HTML5 provides picture. What about it?
  • HTML5 supports other multimedia elements like video. Do we intend to natively support them too or limit our support for img?

My biggest concern here is that it will promote a single, specific multimedia element as first-class in the POD specification. There are both additional types of multimedia elements, as well as elements that can replace this one. So why this one? What about them?

My second biggest concern is that we might be growing here into an attempt to recreate arbitrarily complicated HTML using POD and I'm not sure we'll be able to do it. (It might not be exactly "arbitrarily complicated," but it sure can be. The example in the ticket includes <p> tags, so we're definitely trying to replicate paragraphs along with the <img> tag.)

@dk
Copy link
Author

dk commented Sep 27, 2020

@Grinnz I agree that breaking back compatibility is a thing better to avoid, but I also don't see a problem introducing it as it happened already with =head3 and =head4 back then. If old formatters emit a warning f.ex. "tag =image unknown" but will continue working that is even better I think, that way they send a signal to the user that update is needed

@dk
Copy link
Author

dk commented Sep 27, 2020

@xsawyerx I see a need for local images, f ex when converting to latex or pdf, or using an offline documentation. I agree that local and remote subtags are somewhat inelegant, however I seriously doubt that one can (and for that matter, should) enforce a html-like schema where both local and remote images can be defined as a single unambiguous URI. F ex metacpan or github, when rendering a pod file as html, won't extract referenced images that are found correspondingly either in archive or in a repository, that might have different paths compared to where an image is to be installed locally.

What I meant by INC is to define a local storage for a module, which is in my eyes falls quite naturally in $SITEPERL/lib/$MODULE . Many modules already install pod files there - would you consider that a malpractice too? I don't mind though extending =image with location rules other than INC, as long as this is standartized; I wish though this could be done easily because when one runs "perldoc foo", foo.pod is searched exactly in INC.

  • width and height: probably we won't care, but we may leave a possibility to extend the syntax for that, without breaking back compatibility, =image size 10x20 for example or similar. This could open a pandoras' box though as we would wish then size="80%" and width="10em".

  • title and/or figcaption: could be a good idea actually; add a =image title subtag that gets converted to <figcaption> in a html formatter?

  • picture: that looks like a candidate for =for html5, not for =image. I've seen websites with scientific articles that, when f ex viewed on mobile and a figure is too small to fit, expand it to a larger image popup when clicked on. The html formatter could be doing smart tricks like this too.

  • video: also answering to "single, specific multimedia element". I see =image as an essential tool for including not only in html, but latex and pdf, and generally things that can be printed on paper, i e "documentation" as in P.O.Documentation. In my optics it falls more in the same category as B<> and I<> rather than a multimedia element (which is not 100% correct, yes, but that's for illustrating my point of view). So, no =video as it cannot be printed.

However I very much agree that =image and potential friends like =video could indeed begin to grow into an unmanageable set of features. That's why I'd rather keep =image spartan, confining it to the 4 options I outlined in the ticket: local, remote, text fallback, and no text fallback. Ah yes, a figcaption too; figcaption is nice.

@haarg
Copy link
Contributor

haarg commented Sep 27, 2020

Previous discussion: metacpan/metacpan-web#1965

@xsawyerx
Copy link
Member

Are you suggesting that we decouple the concept of an image from HTML's img property?

If our image property would correlate to the img property in HTML, we would first need to support all the fields it has in a consistent manner (local vs. remote is managed using the implicit URI spec in browsers), we would need to figure out how to support the figure and figcaption options - oh, and maybe srcset too.

If we're not coupling the suggested image property from HTML's img property, we would still need a rich-enough specification to allow translating it into HTML. I think it makes sense for us to do this, but it might be a bigger undertaking to determine the superset of options.

It's worthwhile researching how this is handled in other formats. We could try to steal from TeX, but here lie dragons. TeX is far more capable than Pod and we might be stea^Winspired by TeX but still be unable to properly import the full capability it has (coughPerl6cough). In Markdown, there is a syntax for images but I fear it suffers from the same problems of elevating certain HTML elements to first-class.

Perhaps we could just follow Markdown in minimal support for images and the desire for more compatibility with Markdown to introduce it.

@dk
Copy link
Author

dk commented Sep 27, 2020

I guess it was implicit, but yes, I do suggest to not copycat html's img tag and its properties. Moreover, I don't suggest to dive into devising a rich-enough specification, as this is going to open for 1) endless discussions and 2) will be too much tied up to html. For example, text support in html: it has all these great possibilities to set fonts, use text sub/superpositions, letter spacing, kerning and what not, and yet POD is just fine without these text features. So yes, I think that Markdown's minimal support for images is the way to go, Markdown did it right.

I'm not really proficient in TeX/LaTeX, but I know how to deal with images in PDFs. That format has a rather unique set of image capabilities not necessarily available in html - it can apply arbitrary matrices, stencil masks, alpha masks, porter-duff operators (special alpha blendings), create filling patterns from images, and more. I think supporting these would be a serious overkill, and that is the same point of view I take when arguing against supporting all of html image functionality.

@tonycoz
Copy link
Contributor

tonycoz commented Sep 28, 2020

@xsawyerx I see a need for local images, f ex when converting to latex or pdf, or using an offline documentation. I agree that local and remote subtags are somewhat inelegant, however I seriously doubt that one can (and for that matter, should) enforce a html-like schema where both local and remote images can be defined as a single unambiguous URI. F ex metacpan or github, when rendering a pod file as html, won't extract referenced images that are found correspondingly either in archive or in a repository, that might have different paths compared to where an image is to be installed locally.

Resolving relative references to images is something the parser will likely need to do (possibly as an option indicating a base URL for pod2html).

I think a simple relative URL rather then distinguishing local and remote URLs would work.

I expect most formatters would disable remote URLs by default to avoid remote images becoming an attack vector by default if some image library ends up with an attackable security issue. I'd be inclined to not support remote URLs at all, since some (most?) processors will need to fetch the URL to render the image, or even to just get the size for HTML.

While I'd love images in POD, I'm not sure they belong in the format.

@dk
Copy link
Author

dk commented Sep 28, 2020

@tonycoz As my concern with remotes is only for metacpan/github/etc online renderers, I do agree that remotes, as a general concept, should be out of the picture. But let's look at github f ex: if I want to include an image from the same repository, I need this URL:

https://raw.githubusercontent.com/$USER/$MODULE/$BRANCH/pod/pic.png

whilte a pod itself, when rendered, is served from

https://github.com/$USER/$MODULE/tree/$BRANCH/pod/foo.pod

On one hand I'd love to say just =image pod/pic.png, on the other how would one persuade github to implement this?

@dk
Copy link
Author

dk commented Sep 28, 2020

update:

  1. just tested it on github, shows images just fine.

  2. metacpan can display markdown files but can't show images

So I guess remote and local could be indeed merged! This is great. Which means that we could also borrow from the markdown syntax and make images part of a text paragraph, not a paragraph on its own - with syntax like P<title|uri.png>, for example. This would also cut attempts to extend on the image functionality (width, alpha, whatever), which I think is a good thing.

@yuki-kimoto
Copy link

because I think that images should no longer be hacks but rather 1st class citizens.

I think so. I also want pod syntax which can express image.

@dk
Copy link
Author

dk commented Sep 30, 2020

Another point, the P<> syntax doesn't need to contain special text fallback. Because, contrary to the multitude of the graphical formatters, text can be addressed simply as =for text. That means one can write a pod such as


P<foo.png>

=for text this is graphic

if so desired.

I'm thinking about working on a patch however I don't want to find myself in a situation where it would be rejected or premature because the very topic is undecided yet. @xsawyerx what kind of consensus should be reached there for the green light, if any?

@karenetheridge
Copy link
Member

Proposed changes to pod syntax should be run through the pod-people list before merging.

https://lists.perl.org/list/pod-people.html

@tonycoz
Copy link
Contributor

tonycoz commented Oct 1, 2020


P<foo.png>

=for text this is graphic

The alt text could be expressed in the same way L<> does link text:

P<alt text|url>

@dk
Copy link
Author

dk commented Oct 8, 2020

Hello again,

After running the proposal through pod people, the idea I'm thankful for as it highlighted the flaws I wasn't aware of, and so here's the next version of the proposal. The consensus became a yaml embedded into 'image' formatter, like this:

=begin image

src: /path/file.png
title: "Figure 1"
text: 
  multiline
  entry
resolution: 120

=end image

or, for 90% of cases, simply

=for image src:file.png
   title:"Figure 1"

Apparently the yaml is also TMTWODI about multiline entries ( see https://yaml-multiline.info/ ), and what's more interesting, cpan's YAML doesn't understand neither of these, but that's not really relevant because YAML.pm is not in the core. The proposal is agnostic about multiline style.

Pod people raised a concern about how to parse these YAML sections in core perl modules. It seems the most sensible solution would be to include a minimal Pod::Simple::YAML parser, just enough to parse 1-level deep structures found in the image section.

The "src:" tag is to be used for both local and remote path, referring to the installation and/or distribution root. It will be the module author's job to make sure that, the image referred is installed in the same relative path as it is found in the distribution.

The "text:" fallback will be produced by formatter that understands =image, but where graphic output is unavailable. The drawback is that old and non-conforming formatters will not display the fallback text

The "resolution:" tag is to be used when the image size is important to match the text size. Formatters may use the value, in DPI, to scale the output image accordingly.

The "title:" tag is to be displayed as a generic description of the image. For example XHTML may use <figcaption> for this. When graphic is unavailable, the tag content may not be displayed if the "text:" tag exists.

The format is open for more tags, however I think these should be conservative - potential addition of "width: 90%" and "rotation: 45" and what not will complicate the things greatly; the rationale is to keep syntax rather terse, in the same vein as markdown's image syntax.

In the output, an image will not be part of text, ie a floating image with f ex inline formula. An image will always end the current paragraph and will start a new one. Two =image blocks will be rendered as two separated paragraphs.

It is proposed to adjust the core Pod::Simple so that it knows about the =image block, and will parse it so that the formatters don't need doing so. Pod::Simple creates an element tree and passes it to the formatters; these, in turn, should be adjusted to produce the corresponding output.

Now, I hope I didn't forget anything. Kindly come with comments!

@dk
Copy link
Author

dk commented Oct 13, 2020

@khwilliamson Karl, would that be okay to send a pull request for this feature in Pod::Simple?

@haarg
Copy link
Contributor

haarg commented Oct 13, 2020

Adding YAML to Pod seems like over-complicating things.

I had previously proposed a way to specify alternates where the Pod renderer would pick the first supported of several possible items it could render. The specific syntax I thought of probably isn't perfect. But that seems like it might be a better approach than creating a new way to embed text blobs in Pod.

@dk
Copy link
Author

dk commented Oct 13, 2020

@haarg indeed, but that's apparently too limiting. There's no way to specify a title/footnote/figcaption which is really nice to have. Also, as pointed y pod-people, =for text is too limiting as well, it won't work for *roff f.ex.

@khwilliamson
Copy link
Contributor

I do suggest sending a PR to Pod::Simple, and if people have comments on the specifics, they can do it there

@dk
Copy link
Author

dk commented Oct 20, 2020

@khwilliamson Thank you, I (optimistically) understood your original answer as positive enough :)

Here's the work in progress that I believe already resolved all potential problems, only tests, docs, and polishing left: https://github.com/dk/pod-simple/commits/master/lib/Pod/Simple . The individual commits are rather untidy, as it's more for myself, and when everything is ready I'm planning to kill this repository, fork it anew, and submit a pull request as one huge commit -- unless you would prefer the commits as they are now.

New things introduced:

  • lib/Pod/Simple/YAML.pm and t/yaml.t (this is completely ready and documented)

  • Formatter needs to be re-entrant if it wants to understand =image. This requires accept_image/accept_image_as_text and new events Image and Subdocument that a formatter must catch and respond to. The reason is that "title:" actually can be a pod, not just plain text, which in turns need parsing. Not ready and barely documented.

  • Tests are added but either failing or not finished.

I also tested this on perl's Pod::Man that is not in the Pod::Simple distro, but it seems to be working.

You're very welcome to take a preliminary look.

@dk
Copy link
Author

dk commented Oct 24, 2020

Hello everybody, as promised, there's a pull request sitting at perl-pod/pod-simple#128 . The fork is at https://github.com/dk/pod-simple, you're welcome to review and play with it.

The patch also opens the possibility to create html with images, but not by default. If you want to try pod2html you'll need this first:

--- lib/Pod/Html.pm     2020-10-24 13:09:44.661075012 +0200
+++ /usr/share/perl/5.30.0/Pod/Html.pm  2020-03-06 22:15:57.000000000 +0100
@@ -369,7 +369,6 @@
     my $parser = Pod::Simple::SimpleTree->new;
     $parser->codes_in_verbatim(0);
     $parser->accept_targets(qw(html HTML));
+    $parser->accept_image;
     $parser->no_errata_section(!$Poderrors); # note the inverse
 

@dk
Copy link
Author

dk commented Dec 7, 2020

Hello everyone, the patch seems to be stalled for unclear reasons, and my invitation for discussion why doesn't seem to be working. I'm sorry I'm not really good with soft skills so I don't understand why is that. Would someone kindly look at the status and see whether it is possible to promote the patch, or restart the discussion, or something else?

@xenu
Copy link
Member

xenu commented Dec 7, 2020

FWIW, that's how feature requests in this project usually end up. We don't have a real feature proposal process, so in practice only committers get to propose and implement features.

@xenu
Copy link
Member

xenu commented Dec 7, 2020

Regardless of what I said above, while people generally seem to like the idea of =image, there's strong opposition to the proposed YAML-based syntax (especially in the Pod::Simple PR).

@khwilliamson
Copy link
Contributor

And that is why I have yet to merge this. I haven't had time to educate myself as to the issues involved.

@Leont
Copy link
Contributor

Leont commented Dec 7, 2020

while people generally seem to like the idea of =image, there's strong opposition to the proposed YAML-based syntax

I think that is a fair assessment.

@dk
Copy link
Author

dk commented Dec 8, 2020

Thanks everyone. It is though rather disheartening that the said opposition never uttered a word while the syntax was discussed and agreed on, and only did that after I put lots of efforts in the implementation and polishing and testing. I don't think I have energy for another round of discussion about what syntax would please the crown. Sorry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests