Medias in odoc #1005

panglesd · 2023-09-26T07:21:25Z

Based on #1002.
See #985 for discussion about the syntax.

The syntax

The syntax is the following, where <media> can be either image, video or audio:

{<media>!reference_to_asset}

{{<media>!reference_to_asset}replacement/alternative text}

{<media>:link_to_asset}

{{<media>:link_to_asset}replacement/alternative text}

The media elements are block elements, and as such should be separated from the rest of the content with new lines.
They cannot be put inline (so, for now, no {{:link-to-something}{image!ref}}...) or even inside other block elements (such as table, lists, ...).
The reason for this is that without attributes (e.g. to define the size) it would be quite hard to style the inserted media as an inline in a consistent way...
I think it is enough for most cases. Searching github for {html% and embedding of images seems to comfort this.

The rendering

Only html display medias, using the html5 elements audio, video and img. Even images are not displayed in latex (it lacks "asset" support).

For images, the "replacement text" is used an alt text. The replacement text is also used in case of unresolved reference.

For latex and manpage backend, the replacement text is always used.

Future work

I wondered whether something such as {href!reference.to."asset.png"}, which would render as ../../reference/to/asset.png (for instance) would be a good addition? It would allow more html embedding, such as {html% <img src="%}{href!ref}{html%"/>%}.
What do you think?

Having a way to specify the size (but this requires some syntax discussion!), or to add inline media, would be nice.

gpetiot · 2023-09-28T02:59:00Z

src/document/comment.ml

+            | Error exn ->
+                (* FIXME: better error message *)
+                Printf.eprintf "Id.href failed: %S\n%!"
+                  (Url.Error.to_string exn);


Something like: "Can't resolve asset file path/url"?

That's taken from a "warning" thrown in similar situations.
However, yes, we could finally fix those FIXME instead of propagating them through the code...
Related: #1010 which removed similar unconditional eprintfs.

gpetiot · 2023-09-28T03:08:12Z

src/document/types.ml

@@ -90,6 +94,9 @@ and Block : sig
    | Verbatim of string
    | Raw_markup of Raw_markup.t
    | Table of t Table.t
+    | Image of Target.t * Inline.t
+    | Video of Target.t * Inline.t
+    | Audio of Target.t * Inline.t


The 3 implementations is very similar, what do you think of using something similar to the model:

type media = [ `Image | `Audio | `Video ] type t = ... | Media of media * Target.t * Inline.t

?

I think both implementations are very similar but I can try to see if it can make some part of the code more factorized!

dbuenzli · 2023-09-28T11:54:17Z

They cannot be put inline (so, for now, no {{:link-to-something}{image!ref}}...) or even inside other block elements (such as table, lists, ...).

That seems rather limiting. I don't really care about the inline bit but you definitively want to put these things into lists, for example:

This can be either:
{ul
{- A sine wave {audio!sine.wav}}
{- A triangle wave {audio!tri.wav}}}

I don't really care if the assets element ends up being a block level element rather than an inline but I definitively want it into list elements.

Julow · 2023-09-28T13:03:58Z

I'm in favor of medias to be (nestable) block elements. These elements take time to show up and might never do. I think it's important to make sure they are visible, for example by nesting them in a block with a border and a large padding.
We also need space for a caption in the syntax and in the rendering.

panglesd · 2023-09-28T14:46:28Z

I turned medias into nestable block elements: it is now possible to add them inside lists and tables.
They will still be displayed as blocks. I haven't added any fixed size, border...
About the caption, I think this should be left for figures. I could add a new media, figure whose "replacement text" is the caption.

gpetiot · 2023-10-13T01:43:09Z

src/xref2/ref_tools.ml

@@ -349,7 +349,7 @@ module L = struct
            when name = LabelName.to_string name' ->
              Ok (`Identifier label)
          | _ -> find tl)
-      | [] -> Error (`Find_by_name (`Page, name))
+      | [] -> Error (`Find_by_name (`Label, name))


Is this a fix that could be made in a separate PR?

Yes, it could be made in another PR. Not sure that's worth it, given how small it is! But if anyone confirm it's helpful, I'll separate it into another PR!

gpetiot · 2023-10-13T02:01:54Z

src/html/generator.ml

+              image url
+          | Internal Unresolved ->
+              let a = [ Html.a_class [ "xref-unresolved" ] ] in
+              [ Html.span ~a (inline ~config ~resolve alt) ]


For unresolved images, what do you think of displaying placeholder images like:

When there is no alt text, yes that's a good idea! (are we allowed to use this particular image?)

When there is alt text, I'm not sure what's best!

Personally I think it's better to let things break. This makes it mechanically discoverable (404) if you add a stub, then the page has no problem as far as machines are concerned.

Readers also need to distinguish missing images. Perhaps the alt text could be made mandatory ?
That's what I was most worried about when I suggested to add border, padding and a caption around image blocks.

Readers also need to distinguish missing images.

Browsers already have their own way of notifying users about broken image sources (typically via a small icon representing a broken document), there's no need to invent anything here.

Ah but of course this should not be an a this should be an img tag with the class xref-unresolved and perhaps with the src attribute being simply the undefined reference (so that the broken icon shows up, it does not with an empty src).

Currently it's a span (the a is for attribute) which include visual feedback that something is wrong, but nothing tells the user or the browser that an image should be there. So yes, I think an img with broken url and the alt text as alt text (if there is one) makes sense.
I'll check if "broken url" can be made a real concept, and how the browser react!

panglesd · 2023-11-03T17:21:29Z

Here is the state of this PR!

It is mostly finished, I think. There is one remaining question: what do we do with unresolved medias.

@dbuenzli suggested to use an "unresolved" URL, so that the browser is responsible for showing the breakage.
I think it is a good idea, I just have some trouble accepting to forge an invalid url. Putting the reference as an url is making my brain output "Error: This expression has type reference but an expression was expected of type url" even if both ultimately are strings. It also creates a web request which, maybe, could work and show something unintended?

I have tested how my browser (firefox) displays an image with an url that returns 404. When there is no alt text, it shows an icon of a broken image. When there is an empty alt-text, it shows nothing. When there is a non-empty alt-text, it shows the alt-text as an image. All of this is a very good behaviour in my opinion.
With video, it is also very clear when the file is not found.
With audio however, the normal controls are shown, but as if the file were of length 0s, nothing happens when you click on the "play" button, there is no indication that a file could not be loaded.

Also, to note, I have tried some "invalid" urls, such as \\, and it shows the broken image without needing to make a network request. But this is very browser implementation specific...

I won't have time to work on this more in the coming month.

dbuenzli · 2023-11-04T14:29:14Z

@dbuenzli suggested to use an "unresolved" URL, so that the browser is responsible for showing the breakage.
I think it is a good idea,

In fact I forgot that odoc would already tell you that something is wrong, this reference doesn't exist. So there's little interest in these 404, there is already a way to discover these errors when you produce the docs. So maybe you can just output text "Missing {image,audio,video} asset" classified with xref-unresolved in a span or a div depending whether it's a block or inline element.

gpetiot

A few typos left in the code.

Otherwise, it looks good to me, is something else missing or blocking this PR?

src/model_desc/paths_desc.ml

gpetiot · 2023-11-09T10:40:01Z

src/parser/ast.ml

+    reference_kind
+    * media_href with_location
+    * inline_element with_location list
+    * media (** @since 2.3.0 *)]


probably 2.4.0?

src/parser/token.ml

gpetiot · 2023-11-09T10:55:07Z

src/xref2/ref_tools.ml

@@ -729,26 +770,55 @@ let resolve_reference_dot env parent name =
  | (`C _ | `CT _) as p -> resolve_reference_dot_class env p name
  | `P _ as page -> resolve_reference_dot_page env page name

+let resolve_page_reference env (r : Reference.Page.t) =
+  match r with
+  | `Resolved _ -> failwith "unimplemented"


Let's not forget this

jonludlam · 2023-11-12T23:54:49Z

Otherwise, it looks good to me, is something else missing or blocking this PR?

Yes, I'd just like to try the suggestion in #1002 before we merge that, then this can be rebased on top.

favonia · 2023-11-14T16:26:38Z

I have a related request which I believe will come up once this PR is merged: I hope there's a way for odoc to download external images, audios, and maybe videos and generate offline-viewable pages. The reason is that I might want to generate a self-contained documentation site for offline reading without putting everything in the same git repo.

Maybe this can be done in a different PR, but I hope the current design is compatible with this request.

Reference can now be toward assets. When resolving them, look up the assets of the current page, and if not found, recurse in the parent page. Signed-off-by: Paul-Elliot <peada@free.fr>

Signed-off-by: Paul-Elliot <peada@free.fr>

Co-authored-by: Guillaume Petiot <guillaume@tarides.com>

Signed-off-by: Paul-Elliot <peada@free.fr>

panglesd · 2024-03-18T10:36:03Z

Closing now since asset referencing will change subsequently soon. But some of the code will be used!

panglesd force-pushed the medias-in-odoc branch from dcd0b24 to 41e2c5b Compare September 26, 2023 08:17

gpetiot reviewed Sep 28, 2023

View reviewed changes

panglesd force-pushed the medias-in-odoc branch from 41e2c5b to 75d5830 Compare September 28, 2023 14:39

panglesd force-pushed the medias-in-odoc branch 2 times, most recently from 4fa1ec4 to 76071aa Compare September 28, 2023 15:08

panglesd requested review from jonludlam and Julow September 29, 2023 07:59

gpetiot reviewed Oct 13, 2023

View reviewed changes

gpetiot reviewed Nov 9, 2023

View reviewed changes

EmileTrotignon added the after-2.4 label Dec 5, 2023

panglesd added 11 commits January 8, 2024 09:32

Introduce resolving of asset referencing

7df2c1f

Reference can now be toward assets. When resolving them, look up the assets of the current page, and if not found, recurse in the parent page. Signed-off-by: Paul-Elliot <peada@free.fr>

fix wrong kind in warning for missing label

92a9862

Signed-off-by: Paul-Elliot <peada@free.fr>

ocamlformat

78577ae

Signed-off-by: Paul-Elliot <peada@free.fr>

ref to assets: fix test explanation

68f572d

Signed-off-by: Paul-Elliot <peada@free.fr>

Assets: add documentation

c87f9eb

Signed-off-by: Paul-Elliot <peada@free.fr>

asset reference test: compatiblity

d046ffe

Signed-off-by: Paul-Elliot <peada@free.fr>

Add changelog entry

790dc28

Signed-off-by: Paul-Elliot <peada@free.fr>

Populate environment with assets

d9911e3

Signed-off-by: Paul-Elliot <peada@free.fr>

Asset referencing: compatibility

367601a

Signed-off-by: Paul-Elliot <peada@free.fr>

Assets: Update convention for installed packages

272d8e7

Signed-off-by: Paul-Elliot <peada@free.fr>

Added syntax for medias

1dbbda9

Signed-off-by: Paul-Elliot <peada@free.fr>

panglesd force-pushed the medias-in-odoc branch from 14a8c2f to 39de6d2 Compare January 9, 2024 13:38

Medias: make them nestable blocks

eb67d4e

Signed-off-by: Paul-Elliot <peada@free.fr>

Medias: add documentation

8892a68

Signed-off-by: Paul-Elliot <peada@free.fr>

panglesd force-pushed the medias-in-odoc branch 2 times, most recently from ef077e3 to 4e40617 Compare January 10, 2024 07:42

panglesd and others added 4 commits January 10, 2024 08:42

Media: fix warning when a non-asset ref is a media target

1116a03

Signed-off-by: Paul-Elliot <peada@free.fr>

Fix typos in media support

5b1333c

Co-authored-by: Guillaume Petiot <guillaume@tarides.com>

Fix @SInCE annotation on parser's AST for media

975bdde

Signed-off-by: Paul-Elliot <peada@free.fr>

Add media to search traverse

4e40617

Signed-off-by: Paul-Elliot <peada@free.fr>

panglesd closed this Mar 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Medias in odoc #1005

Medias in odoc #1005

panglesd commented Sep 26, 2023

gpetiot Sep 28, 2023

panglesd Sep 28, 2023

gpetiot Sep 28, 2023

panglesd Sep 28, 2023

dbuenzli commented Sep 28, 2023

Julow commented Sep 28, 2023

panglesd commented Sep 28, 2023

gpetiot Oct 13, 2023

panglesd Oct 17, 2023

gpetiot Oct 13, 2023

panglesd Oct 17, 2023

dbuenzli Oct 17, 2023 •

edited

Julow Oct 25, 2023

dbuenzli Oct 25, 2023

dbuenzli Oct 25, 2023

panglesd Oct 25, 2023

panglesd commented Nov 3, 2023

dbuenzli commented Nov 4, 2023

gpetiot left a comment

gpetiot Nov 9, 2023

gpetiot Nov 9, 2023

jonludlam commented Nov 12, 2023

favonia commented Nov 14, 2023 •

edited

panglesd commented Mar 18, 2024

Medias in odoc #1005

Medias in odoc #1005

Conversation

panglesd commented Sep 26, 2023

The syntax

The rendering

Future work

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbuenzli commented Sep 28, 2023

Julow commented Sep 28, 2023

panglesd commented Sep 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbuenzli Oct 17, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

panglesd commented Nov 3, 2023

dbuenzli commented Nov 4, 2023

gpetiot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonludlam commented Nov 12, 2023

favonia commented Nov 14, 2023 • edited

panglesd commented Mar 18, 2024

dbuenzli Oct 17, 2023 •

edited

favonia commented Nov 14, 2023 •

edited