Initial table per-cell customization [More Flexible Tables Pt.2a]

[PgBiel]

Creates the elements grid.cell and table.cell with the properties fill, align and inset, besides body.
Fields x and y will be added in a future PR.
Part of the second task in #3001

• As a drive-by, fixes Wrong Col and Row number on table when have gutter parameter #1386 🙂 (with e364dcd)
• Added tests to grid-cell.typ and table-cell.typ.
• Changed grid and table docs (we can probably adjust them better after this PR if needed).

Summary of changes

User-facing API

You can now override grid and table properties for particular cells through grid.cell and table.cell. You can also apply show rules on them. Show rules allow overriding align and inset, but not fill.

The following examples now work:

#grid(
  columns: 2,
  fill: red,
  align: left,
  inset: 5pt,
  [ABC], [ABC],
  grid.cell(fill: blue)[C], [D],
  grid.cell(align: center)[E], [F],
  [G], grid.cell(inset: 0pt)[H]
)

#{
  show grid.cell: emph
  grid(
    columns: 2,
    gutter: 3pt,
    [Hello], [World],
    [Sweet], [Italics]
  )
}

#table(
  columns: 2,
  fill: green,
  align: right,
  [*Name*], [*Data*],
  table.cell(fill: blue)[J.], [Organizer],
  table.cell(align: center)[K.], [Leader],
  [M.], table.cell(inset: 0pt)[Player]
)

#{
  show table.cell: emph
  table(
    columns: 2,
    [Person], [Animal],
    [John], [Dog]
  )
}

The outputs of the examples with table are:

Implementation details

1. GridLayouter and related types (including Celled<T>) are now in a new file: layout/gridlayouter.rs.
2. Created a Cell struct which (for now) holds a body (Content) and a fill. (Contains only properties useful for the GridLayouter.) It implements From<Content> so you can create a Cell with just a body and no property overrides (used in list and enum's implementations).
3. Created GridCell and TableCell, the elements corresponding to grid.cell and table.cell, respectively. Moved the application of per-cell alignment and inset to the Show implementation of those elements.
4. Created a ResolvableCell trait with a method .resolve_cell taking its final position in the grid (x, y), the global grid options (fill, align and inset), and returning a Cell (with only body and fill). This is implemented by GridCell and TableCell.
   • This functions as a "mini-Synthesize": the cell's fields are adjusted to their final values, so that, in show rules, it.fill, it.align and it.inset will have their real values instead of auto everywhere, which isn't very useful.
   • After adjusting its fields, the cell packs itself into type-erased Content, and becomes the body in the returned Cell, which also holds its final fill.
5. Created a CellGrid struct which holds a vector of cells (they are owned); the tracks; is_rtl and has_gutter. Those fields were removed from GridLayouter (except for has_gutter as it's just a bool) and replaced by a grid: &'a CellGrid field.
   • CellGrid is responsible for positioning the cells in the grid according to the tracks. (In this PR, for now, it just converts cells to a vector of Cell and ensures that vector has the correct length.) It does not, however, measure the tracks, or otherwise do anything related to Layout - that's entirely done by GridLayouter.
   • The GridLayouter::cell method, which retrieves the cell at a certain position, was moved to CellGrid::cell. This is so the GridLayouter can call self.grid.cell(x, y) without prompting a full immutable borrow of self, which caused borrow check problems (when using self.cell(x, y)) inside render_fills_strokes, as self was already mutably borrowed, but through a different field.
   • CellGrid can be constructed either through CellGrid::new, which accepts an array of already final Cell instances (used by list and enum), or through CellGrid::new_resolve::<T: ResolvableCell> (used by grid and table), which takes an array of resolvable cells and resolves them into final Cell instances, before calling CellGrid::new at the end.
   • CellGrid is created by the caller of GridLayouter, and must be passed by reference as a parameter upon creating a new GridLayouter.
6. GridLayouter was adapted to use per-cell fill instead of a global fill (assumes each cell was already resolved before through CellGrid, if needed, and is aware of its final fill property). It does not handle alignment and inset at all.

Additionally, created Smart::or_else since it was convenient for the implementation of resolve_cells by GridCell and TableCell. Fixed some weird docs on Smart::and_then as a drive-by.

Alternative design: Generics

Before dc99528, Cell was a trait implemented by GridCell and TableCell with fill as a function (instead of a struct with body and fill as fields). This caused CellGrid to be generic over T: Cell and hold a Vec<T> instead of a Vec<Cell>. As a result, GridLayouter was also generic over T: Cell. There were two main problems related to this:

1. With GridLayouter being generic, three copies of its code were made: one for GridCell, for TableCell and for Content (list / enum). This could cause some bloat, although it could be reduced by refactoring a bit the implementation. Still, that was something to consider.
2. GridCell and TableCell had to (somewhat redundantly) implement Layout to satisfy being a Cell, but they already implemented Show. In the Layout code, additionally, each cell was being cloned so it could be pack()ed before laying out. Thus, each cell was being cloned on each call to measure() or layout() by the GridLayouter. With the new design, we only pack() once (without cloning!): on the resolve_cell call, before the GridLayouter instance is even created.
3. Perhaps due to 2, the new design also seems to perform slightly faster (and I have thus updated the numbers below). This PR used to cause the "extreme" sample documents mentioned below to perform 10-14% slower; that has now been reduced to about 8% slower.

Possible problems and regressions

• Now, each cell element (packed in each Cell.body field) will hold a copy of their own fill, alignment and inset data. Thus, cells are now larger than just a simple Content body field. Not sure if there's much we can do about this if we want to make "mini-Synthesize" possible. (Would be convenient with some Cow-like thing, but not sure how we could achieve that?)
  • I believe this shouldn't be that bad. We probably won't change this much in further PRs anyway (other than adding more fields).
• Some unscientific performance tests:
  • On release mode, this PR (compared to b125628) seems to make a document with only a table and a grid with 400 [ABC] cells each 3-4% slower to cold compile (5.2 -> 5.4ms) on my computer (AMD Ryzen 7 5700G CPU). Increasing to 5000 [ABC] cells renders a 10% cold compile time increase (32ms -> 35ms) For a document with 50 tables and 50 grids each with 5000 [ABC] cells, it becomes about a 8% cold compile time increase (645 -> 700ms). It seems to remain that way for 5000 tables and 5000 grids with 5000 [ABC] cells each (34s -> 37s), though that's very extreme already 😂
  • (I had included some debug mode measurements, though I don't think debug mode perf is very relevant)
  • Conclusion: It's not that bad, even on unrealistically extreme table usage. I think this PR does pretty much fine in regard to perf, and we can always optimize later. Also, this is still MUCH better than tablex in terms of performance (tablex can't even compile the larger samples I used without OOM'ing on my system with 64 GB RAM).

[PgBiel]

This will be called (and the for loop in render_fills_strokes will run) for all users of GridLayouter, including list and enum, which don't use fills and strokes at all. Before, I avoided that by checking if the global fill was None, but now I can't do that due to per-cell overrides.
Should I perhaps add some toggle in this layout method to opt into rendering fills and strokes, as to optimize for those use cases? Or perhaps have some associated constant in trait Cell, something like const USES_STROKES_OR_FILLS: bool;, to indicate whether this is necessary?

[laurmaedje]

I wouldn't worry about it for now. The overhead is probably relatively low.

[laurmaedje]

I think this comment is outdated.

[laurmaedje]

I think this example isn't ideal because emphasizing the whole grid would be the same. Maybe we can come up with something that's really best done via show grid.cell.

[PgBiel]

I'm not sure, I think with the PR at the current state we'd have to add a fairly non-standard example here (e.g. apply emph based on alignment or something). With the x, y fields PR, however, we'd probably have more to show, since we could e.g. make only the first row bold.

[laurmaedje]

Then maybe let's remove the example until we have something idiomatic to show. Or I guess keep and change it later. I don't mind either way.

[PgBiel]

Sure, removing sounds fair for now.

[laurmaedje]

especially the third and fourth test are a bit hard to understand because of the colors. maybe blue should be replaced by aqua on the tests? it is better suited for a background color.

[PgBiel]

Should we make inset Celled in this PR? Or leave this for later maybe (perhaps to make it faster to review this PR, since this isn't urgent)?

[laurmaedje]

If it's a small diff with no further complication, I'm fine with having it in this PR.

[PgBiel]

Right... I'm having some trouble because of #[fold]. Celled<T> doesn't implement Fold, so I tried to implement Fold for it using something like

impl<T: Fold<Output = T>> Fold for Celled<T> {
    type Output = Self;

    fn fold(self, outer: Self::Output) -> Self::Output {
        match (self, outer) {
            (Self::Value(inner), Self::Value(outer)) => Self::Value(inner.fold(outer)),
            (inner, _) => inner
        }
    }
}

Note the T: Fold<Output = T> bound there; that was necessary for Output to be Self. But that doesn't work, because Sides<Option<T>>::Output is Sides<T>, so <Celled<T> as Fold>::Output must be Celled<T::Output> instead. So I tried something like

impl<T: Fold> Fold for Celled<T> {
    type Output = Celled<T::Output>;

    fn fold(self, outer: Self::Output) -> Self::Output {
        match (self, outer) {
            (Self::Value(inner), Celled::Value(outer)) => Celled::Value(inner.fold(outer)),
            (inner, _) => inner,
        }
    }
}

but that doesn't work because inner is Celled<T>, not Celled<T::Output>.

So I think we'd have to give up on fold for inset if we want this to work properly (or special-case the Fold implementation for Sides<Option<T>>)... though it would be nice to have fold for the (Value, Value) case at least... 🤔

Anyway... maybe we can just leave this to another PR or something so we don't have to think about this now :p

[laurmaedje]

Maybe that's the reason I didn't add it in the first place!

[laurmaedje]

Suggested change

[laurmaedje]

This could maybe also be realized as a Default bound on the trait.

[laurmaedje]

Instead of this reserve, we could also allocate the full cell vector with_capacity upfront and then use a for loop to populate it above. This way we can also skip the if condition here, I think.

[PgBiel]

Do you mean calculating how many cells will be needed with something like let capacity = cells.len() + cells.len() % c, and then doing a for i in 0..capacity instead of looping on cells itself, taking cells.get(i).unwrap_or_else(T::default), resolving it and pushing it to the final vector on each iteration?

That could perhaps work. I'll have to allocate a new vector with with_capacity anyway in my x,y fields PR (since cells will be potentially moved around), though I think I'll have to keep this if with reserve in the next PR, since we can't predict how long the final cells vector will be when cells can have arbitrary positions (we can only predict its minimum length). For this PR, though, this can probably work. (Not sure how it behaves in terms of performance, but hopefully shouldn't be too bad?)

[laurmaedje]

I meant the capacity calculation like you said. I didn't necessarily mean putting everything into a single loop, but that might work as well! I was just thinking that if we reserve everything up-front, then the cells_remaining loop doesn't need to be in an if because it would just run zero times. I'll leave up to you whether a single loop or two loops work better.

[PgBiel]

Oooh!

Actually, initially the if wasn't there, but a lot of tests started failing... Turns out that, if cell_count % c == 0 (the last row is already filled), cells_remaining would be equal to c - 0, a.k.a. c, so it would add more cells than necessary. I couldn't find a better alternative at the time... though, using early allocation as you suggest, we could probably just use let cells_remaining = cells.capacity() - cells.len() instead :p

[laurmaedje]

Ah, I see. That makes sense.

[laurmaedje]

From https://doc.rust-lang.org/std/vec/struct.Vec.html#method.with_capacity:

The vector will be able to hold at least capacity elements without reallocating.

I don't think we can rely on calling cells.capacity(). But we can use the variable we called with_capacity with.

[PgBiel]

I've simplified the overall structure of the code. I couldn't manage to remove the if, unfortunately. Edit: I've managed to remove the if; I just had to recall a fundamental lesson in modular arithmetic: c - cell_count % c might not be within 0..c, but (c - cell_count % c) % c will be :)

I tried to use Vec::with_capacity, but I would necessarily have to change things to use a for loop with push(next()) or something - I thought of using .extend() to avoid doing that, but turns out the usage of SourceResult inside iterators is a pain (I'd have to collect and end up creating yet another vector!). I will have to eventually create that for loop and stuff, but I thought the new code is simpler for now; let me know what you think.

[PgBiel]

Regarding possible capacity optimizations, I believe the iterator holds the correct size hint all throughout the iterator "pipeline" (see here: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=b565d7bb84801927b1029389d1e305de ), so we should probably be fine I think, assuming collect is smart enough.

[laurmaedje]

Thank you!