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.

Sign up for GitHub

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

Jump to bottom

Media in odoc 3! #1184

Merged
merged 17 commits into from
Aug 27, 2024
Merged

Media in odoc 3! #1184

merged 17 commits into from
Aug 27, 2024

Conversation

panglesd
Copy link
Collaborator

@panglesd panglesd commented Aug 1, 2024 •
edited
Loading

This is a revival of #1005 but with the new asset references from odoc 3.

The main difference for users is that the media target use path instead of odoc references: {image!path/to/image.jpg} without needing any escaping.

See #1005 and #1113 for prior discussions.

Julow
Julow approved these changes Aug 2, 2024
View reviewed changes
Copy link
Collaborator

@Julow Julow left a comment

Choose a reason for hiding this comment

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

The future is here! I've left some comments but I think these could be fixed later, I don't want to delay the PR.

doc/odoc_for_authors.mld Outdated
video. Each of them can refer to the file either using an asset
reference, or a direct link.

The markup is [{<media>:link}], [{<media>!ref}],
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the <media> part is unclear. I suggest showing examples for images ({image:link}, {image!ref}, etc..) and later say the same work for video and audio (possibly with more examples).

src/document/comment.ml Outdated
match Url.from_identifier ~stop_before:false id with
| Ok url -> Target.Internal (Resolved url)
| Error exn ->
(* FIXME: better error message *)
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's unclear why this is needed. Couldn't we change the type of `Media's first argument to never hit this case ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Trying to turn odoc IDs that contain core types/exception returns an error. Since there is no type including "all ids without core types", we end up with a lot of those "FIXME: better error message", assert false and other failwith "bad"...

In 34ef32c I removed this case since we know Asset identifier won't have core types/exceptions. But this is not fixing it in any other context, which is out of scope from this PR.

src/html/generator.ml Outdated
@@ -206,6 +207,26 @@ let text_align = function

let cell_kind = function `Header -> Html.th | `Data -> Html.td

(* Turns an inline into a string, for use as alternative text in
images *)
let rec alt_of_inline (i : Inline.t) =
Copy link
Collaborator

Choose a reason for hiding this comment

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

I find it weird to allow inline markup in the syntax but then strip it out. The AST could contain just a string.
This markup is used in other backends but I don't think that's going to be used by anyone. If someone wanted to write something richer than an alt text here, why hide it on the most common backend?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm happy "forbidding" marked-up content in alt texts (just like block content is forbidden in inline content), but I'd still want to parse the content.

Indeed, it would be very strange to me if {{image:link}Some {b emphasized} content} were an image with alt text "Some {b emphasized" followed by the text "content}".

We could raise a warning (Warning: '{b ...}' (bold content) is not allowed in '{{:...} ...}' (external link)., and have the AST contain just a string, as you suggest. But do you agree that we should render it as it is rendered now ("Some emphasized content", stripping the {b) ?

src/html/generator.ml Outdated
@@ -243,6 +264,55 @@ let rec block ~config ~resolve (l : Block.t) : flow Html.elt list =
let extra_class = [ "language-" ^ lang_tag ] in
mk_block ~extra_class Html.pre (source (inline ~config ~resolve) c)
| Math s -> mk_block Html.div [ block_math s ]
| Audio (target, content) ->
let content = inline ~config ~resolve content in
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's a lot of duplicated code and it's not entirely obvious code. I'd really prefer to have this factorized.

src/xref2/ref_tools.ml Outdated
@@ -831,6 +831,11 @@ let resolved3 (r, _, _) = resolved1 r

and resolved2 (r, _) = resolved1 r

let resolve_asset_reference env (r : Reference.Asset.t) : Asset.t ref_result =
match r with
| `Resolved _r -> failwith "What's going on!?"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Haha I think we can just return r. In the past some things could be linked twice in some complex cases with includes and stuff like that. This could happen again, let's not crash.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed for reresolving assets in 01109c6
The strange failwith is still there for module and signature references, since it's harder to solve and out of scope from this PR.

src/model/reference.ml
(* The string is scanned right-to-left, because we are interested in right-most
hyphens. The tokens are also returned in right-to-left order, because the
traversals that consume them prefer to look at the deepest identifier
first. *)
let tokenize location s : token list =
let tokenize location s : token list * path_prefix option =
Copy link
Collaborator

Choose a reason for hiding this comment

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

Much better and safer!

@panglesd
Copy link
Collaborator Author

I believe all review comments have been addressed!

Here are the important changes:

  • Alternative text is now a string for all medias.
  • HTML generation also add the aria-label with alt text content on audio and video elements, as per suggested here.

@panglesd
Copy link
Collaborator Author

Rebased! (and fixed changelog entry for #1185)

@panglesd
Copy link
Collaborator Author

I looked at occurrences of images in odoc documentation using embedded html: see this.

Not all, but many uses include a way to specify the size of the image.

While I think it should be done in another PR, a syntax for specifying the size will be quite important to define!

@jonludlam jonludlam force-pushed the media-in-odoc3 branch 3 times, most recently from 5f7712e to 62663f5 Compare August 27, 2024 14:50
panglesd and others added 17 commits August 27, 2024 16:43
@panglesd @jonludlam
Add support for medias (images, audio and video)
dc3aeb8
@panglesd @jonludlam
Reintroduce emptyness check in path components of references
e0f5ad5
@panglesd @jonludlam
Media: fix @SInCE tag for media in parser's ast
c5aa489
@panglesd @jonludlam
Media: add documentation
fe7aebd
@panglesd @jonludlam
Added change entry for medias
6a8caf5
@panglesd @jonludlam
Disable media test on OCaml < 4.07
f9e016c 
Due to difference in cmdliner output.
@panglesd @jonludlam
Compatibility
9c7c123
@panglesd @Julow @jonludlam
Typos in documentation
bd3a857 
Co-authored-by: Jules Aguillon <[email protected]>
@panglesd @jonludlam
Medias: improve test
b6ff7bb
@panglesd @jonludlam
Media: improve docs for authors
be3e6f7
@panglesd @jonludlam
Medias: rule out error case
5d7a256
@panglesd @jonludlam
Media: factor html generation
5a175b3
@panglesd @jonludlam
Medias: allow re-resolving asset references
0f3457d
@panglesd @jonludlam
Media: Do not interpret markup in replacement text
ba6d1cc
@panglesd @jonludlam
Media: Fix loc and tests for media alt text
4852354
@panglesd @jonludlam
Media: newline in alt text are turned into spaces
4bec2be
@panglesd @jonludlam
Media: add alt text to aria-label for audio and video
3d20740 
As per the recommended solution of:
https://www.w3.org/WAI/PF/HTML/wiki/Media_Alt_Technologies
@jonludlam jonludlam merged commit ef9ca48 into ocaml:master Aug 27, 2024
13 checks passed
jonludlam added a commit to jonludlam/opam-repository that referenced this pull request Jan 23, 2025
@jonludlam
[new release] odoc (6 packages) (3.0.0~beta1)
1bbe221 
CHANGES:

### Highlight

- Hierarchical documentation (@jonludlam, @panglesd, @Julow)
  Pages can now be organized in a directory tree structure.
  Relative and absolute references are added:
  `{!./other_page.label}`, `{!//other_page}`.

- Improved sidebar and breadcrumbs navigation (@panglesd, @gpetiot)
  The documentation pages and the libraries of the entire package are shown on
  the left sidebar.

- Added support for images, videos, audio and other assets
  The syntax is `{image!/reference/to/asset}` or `{image:URL}` for images.
  The syntax for `{video...}` and `{audio...}` is the same.
  (@panglesd, @EmileTrotignon, ocaml/odoc#1170, ocaml/odoc#1171, ocaml/odoc#1184, ocaml/odoc#1185)

- Search using Sherlodoc (@panglesd, @EmileTrotignon, @Julow)
  A new search bar that supports full-text and type-based search.

### Added

- Experimental driver (@jonludlam, @panglesd)
  The driver builds the documentation for a collection of Opam packages using
  the newer Odoc features. It supports linking external packages to ocaml.org
  and markdown files.
  This is experimental and will break in the future.

- Cross-package references (@panglesd, @Julow)
  Pages and modules from other packages can be referenced:
  `{!/otherpackage/page}`, `{!/otherpackage/Module.t}`.

- Option to remap links to other packages to ocaml.org or other site.
  See the `--remap` option of the driver or the `--remap-file` option of `odoc html-generate`.
  (@jonludlam, ocaml/odoc#1189, ocaml/odoc#1248)

- Option to compute occurrences of use of each identifiers
  The commands `aggregate-occurrences` and `count-occurrences` are added.
  (@panglesd, ocaml/odoc#976, ocaml/odoc#1076, ocaml/odoc#1206)

- Added the `odoc classify` command (@jonludlam, ocaml/odoc#1121)
  Helps driver detecting which modules belong to which libraries.
- Added `--suppress-warnings` to the CLI to remove warnings from a unit, even
  if they end up being raised in another unit through expansion
  (@jonludlam, ocaml/odoc#1260)
- Add clock emoji before `@since` tag (@yawaramin, ocaml/odoc#1089)
- Navigation for the search bar : use '/' to enter search, up and down arrows to
  select a result, and enter to follow the selected link. (@EmileTrotignon, ocaml/odoc#1088)
- Fix a big gap between the preamble and the content of a page (@EmileTrotignon, ocaml/odoc#1147)
- Add a marshalled search index consumable by sherlodoc (@EmileTrotignon, @panglesd, ocaml/odoc#1084)
- Allow referencing of polymorphic constructors in polymorphic variant type
  aliases (@panglesd, ocaml/odoc#1115)
- Added a home icon in the breacrumbs (@panglesd, ocaml/odoc#1251)
  It can be disabled with a CLI option.
- Add a frontmatter syntax for mld pages (@panglesd, ocaml/odoc#1187, ocaml/odoc#1193, ocaml/odoc#1243, ocaml/odoc#1246, ocaml/odoc#1251)
  Allows to specify the title of a page, the order of sub-pages and other
  behaviors in the sidebar.
- Added `odoc-md` to process standalone Markdown pages (@jonludlam, ocaml/odoc#1234)

### Changed

- The command line interface changed to support the new features.
  + Packages and libraries: `odoc link` must now be aware of packages and
    libraries with the `-L libname:path` and `-P pkgname:path` options. The
    module search path should still be passed with the `-I` option.
    The current package should be specified with `--current-package=pkgname`.
  + Hierarchy: `odoc compile` now outputs `.odoc` in the directory tree
    specified with `--output-dir=DIR` and the parent identifier must be
    specified with `--parent-id=PARENT`.
    The option `--source-parent-file` is removed.
  + Source code: Implementations are compiled with `compile-impl` instead of
    with `compile`. The options `--cmt=..` and `--source-name=..` are removed.
    Source code pages are generated with `html-generate-source`.
  + Assets: The commands `compile-asset`, `html-generate-asset` are added.
    The option `html-generate --asset` is removed.
  + Sidebar: The index is built using `compile-index`. The sidebar data is
    extracted from the index with `sidebar-generate` and passed to
    `html-generate --sidebar=..`.

- The syntax for `@tag` is now delimited (@panglesd, ocaml/odoc#1239)
  A `@tag` can now be followed by a paragraph or other elements.

- Updated colors for code fragments (@EmileTrotignon, ocaml/odoc#1023)
- Fixed complexity of looking up `.odoc` files (@panglesd, ocaml/odoc#1075)
- Normalize whitespaces in codespans (@gpetiot, ocaml/odoc#1085)
  A newline followed by any whitespaces is normalized as one space character.
- Reduce size of `Odoc_html_frontend` when compiled to javascript
  (@EmileTrotignon, ocaml/odoc#1072)
- Overhaul of module-type-of expansions and shadowing code (@jonludlam, ocaml/odoc#1081)
- Output file paths and labels in the man and latex backends changed to avoid
  name clashes (@Julow, ocaml/odoc#1191)

### Fixed

- Fix variant constructors being hidden if they contain hidden types
  (@jonludlam, ocaml/odoc#1105)
- Fix rare assertion failure due to optional parameters
  (@jonludlam, ocaml/odoc#1272, issue ocaml/odoc#1001)
- Fix resolution of module synopses in {!modules} lists that require --open
  (@jonludlam, ocaml/odoc#1104}
- Fix top comment not being taken from includes often enough (@panglesd, ocaml/odoc#1117)
- Fixed 404 links from search results (@panglesd, ocaml/odoc#1108)
- Fixed title content not being picked up across pages when rendering references
  (ocaml/odoc#1116, @panglesd)
- Fix wrong links to standalone comments in search results (ocaml/odoc#1118, @panglesd)
- Remove duplicated or unwanted comments with inline includes (@Julow, ocaml/odoc#1133)
- Fix bug where source rendering would cause odoc to fail completely if it
  encounters invalid syntax (@jonludlam ocaml/odoc#1208)
- Add missing parentheses in 'val (let*) : ...' (@Julow, ocaml/odoc#1268)
- Fix syntax highlighting not working for very large files
  (@jonludlam, @Julow, ocaml/odoc#1277)
jonludlam added a commit to jonludlam/opam-repository that referenced this pull request Jan 23, 2025
@jonludlam
[new release] odoc (5 packages) (3.0.0~beta1)
0ffc6db 
CHANGES:

- Hierarchical documentation (@jonludlam, @panglesd, @Julow)
  Pages can now be organized in a directory tree structure.
  Relative and absolute references are added:
  `{!./other_page.label}`, `{!//other_page}`.

- Improved sidebar and breadcrumbs navigation (@panglesd, @gpetiot)
  The documentation pages and the libraries of the entire package are shown on
  the left sidebar.

- Added support for images, videos, audio and other assets
  The syntax is `{image!/reference/to/asset}` or `{image:URL}` for images.
  The syntax for `{video...}` and `{audio...}` is the same.
  (@panglesd, @EmileTrotignon, ocaml/odoc#1170, ocaml/odoc#1171, ocaml/odoc#1184, ocaml/odoc#1185)

- Search using Sherlodoc (@panglesd, @EmileTrotignon, @Julow)
  A new search bar that supports full-text and type-based search.

- Experimental driver (@jonludlam, @panglesd)
  The driver builds the documentation for a collection of Opam packages using
  the newer Odoc features. It supports linking external packages to ocaml.org
  and markdown files.
  This is experimental and will break in the future.

- Cross-package references (@panglesd, @Julow)
  Pages and modules from other packages can be referenced:
  `{!/otherpackage/page}`, `{!/otherpackage/Module.t}`.

- Option to remap links to other packages to ocaml.org or other site.
  See the `--remap` option of the driver or the `--remap-file` option of `odoc html-generate`.
  (@jonludlam, ocaml/odoc#1189, ocaml/odoc#1248)

- Option to compute occurrences of use of each identifiers
  The commands `aggregate-occurrences` and `count-occurrences` are added.
  (@panglesd, ocaml/odoc#976, ocaml/odoc#1076, ocaml/odoc#1206)

- Added the `odoc classify` command (@jonludlam, ocaml/odoc#1121)
  Helps driver detecting which modules belong to which libraries.
- Added `--suppress-warnings` to the CLI to remove warnings from a unit, even
  if they end up being raised in another unit through expansion
  (@jonludlam, ocaml/odoc#1260)
- Add clock emoji before `@since` tag (@yawaramin, ocaml/odoc#1089)
- Navigation for the search bar : use '/' to enter search, up and down arrows to
  select a result, and enter to follow the selected link. (@EmileTrotignon, ocaml/odoc#1088)
- Fix a big gap between the preamble and the content of a page (@EmileTrotignon, ocaml/odoc#1147)
- Add a marshalled search index consumable by sherlodoc (@EmileTrotignon, @panglesd, ocaml/odoc#1084)
- Allow referencing of polymorphic constructors in polymorphic variant type
  aliases (@panglesd, ocaml/odoc#1115)
- Added a home icon in the breacrumbs (@panglesd, ocaml/odoc#1251)
  It can be disabled with a CLI option.
- Add a frontmatter syntax for mld pages (@panglesd, ocaml/odoc#1187, ocaml/odoc#1193, ocaml/odoc#1243, ocaml/odoc#1246, ocaml/odoc#1251)
  Allows to specify the title of a page, the order of sub-pages and other
  behaviors in the sidebar.
- Added `odoc-md` to process standalone Markdown pages (@jonludlam, ocaml/odoc#1234)

- The command line interface changed to support the new features.
  + Packages and libraries: `odoc link` must now be aware of packages and
    libraries with the `-L libname:path` and `-P pkgname:path` options. The
    module search path should still be passed with the `-I` option.
    The current package should be specified with `--current-package=pkgname`.
  + Hierarchy: `odoc compile` now outputs `.odoc` in the directory tree
    specified with `--output-dir=DIR` and the parent identifier must be
    specified with `--parent-id=PARENT`.
    The option `--source-parent-file` is removed.
  + Source code: Implementations are compiled with `compile-impl` instead of
    with `compile`. The options `--cmt=..` and `--source-name=..` are removed.
    Source code pages are generated with `html-generate-source`.
  + Assets: The commands `compile-asset`, `html-generate-asset` are added.
    The option `html-generate --asset` is removed.
  + Sidebar: The index is built using `compile-index`. The sidebar data is
    extracted from the index with `sidebar-generate` and passed to
    `html-generate --sidebar=..`.

- The syntax for `@tag` is now delimited (@panglesd, ocaml/odoc#1239)
  A `@tag` can now be followed by a paragraph or other elements.

- Updated colors for code fragments (@EmileTrotignon, ocaml/odoc#1023)
- Fixed complexity of looking up `.odoc` files (@panglesd, ocaml/odoc#1075)
- Normalize whitespaces in codespans (@gpetiot, ocaml/odoc#1085)
  A newline followed by any whitespaces is normalized as one space character.
- Reduce size of `Odoc_html_frontend` when compiled to javascript
  (@EmileTrotignon, ocaml/odoc#1072)
- Overhaul of module-type-of expansions and shadowing code (@jonludlam, ocaml/odoc#1081)
- Output file paths and labels in the man and latex backends changed to avoid
  name clashes (@Julow, ocaml/odoc#1191)

- Fix variant constructors being hidden if they contain hidden types
  (@jonludlam, ocaml/odoc#1105)
- Fix rare assertion failure due to optional parameters
  (@jonludlam, ocaml/odoc#1272, issue ocaml/odoc#1001)
- Fix resolution of module synopses in {!modules} lists that require --open
  (@jonludlam, ocaml/odoc#1104}
- Fix top comment not being taken from includes often enough (@panglesd, ocaml/odoc#1117)
- Fixed 404 links from search results (@panglesd, ocaml/odoc#1108)
- Fixed title content not being picked up across pages when rendering references
  (ocaml/odoc#1116, @panglesd)
- Fix wrong links to standalone comments in search results (ocaml/odoc#1118, @panglesd)
- Remove duplicated or unwanted comments with inline includes (@Julow, ocaml/odoc#1133)
- Fix bug where source rendering would cause odoc to fail completely if it
  encounters invalid syntax (@jonludlam ocaml/odoc#1208)
- Add missing parentheses in 'val (let*) : ...' (@Julow, ocaml/odoc#1268)
- Fix syntax highlighting not working for very large files
  (@jonludlam, @Julow, ocaml/odoc#1277)
@jonludlam jonludlam mentioned this pull request Jan 23, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Julow Julow Julow approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@panglesd @Julow @jonludlam