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

[PgBiel]

Adds fields x and y to grid.cell and table.cell, in both ways: not only can you access them in show rules to determine how the cell should be displayed based on its coordinates, but you can also specify x and y manually for a cell to ensure it will be placed at that particular position (however, will raise an error if that would lead to two cells in the same position).
Specifying x but not y will lead the cell to be placed in the first available row in that column (or create a row if needed). Similarly, specifying y but not x will lead the cell to be placed in the first available column in that row (or error if no column is available, since columns are fixed).

Added tests and docs with some examples.

Second part of the second task in #3001

This PR is a draft PR as it depends on #3037
(For a simpler diff, check out PgBiel/typst@table-cells...PgBiel:typst:cell-positioning )

User-facing API

1. You can now have show rules on cells depend on the cell's final x (column) and y (row) coordinates:

// Style based on position
#{
  show table.cell: it => {
    if it.y == 0 {
      strong(it)
    } else if it.x == it.y {
      emph(it)
    } else {
      it
    }
  }
  table(
    columns: 3,
    gutter: 3pt,
    [Name], [Age], [Info],
    [John], [52], [Nice],
    [Mary], [50], [Cool],
    [Jake], [49], [Epic]
  )
}

2. You can now specify where a cell should be placed:

#grid(
  columns: 2,
  [A], [B],
  grid.cell(x: 1, y: 2)[C], grid.cell(x: 0, y: 2)[D],
  grid.cell(x: 1, y: 1)[E], grid.cell(x: 0, y: 1)[F],
)

Implementation details

• In CellGrid::resolve, the grid (vector of cells) is now built from scratch, since cells can now be positioned in any way they wish, so the cells vector would have to be reordered; thus, we create a resolved_cells vector, and iterate over cells to determine the final position of each cell, resolve it, and place it in the appropriate position in the vector (three steps).
• When positioning a cell in an index which would be out of bounds for cells, resolved_cells expands accordingly. But it has to expand with something (e.g. if a cell is placed 900 positions after the last one, what do we fill the empty vector spaces with?). We could expand adding many normal cells with empty bodies, but then those cells would be taking up space unnecessarily - it should be possible to override those "filler" cells later if we want.
  • Therefore, resolved_cells starts out as a vector of Option<Cell> - it has None where a new cell can be placed.
• An absent cell position (None in Vec<Option<Cell>>) can be replaced by a cell later in cells which might want to occupy its space (turning the position to Some(cell)). However, if there are any absent cell positions remaining after rebuilding the grid, they are converted to normal cells with empty bodies (necessary so we can synthesize properties on the empty spaces, e.g. fill or align, and so we can apply show rules on them too).
  • This is done by iterating over resolved_cells and converting Option<Cell> to Cell (just unwrap if the cell is Some, otherwise create a new empty cell).
  • Theoretically this is only necessary if resolved_cells.len() != cells.len(), since the only way a None entry can remain after the grid is built is if resolved_cells was actually expanded. This could lead to a potential optimization here, but that would have to be discussed. For now, this works.

Remaining questions

• (Solved) Should cells specified without overriding any coordinate - x or y - automatically skip cells which are already occupied by cells with arbitrary/custom position? For example,

#table(
   columns: 3,
   [A], [B], [C],
   table.cell(x: 1, y: 0),
)

will error, which makes sense, since [B] was already placed in the grid by the time we got to that last cell. But the following also errors; maybe it shouldn't (and we should just skip the arbitrarily-positioned cell)?

#table(
   columns: 3,
   table.cell(x: 1, y: 0),
   [A], [B], [C],
)

• Conclusion: I made it not error (since that wouldn't be very useful). Instead, cells with fully automatic position (x: auto and y: auto, the default) will just skip non-absent (non-None) positions. (Implemented in c376e57)

Performance implications

In my unscientific tests using documents containing only large tables, tables took about 70-80% longer to cold compile with this PR.

Maybe there isn't too much we can do about this, unfortunately...

TODO

• TODO: Fix problem with incomplete (non-c-large) rows
• TODO: Docs.
• TODO: Merge other PR.

[Julian24816]

Quick suggestion: As this branch depends on the changes on branch table-cells you could set that branch as this PRs target branch (and don't merge this/keep it as draft). This will:

• allow you to review only the changes added by this branch without the changes on table-cells
• once you merge that other PR, the base branch of this PR will automatically be changed back to main by GitHub
• if you review/approve this PR now, this review will not become outdated by merging the other PR

[PgBiel]

Hey @Julian24816 , thanks for the suggestion. I believe the problem with your idea is that this branch is in my fork, so the PR would have to be moved to my fork... Do you know if there's a way around this?

[Julian24816]

Hey @Julian24816 , thanks for the suggestion. I believe the problem with your idea is that this branch is in my fork, so the PR would have to be moved to my fork... Do you know if there's a way around this?

Oh, the Smartphone App hid this important detail from me 😅 sadly I don't have any experience with this topic in cross fork PRs...

[laurmaedje]

Please mark things that you've implemented as resolved. This helps me see whether the PR is ready for another round of review / merging when looking at it. Thanks. :)

[PgBiel]

Sure! I was told otherwise when contributing elsewhere (that the reviewer is responsible for marking threads as resolved), so I've refrained from doing so since then :P. However, when I'm unsure about something, I'll probably keep it open and ask explicitly for feedback, otherwise I'll just resolve it. 👍

[laurmaedje]

I was told otherwise when contributing elsewhere (that the reviewer is responsible for marking threads as resolved)

Interesting. Not sure what the industry practice is, but most contributors here just mark it themselves and it works well enough. Typically, I'd notice anyway that something didn't change when doing another review round. But having the marking definitely helps to decide whether another review round should be done.

However, when I'm unsure about something, I'll probably keep it open and ask explicitly for feedback, otherwise I'll just resolve it. 👍

Sounds good.

[laurmaedje]

Aside from the one conflict, this looks ready to merge.

[PgBiel]

Aside from the one conflict, this looks ready to merge.

I was taking a look, and this conflict seems more significant than I thought; after #3200, we'll probably have to migrate Grid and Table to use Packed<GridCell> and Packed<TableCell> in order to be able to get each cell's span. But I'm kinda unsure regarding which trait impls should go on GridCell/TableCell directly and which should go on Packed<GridCell/TableCell>, considering that Packed<T> seems to implement some traits by itself as well (such as Clone, though that one is probably fine to use by default). I'm thinking that I'll probably have to implement ResolvableCell on Packed<...Cell> instead so we can get a cell's span...
...or maybe we could just force CellGrid to take a vector of Packed<T> where T: ResolvableCell. Not sure.

I'll take a further look at this later today.

[PgBiel]

Okay, well, I'm not very familiar with the new stuff from #3200, but here's what I did:

1. Initially, in 47d4108, I experimented with having CellGrid::resolve take a vector of Packed<T> directly. It worked and was fairly simple, but it did introduce some perhaps unnecessary coupling with Packed, so I set off to change that.
2. In 5c6b9b4, I managed to return CellGrid::resolve to its previous code unchanged by implementing ResolvableCell and Default on Packed<GridCell/TableCell>. However, it did require one or two workarounds due to borrow checking stuff (in particular for the self.push_align(...) stuff since self.align() was being used, leading to concurrent mut / non-mut borrows due to the implicit deref/deref_mut), but right now I think I prefer this approach so as to keep CellGrid::resolve as generic and simplified as possible.

Feel free to give opinions; I can revert 5c6b9b4 if that'd be preferred.

[PgBiel]

Q: If we keep my current approach of implementing ResolvableCell on Packed<GridCell/TableCell> (at 4b37f74), is it fine to mutate the Packed directly like I'm doing, or should I always unpack() inside resolve_cell?

Edit: If unpack() is wiser, I was considering replacing the iteration using .iter().cloned() with just .iter() and having resolve_cell take a shared reference (to force a clone - in this case, unpack() - inside resolve_cell).

[laurmaedje]

It worked and was fairly simple, but it did introduce some perhaps unnecessary coupling with Packed, so I set off to change that.

Everything is coupled with Packed. That's okay.

However, it did require one or two workarounds due to borrow checking stuff (in particular for the self.push_align(...) stuff since self.align() was being used, leading to concurrent mut / non-mut borrows due to the implicit deref/deref_mut), but right now I think I prefer this approach so as to keep CellGrid::resolve as generic and simplified as possible.

Yeah, that's also a thing in a few other parts of the codebase now.

If we keep my current approach of implementing ResolvableCell on Packed<GridCell/TableCell> (at 4b37f74), is it fine to mutate the Packed directly like I'm doing, or should I always unpack() inside resolve_cell?

Mutating the packed is fine. Unpacking is actually worse because it'd lose the spans and other metadata that's stored in the Packed.

TLDR: Everything looks good as-is.

[laurmaedje]

I'm excited for advanced tables slowly coming together. 🎉

[PgBiel]

Same here 🚀
Thanks!