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

PgBiel · 2023-12-21T15:19:35Z

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

GridLayouter and related types (including Celled<T>) are now in a new file: layout/gridlayouter.rs.
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).
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.
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.
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.
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:

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.
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.
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 · 2023-12-21T15:32:13Z

crates/typst/src/layout/gridlayouter.rs

+
+        self.finish_region(engine)?;
+
+        self.render_fills_strokes()?;


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?

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

Enable show rules

table.typ and grid-styling.typ were getting cluttered

laurmaedje · 2024-01-03T12:25:39Z

crates/typst/src/layout/grid.rs

-        );
+        )?;
+
+        // Prepare grid layout by unifying content and gutter tracks.


I think this comment is outdated.

laurmaedje · 2024-01-03T12:26:38Z

crates/typst/src/layout/grid.rs

+///   // Creating a temporary #{ ... } scope to avoid affecting other grids
+///   // with this show rule.
+///   // Here, we will italicize all cells.
+///   show grid.cell: emph


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.

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.

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.

Sure, removing sounds fair for now.

crates/typst/src/layout/grid.rs

laurmaedje · 2024-01-03T12:38:40Z

tests/ref/layout/grid-cell.png

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.

crates/typst/src/layout/gridlayouter.rs

Co-authored-by: Laurenz <laurmaedje@gmail.com>

crates/typst/src/layout/grid/mod.rs

PgBiel · 2024-01-04T12:49:11Z

crates/typst/src/layout/grid/mod.rs

+    /// ```
+    #[fold]
+    #[default(Sides::splat(Abs::pt(0.0).into()))]
+    pub inset: Sides<Option<Rel<Length>>>,


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)?

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

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

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

laurmaedje · 2024-01-04T14:48:39Z

crates/typst/src/layout/grid/layout.rs

+pub struct Cell {
+    /// The cell's body.
+    pub body: Content,
+


Suggested change

laurmaedje · 2024-01-04T14:49:13Z

crates/typst/src/layout/grid/layout.rs

+
+    /// Creates a cell with empty content.
+    /// Needed to fill incomplete rows.
+    fn new_empty_cell() -> Self;


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

crates/typst/src/layout/grid/layout.rs

laurmaedje · 2024-01-04T14:50:38Z

crates/typst/src/layout/grid/layout.rs

+        let cell_count = cells.len();
+        if cell_count % c != 0 {
+            let cells_remaining = c - (cell_count % c);
+            cells.reserve_exact(cells_remaining);


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.

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?)

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.

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

Ah, I see. That makes sense.

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.

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.

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.

crates/typst/src/layout/grid/layout.rs

laurmaedje · 2024-01-05T19:30:13Z

Thank you!

PgBiel mentioned this pull request Dec 21, 2023

Tracking issue: More flexible tables #3001

Closed

18 tasks

PgBiel commented Dec 21, 2023

View reviewed changes

Heinenen mentioned this pull request Dec 22, 2023

Make Tufte-style tables achievable, and possibly the default. #226

Closed

PgBiel mentioned this pull request Dec 23, 2023

Table cell x and y fields [More Flexible Tables Pt.2b] #3050

Merged

4 tasks

PgBiel added 26 commits January 2, 2024 09:53

move gridlayouter to its own file

6792d71

create Cell trait

e881cbd

remove allow(dead_code) from GridLayouter fill,stroke

164a69c

create GridCell,TableCell

b70c15c

create CellGrid

354a6d9

create ResolvableCell

ccee965

impl (Resolvable)Cell for Grid/TableCell

994fdcd

pass CellGrid to GridLayouter

7e768d2

use Show for Grid/TableCell

12b8333

Enable show rules

add grid/table.cell tests

2b03d0a

refactor: use self.grid on GridLayouter

a6187b8

refactor: move GridLayouter#cell to CellGrid

f742e17

remove (now unused) 'is_rtl' field from layouter

d384386

change Cell trait to require a fill

2eb4901

implement per-cell fill

c155738

update grid styling tests with per-cell fill

c23305a

deduplicate Show code for GridCell/TableCell

50ddad8

implement and use Smart::or_else

32413fa

update grid and table docs

366bcc9

add doc examples to tests

23c1b41

fix Smart docs

aed317b

adjust comment lengths

80eccc9

fix coordinates with gutter

3fe24b8

test grid/table styling with gutters

bba3523

move grid/table cell tests to separate files

09fceaa

table.typ and grid-styling.typ were getting cluttered

remove unneeded collect (oversight)

1904f74

laurmaedje reviewed Jan 3, 2024

View reviewed changes

PgBiel and others added 8 commits January 3, 2024 11:13

use simple underscores for x, y

8798f2b

Co-authored-by: Laurenz <laurmaedje@gmail.com>

CellGrid: move 'cells' method under constructors

101d598

CellGrid: rename 'new_resolve' to 'resolve'

86a7339

remove outdated comments

2d3c4d2

Merge branch 'main' into table-cells

4c02090

refactor: move grid,gridlayouter to grid folder

d716747

use aqua fill on cell tests

ccb788f

remove show grid/table cell examples

3d7f812

PgBiel commented Jan 4, 2024

View reviewed changes

laurmaedje reviewed Jan 4, 2024

View reviewed changes

PgBiel added 11 commits January 4, 2024 12:13

forgot these underscores

fe231e8

only store has_gutter on grid

f09101f

remove unnecessary newline

b1d0a5f

remove GridLayout type

4e1d8b1

refactor: resolve uses Default and slice

ff8df39

remove 'if' in 'cells_remaining'

8db440b

fold per-cell align and inset

5dff5d2

add per-cell fold tests

944530b

fix per-cell folding

d01fe1e

add tests for folding with auto align

aaa8896

move some tests up

8c8439f

laurmaedje added this pull request to the merge queue Jan 5, 2024

Merged via the queue into typst:main with commit 8fa573a Jan 5, 2024

PgBiel deleted the table-cells branch January 5, 2024 19:34

PgBiel mentioned this pull request Feb 7, 2024

Incorrect interaction between rtl languages and table columns #3373

Closed

1 task

memeplex mentioned this pull request Mar 8, 2024

typst reader jgm/pandoc#8740

Closed

PgBiel mentioned this pull request Mar 13, 2024

Ensure grids have at least the given amount of rows #3644

Merged

Uh oh!

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

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

Uh oh!

Conversation

PgBiel commented Dec 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of changes

User-facing API

Implementation details

Alternative design: Generics

Possible problems and regressions

Uh oh!

PgBiel Dec 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PgBiel Jan 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PgBiel Jan 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PgBiel Jan 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PgBiel Jan 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

laurmaedje commented Jan 5, 2024

Uh oh!

Uh oh!

PgBiel commented Dec 21, 2023 •

edited

Loading

PgBiel Dec 21, 2023 •

edited

Loading

PgBiel Jan 5, 2024 •

edited

Loading

PgBiel Jan 4, 2024 •

edited

Loading

PgBiel Jan 4, 2024 •

edited

Loading

PgBiel Jan 5, 2024 •

edited

Loading