Add labels to basic text block (such as paragraphs and code blocks) #974

panglesd · 2023-06-20T16:28:55Z

This allows to generate anchors, and link to specific places in the rendered html.
This is useful for search results in doc comments or mld page. Clickable anchors are not generated.

Somehow part of #972.

This allows to generate anchors, and link to specific places in the rendered html. This is useful for search results in doc comments or mld page. Clickable anchors are not generated. Signed-off-by: Paul-Elliot <peada@free.fr>

Julow

Some < 4.10 test files are not uptodate.

Julow · 2023-06-21T12:57:59Z

src/model/comment.ml

+  [ `Paragraph of Identifier.Label.t * paragraph
+  | `Code_block of Identifier.Label.t * string option * string with_location
+  | `Math_block of Identifier.Label.t * string
+  | `Verbatim of Identifier.Label.t * string
  | `Modules of module_reference list


Modules compiles to a html block element and doesn't contain odoc nestable block elements so could have a label too.

Julow · 2023-06-21T13:11:19Z

src/xref2/ref_tools.ml

+    List.for_all
+      (function
+        | `Label
+            (_, { Component.Label.label = _; content = Heading _; location = _ })


Perhaps the warning should be removed if it's due to generated labels ? The printing function used in the warning should at least not name it section-p0.

For example

(** *) (** foo *) (** {1:p0 A} *) (** {!p0} *)

Warning: Reference to 'p0' is ambiguous. Please specify its kind: section-p0, section-p0. File "../test.mli", line 5, characters 4-12: Warning: Label 'p0' is ambiguous. The other occurrences are: File "../test.mli", line 3, character 4

Julow · 2023-06-21T13:20:57Z

src/xref2/link.ml

+    ({ Odoc_model.Paths.Identifier.iv = `Label (lbl_parent, label_name); _ } as
+    id) =
+  let label_name = Names.LabelName.to_string label_name in
+  let need_new_label label = ambiguous_labels ~loc env label != [] in


Could this be a O(n^2) complexity ? There's ambiguous_labels inside Env that works on identifiers and could be exposed through a new is_label_ambiguous function.

Julow · 2023-06-21T13:27:11Z

src/xref2/component.mli

@@ -443,11 +455,14 @@ and CComment : sig
 end

 and Label : sig
+  (** In order to generate content for links without content *)
+  type content = Heading of Odoc_model.Comment.paragraph | NestableBlock


Identifier.Label.t is often deconstructed directly and used to have one meaning. Now it has two due to this and I can't be sure it makes sense everywhere it's matched on.

Have you though of adding a new kind of identifier for generated labels ? That wouldn't be a simplification but the disambiguation and reference resolving would be explicit in how they deal with the new kind of labels.

jonludlam · 2023-06-22T13:34:43Z

src/model/semantics.ml

+  fun kind status ->
+    let lbl =
+      match kind with
+      | `Paragraph -> "p"


Are these likely to clash with any other hand-written labels?

jonludlam · 2023-06-22T13:40:30Z

src/xref2/component.ml

@@ -474,11 +486,14 @@ end =
  CComment

 and Label : sig
+  (** In order to generate content for links without content *)
+  type content = Heading of Odoc_model.Comment.paragraph | NestableBlock
+
  type t = {


The attrs have been moved onto the `Heading constructor right? Any idea why they weren't there to begin with? With your fix it's much more similar to Comment.block_element now, so I'm confused why we didn't do that from day one!

jonludlam · 2023-06-22T13:59:35Z

I'm a little nervous about allowing these new labels as valid references, it seems a bit brittle. I'd much prefer it that if you wanted to link to a paragraph or code block or whatever that we could add in an explicit label. I can see the use of them in search, where we'll want to be able to link to anything automatically, of course.

dbuenzli · 2023-06-22T14:23:30Z

These labels are indeed going to be quite brittle, changing when the page changes.

I'm starting to think that maybe search should just only link directly on stuff that has direct user defined anchors (that includes structure items). When there is no such anchor it seems to me that just linking on the page like regular search engines do is ok.

jonludlam · 2023-06-22T15:06:14Z

I still think it's quite useful to be able to link directly from the search to the relevant part, especially in a large file. Though if that's the primary use-case then it's a little questionable why we're adding the links in the model rather than only in the html output, since it does mean a bit of pain going back and forth between Odoc_model.Comment and Component.CComment types.

dbuenzli · 2023-06-22T15:30:39Z

I still think it's quite useful to be able to link directly from the search to the relevant part, especially in a large file.

When you browse the web via a search engine the conjunction of result snippets and in page search usually does a good job at that.

I just worry that people are going to cut and paste around brittle links.

panglesd · 2023-06-22T16:17:56Z

One possibility would be to use the last heading as target when clicking on a search result corresponding to a paragraph. It would be a bit less precise, but still better than just linking to the page.
Auto-generated label for headings are also a bit brittle, although less than the one I'm introducing. But they are much easier to copy-paste, since they have clickable anchors to get the link.

Disambiguation of headings also need to be done earlier than after the document is generated, as is currently the case.
I guess for headings, since they can be referenced, it makes sense to disambiguate them at the model level, at least for the auto-generated one.

dbuenzli · 2023-06-22T16:25:17Z

One possibility would be to use the last heading as target when clicking on a search result corresponding to a paragraph.

That could be an idea!

Auto-generated label for headings are also a bit brittle, although less than the one I'm introducing.

Indeed. I don't think they are a good idea either. But somehow it's either that or having less usable pages because people don't care. There's no real good solution that I'm aware of.

panglesd · 2023-06-22T16:36:18Z

why we're adding the links in the model rather than only in the html output

The compilation of the search index is done from the .odocl, so the links should be knowable without going through the whole html/document generation.

One possibility would be to add a common first phase to html-generate and compile-index, which gives labels to the basic blocks. It's a pass that would be done twice (one for each command), but it would avoid the back and forth with Odoc_model.Comment and Component.CComment.

panglesd · 2023-06-22T16:36:26Z

I wonder what is @jonludlam and @Julow opinion on using the last heading as target. I'm hesitant between this and the current approach.

dbuenzli · 2023-06-22T16:44:48Z

I wonder what is @jonludlam and @Julow opinion on using the last heading as target. I'm hesitant between this and the current approach.

Note that this kind of stuff is hard to assess whether it will work well or not with users. There's little you can do but try and see how it fares in practice…

(And as such I would try that idea first)

jonludlam · 2023-06-23T10:11:34Z

Last heading or most recent anchor of any sort? I'm in favour of using the anchors we have right now and if that turns out to be a bad user experience then we can revisit this decision.

jonludlam · 2023-06-23T10:12:50Z

The compilation of the search index is done from the .odocl, so the links should be knowable without going through the whole html/document generation.

Aren't we outputting an html snippet in the index?

panglesd · 2023-06-23T10:39:18Z

Aren't we outputting an html snippet in the index?

Yes, but we are generating them separately for each entry. Disambiguation needs the document as a whole to work.

I'm putting this PR on pause to focus on the "last heading" or "last anchor" solution.

jonludlam · 2023-06-23T15:00:48Z

Yes, but we are generating them separately for each entry. Disambiguation needs the document as a whole to work.

Right, makes sense, thanks.

jonludlam · 2023-09-14T17:15:18Z

I'll close this PR now then - we can do disambiguation in another PR.

Julow reviewed Jun 21, 2023

View reviewed changes

jonludlam reviewed Jun 22, 2023

View reviewed changes

jonludlam closed this Sep 14, 2023

panglesd mentioned this pull request Sep 29, 2023

Support for search in odoc #972

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add labels to basic text block (such as paragraphs and code blocks) #974

Add labels to basic text block (such as paragraphs and code blocks) #974

panglesd commented Jun 20, 2023

Julow left a comment

Julow Jun 21, 2023

Julow Jun 21, 2023

Julow Jun 21, 2023

Julow Jun 21, 2023

jonludlam Jun 22, 2023

jonludlam Jun 22, 2023

jonludlam commented Jun 22, 2023

dbuenzli commented Jun 22, 2023

jonludlam commented Jun 22, 2023 •

edited

Loading

dbuenzli commented Jun 22, 2023 •

edited

Loading

panglesd commented Jun 22, 2023

dbuenzli commented Jun 22, 2023

panglesd commented Jun 22, 2023

panglesd commented Jun 22, 2023

dbuenzli commented Jun 22, 2023 •

edited

Loading

jonludlam commented Jun 23, 2023

jonludlam commented Jun 23, 2023

panglesd commented Jun 23, 2023

jonludlam commented Jun 23, 2023

jonludlam commented Sep 14, 2023

Add labels to basic text block (such as paragraphs and code blocks) #974

Add labels to basic text block (such as paragraphs and code blocks) #974

Conversation

panglesd commented Jun 20, 2023

Julow left a comment

Choose a reason for hiding this comment

Julow Jun 21, 2023

Choose a reason for hiding this comment

Julow Jun 21, 2023

Choose a reason for hiding this comment

Julow Jun 21, 2023

Choose a reason for hiding this comment

Julow Jun 21, 2023

Choose a reason for hiding this comment

jonludlam Jun 22, 2023

Choose a reason for hiding this comment

jonludlam Jun 22, 2023

Choose a reason for hiding this comment

jonludlam commented Jun 22, 2023

dbuenzli commented Jun 22, 2023

jonludlam commented Jun 22, 2023 • edited Loading

dbuenzli commented Jun 22, 2023 • edited Loading

panglesd commented Jun 22, 2023

dbuenzli commented Jun 22, 2023

panglesd commented Jun 22, 2023

panglesd commented Jun 22, 2023

dbuenzli commented Jun 22, 2023 • edited Loading

jonludlam commented Jun 23, 2023

jonludlam commented Jun 23, 2023

panglesd commented Jun 23, 2023

jonludlam commented Jun 23, 2023

jonludlam commented Sep 14, 2023

jonludlam commented Jun 22, 2023 •

edited

Loading

dbuenzli commented Jun 22, 2023 •

edited

Loading

dbuenzli commented Jun 22, 2023 •

edited

Loading