Skip to content

Conversation

Octachron
Copy link
Member

While compiling the OCaml manual, it may happen (or not) that very large sum types make the hyperref package self-destruct with

\pdfendlink ended up in different nesting level than \pdfstartlink

due to a bad interaction between the hyperref and longtable package.

There is no clear fix beyond manual edition of the source, which is not reasonable for the generated latex source.

Thus this PR simply limits the number of rows of generated tables, which in turns removes the assumption that there is a reasonable latex package that knows how to handle multi-pages tables.

@Drup
Copy link
Contributor

Drup commented Oct 28, 2020

Thus this PR simply limits the number of rows of generated tables, which in turns removes the assumption that there is a reasonable latex package that knows how to handle multi-pages tables.

Welp.

(Raw.small_table pp) tbl
Raw.break Line
else
large_table size ppf tbl
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure why you now inserts breaks around the small table, but the result is a code that mixes abstraction level, which is not so nice in this function. If it was just then small_table tbl else large_table size tbl it would be fine, otherwise maybe you could move the whole thing to a single table size tbl call?

Copy link
Member

Choose a reason for hiding this comment

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

Not being familiar with this codebase, I find it a bit confusing that there is row_size = Huge|Large|Small|Empty, and then this new check which sounds related to the column size (or row count) rather than row size, but also appears to call Raw.{small,large}_table in the same way as width-based distinctions. It looks like small_table means "small width", but small_table_limit means "small height". Urgh.

Copy link
Member Author

Choose a reason for hiding this comment

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

In this context, small_table means essentially any tables that the basic tabular latex environment may render correctly.

There is indeed two notions of sizes colliding here: the height of the tables, and the maximal "width" of cells. I have renamed small_table_limit to small_table_height_limit, does this help?

I have moved the newlines to the raw latex side, with a comment that explains that we don't want the table to be inlined in-between delimiters.

let matrix ppf m = List.iter (row ppf) m in
match size with
| Huge -> Raw.break ppf Line; matrix ppf tbl
| Large | _ -> Raw.indent matrix ppf tbl
Copy link
Member

Choose a reason for hiding this comment

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

In the Huge case, the output now always begins and ends with a Line break. In the Large|Small|Empty case, your change to the output guarantees that we always have a Line break at the end, but not at the beginning. On the other hand, small-height small tables get a Line break at both the beginning and end. This sounds a bit inconsistent, but I don't know what the intent is.

Copy link
Member Author

Choose a reason for hiding this comment

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

The Huge case is in fact unnecessary, this was probably some scoria from prior implementation of recursive tables.

Copy link
Member

Choose a reason for hiding this comment

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

With the new implementation I'm still confused by line breaks in the code: if I understand correctly, row produces a final line break on lists of size 0 or 1, but not 2 and above, and it's not clear to me why.

Copy link
Member Author

Choose a reason for hiding this comment

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

There is probably too much hardwired knowledge about the imbrication of different environments, but here the indentation environment provides the line break for rows with more than one elements.

@Octachron Octachron force-pushed the latex_small_table_should_be_really_small branch from d355640 to 308a187 Compare November 6, 2020 17:47
@gasche
Copy link
Member

gasche commented Nov 6, 2020

Thanks, this is all clearer with the new changes. Please feel free to go along.

@jonludlam
Copy link
Member

Thanks everyone! In it goes.

@jonludlam jonludlam merged commit 3dbef23 into ocaml:master Nov 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants