Skip to content

Conversation

PgBiel
Copy link
Contributor

@PgBiel PgBiel commented Feb 10, 2024

Fourth task in #3001.

Note: The rowspans PR (Part 3b) was delayed as we are still figuring out the appropriate algorithm for a particular problem, so, in the meantime, we shall have table line customization!

Note: This PR does not include major changes to docs. These were postponed so review doesn't have to take that into account for now. I've added some basic docs for the new elements and options though (just so they don't go missing).

User-facing API

  • The stroke option of grids and tables was changed: now it simply simply changes the stroke around all cells in the way specified. In particular, #table(stroke: red) is now the same as #table(stroke: (left: red, right: red, top: red, bottom: red)), which means that the stroke above, below, to the left and to the right of each cell is red.

    • This means that table lines, by default, no longer cross gutter spacing (since there are no cells within gutter), unless you use the new mechanism of explicit table lines (see below).
    • Note that you can override each cell's stroke parameter based on its position (similarly to fill and align): #table(stroke: (x, y) => if y == 0 { (top: blue, rest: red) } else { red }) will have the line above each cell at the first row of the table be blue, exceptionally. For any other rows, the table lines will be entirely red around all cells.
  • You can use grid.cell(stroke: ...) and table.cell(stroke: ...) to override the grid/table's stroke option for a particular cell.

  • There are new elements, table.hline (horizontal) and table.vline (vertical) - and likewise for grid - called explicit table lines, which can be used to manually override table stroke (including stroke set through grid.cell(stroke: ...)).

    • They are very configurable, and can cross gutter spacing, because explicit lines are not limited to a single cell (they can span multiple cells, including the gutter spacing between them).
    • You can use start: and end: to control their lengths, and x: (for vlines) or y: (for hlines) to specify in which position they appear. By default, they will appear after the last cell you specified, for convenience (a familiar experience for booktabs users).
    • Note: hlines above the first row (index 0) and below the last row (index {amount of rows}) of the table are part of the table border and repeated in every page.
  • Check them in action: (credit to @igidrau for the double-line code)

image

#let double-line = pattern(size: (1.5pt, 1.5pt), {
  place(line(stroke: .6pt, start: (0%, 50%), end: (100%, 50%)))
})

#table(
  stroke: (_, y) => if y != 1 { (bottom: black) },
  columns: 3,
  table.cell(colspan: 3, align: center)[*Epic Table*],
  align(center)[*Name*], align(center)[*Age*], align(center)[*Data*],
  table.hline(stroke: (paint: double-line, thickness: 2pt)),
  [John], [30], [None],
  [Martha], [20], [A],
  [Joseph], [35], [D]
)
  • See the repetition of border hlines - at the top and bottom - in action:

image

#set page(height: 5em)
#table(
  columns: 3,
  inset: 3pt,
  table.hline(y: 0, end: none, stroke: 3pt + blue),
  table.vline(x: 0, end: none, stroke: 3pt + green),
  table.hline(y: 5, end: none, stroke: 3pt + red),
  table.vline(x: 3, end: none, stroke: 3pt + yellow),
  [a], [b], [c],
  [a], [b], [c],
  [a], [b], [c],
  [a], [b], [c],
  [a], [b], [c],
)

Implementation details

API-related changes

  • The stroke field of GridElem and TableElem is now Celled<Sides<Option<Option<Arc<Stroke>>>>>.
    • It represents the grid-wide override for each cell's stroke, which has four sides.
    • Arc is used in an attempt to reduce unnecessary overhead when cloning strokes.
    • As a consequence, table lines, by default, will no longer cross gutter spacing, unless you use explicit hline/vline overrides, explained below.
  • The internal Cell type now has a stroke: Sides<Option<Arc<Stroke<Abs>>>> field for each cell's resolved individual stroke overrides. To reflect this, GridCell and TableCell - exposed to the user - now have stroke: Sides<Option<Option<Arc<Stroke>>>> fields to be able to define each cell's individual stroke override.
  • The per-cell stroke override mechanisms are similar to that of other cell properties (fill, align and inset), through the ResolvableCell trait, with CellGrid::resolve receiving the grid-wide value to pass on to each cell.
    • Of note, I had to create a dedicated ResolvedCelled type in order to be able to add #[resolve] to the stroke fields mentioned above, because, when the Celled is a Func, it has to be resolved later - when the function is actually called. So, resolving a Celled will return a ResolvedCelled.
  • Grids' and tables' stroke fields are still #[fold] as, due to Refactor folding #3294 , I could implement Fold on Celled<T> which simply returns another Celled<T>. In my implementation, actual folding only occurs when both folded values are Celled::Value. When one is an array or a function, the innermost wins, to simplify things. (Not sure if this is the best behavior but I think it's fine. Less "magical", so to speak, I guess. But we could in theory change it so arrays can fold as well somehow.)
  • Created the elements grid.hline, table.hline (horizontal lines); grid.vline and table.vline (vertical lines) for maximum flexibility regarding table lines.
    • They are meant to override cell strokes, and can span multiple cells and cross gutter spacing.
    • They have several properties, including their index, start and end range, position (start (default) or end for vlines and top (default) or bottom for hlines - indicates whether they appear before or after the track with their index, which is only relevant when using gutter), and stroke.
    • They do not implement Show and thus are only usable in the context of grids and tables. Show rules, therefore, are also irrelevant for them. However, they are still affected by set rules.
  • Hlines and vlines are specified alongside cells in a grid or table, which now have dedicated GridChild and TableChild enums (hline/vline/cell) for their children, which are translated to the internal GridItem enum (hline/vline/cell) received by CellGrid::resolve.
    • When a line's index (y for hlines and x for vlines) is omitted / specified as auto (default), the line will be placed after the last automatically placed cell in the grid. If no such cell has been placed yet in the grid, it is placed at index 0.
    • Lines placed at invalid indices cause an error. This is handled in CellGrid::resolve, which keeps a buffer of all hlines and checks them all at the same time after placing all cells (because the amount of rows isn't known until then). Vline validation, however, is always possible (since the amount of columns is fixed) and occurs immediately after processing each vline.
    • CellGrid::resolve also produces an error if end < start for a line. Though this is kinda optional and doesn't make much of a difference.
  • The table's border lines - the lines which appear at the top, bottom, left and right of each page of the table - are precisely the final table strokes (including relevant cell strokes and lines) above the first row (top); below the last row (bottom); before the first column (left); and after the last column (right).
    • No option has been added to customize just that for now. We could add some more convenient option for outside stroke later. For now, one has to add hlines / vlines / cell strokes at those particular positions to set table borders.
  • CellGrid now has the fields hlines and vlines to hold the final, resolved lines. They are vectors of vectors of lines (internal Line type holding common line properties) - one vector for each row (hlines) and for each column (vlines), including one extra vector for lines at the maximum index (which isn't before any row, but rather after the last row).
    • This was chosen - instead of one mega vector with all hlines and vlines - so we don't have to go through the entire list of hlines and vlines on each column / row, thus speeding things up. However, we don't know yet how often hlines and vlines will be used, so it's possible that this could actually be detrimental for performance and/or memory usage. Not sure.

Layout changes

  • Created a new layout/grid/lines.rs file and moved all line-related algorithms to it (other than rendering).
    • This includes the algorithm for vline splitting, which was renamed to generate_line_segments as it is now used for hlines as well.
    • The relevant unit tests were also moved.
  • generate_line_segments was changed: it will now make a single run across a given line index to find all relevant lines with that index and resolve the final strokes for each position at that index. And, instead of producing a vector of final line segments, it now returns an iterator which yields a line segment each time it has been finished - that is, a segment starts being built as soon as a non-None stroke is found, and it is yielded / returned to the iterator's next as soon as there's a track for which we have to pick a different stroke (either None or something else), which happens because a particular cell chose a different stroke, or the user placed a line override etc.
    • We also have an additional check for when all tracks were traversed: if the current segment wasn't interrupted before that, then we interrupt it at the end, since that means we reached the end of the region so the current line segment is complete.
    • The iterator is used so we don't have to create additional vectors when we will be pushing to one global vector of lines, as explained later.
  • In render_fills_strokes, we now push all lines to a vector before drawing them.
    • This is so we can sort them later based on thickness. Lines with larger thickness are drawn on top, producing better visuals when they conflict with other lines. This seems to be what's done by web browsers with HTML tables as well (at least Firefox).
    • The line vector is sorted from bottom to top layer, hence why vertical lines are being pushed first now, because they default to appearing below horizontal lines when both have the same thickness.
    • Since we use prepend to add each line to the frame, we have to iterate the vector in reverse so that lines appear with the correct priority.

TODO

  • TODO: Create draft PR
  • TODO: Tests
  • TODO: Code cleanup
  • TODO: Decide about horizontal, vertical APIs (delayed for now)
  • TODO: Decide about table.vline(position: left | right) or table.vline(position: start | end)
  • TODO: Undraft PR

@PgBiel PgBiel mentioned this pull request Feb 10, 2024
18 tasks
Table defaults to default stroke, Grid doesn't (no stroke).
This is used in FromValue.
On split_vlines, we always go through every row in the current column,
so we in particular we go through every vline in this column going
through the current row and fold their strokes with the current
automatic line's stroke and with the strokes of the cells around the
vline.

A similar procedure should be done for horizontal lines.
This should make it possible to reuse its code for horizontal lines
Make sure we can properly check for errors, and also that hlines
maintain the appropriate order.
Ensure lines with larger thickness are drawn on top, avoiding smaller
hlines going "inside" larger vlines.
- table.stroke is now just an override for cell.stroke, it is nothing
  special. In particular, #table(stroke: red) and #table(stroke: (_, _)
=> red) are now the same.
- therefore stroke never crosses gutters by default now
- changed folding priorities: hline/vline now have max priority over
  cell strokes. They will be used for full control over the table's
lines, and they can cross gutters like before.
- removed outside stroke for the time being.
Now table stroke overrides per-cell stroke instead of lines, and thus
gutter spacing is not crossed.
This is because it is visually swapped when RTL is enabled.
use explicit vlines for that purpose
Actually test the case in which the grid doesn't specify anything for a
side, thus folding (unspecified side) with (unspecified side), which
should appear as 'none' in the grid.cell show rule
@PgBiel PgBiel marked this pull request as ready for review February 13, 2024 23:43
Copy link
Contributor Author

@PgBiel PgBiel left a comment

Choose a reason for hiding this comment

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

Some remaining questions:

Copy link
Member

@laurmaedje laurmaedje left a comment

Choose a reason for hiding this comment

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

Two extra notes on no particular point of the code:

  • Decide about horizontal, vertical APIs (delayed for now).

    Don't we effectively have that now thanks to Sides? E.g. #table(stroke: (x: red), ..). Though I do not understand why stroke: (x: none, y: red) is different from stroke: (y: red). I think the latter should do the same as the former.

  • Side Note: When you use grid.cell in table or vice versa, it just has no effect and proceeds completely silently. I thought that colspan is completely broken for a moment because I used a grid cell in a table. That's a bit of a pitfall. In principle, I like how the lines don't implement Show, but I guess it's not really an option for cells.

@PgBiel
Copy link
Contributor Author

PgBiel commented Feb 17, 2024

Two extra notes on no particular point of the code:

  • Decide about horizontal, vertical APIs (delayed for now).

    Don't we effectively have that now thanks to Sides? E.g. #table(stroke: (x: red), ..).

I meant more as a way to specify hlines and vlines (not cell strokes) in bulk, which can be useful when you have gutter. But I delayed that discussion because I'd like to see proper use cases for that first.

Though I do not understand why stroke: (x: none, y: red) is different from stroke: (y: red). I think the latter should do the same as the former.

It's really just a property of #[fold], which folds with whatever the default value is. I think it's consistent with other places in Typst where we have a default value for a parameter. But I could be wrong.

(In other words, the behavior here should be whatever is consistent with the rest of Typst. But I'd also have to somehow override what #[fold] does to change that.)

  • Side Note: When you use grid.cell in table or vice versa, it just has no effect and proceeds completely silently. I thought that colspan is completely broken for a moment because I used a grid cell in a table. That's a bit of a pitfall. In principle, I like how the lines don't implement Show, but I guess it's not really an option for cells.

Yeah, I myself have faced this problem multiple times during tests 😅

It's not too simple... I think removing Show from cells would probably mess up show rules. If not for that, it would be doable (we could just manually apply the "default show rule" then).

Perhaps we could just reject cells of the wrong type for table and grid. Or at least add a warning for it. Not sure.

Though I guess a problem with adding an error or warning is that you might be using content from an external source (a parameter or something), and you wouldn't want to inadvertently error deep down based on some decision by the user at the highest level...

Eh... I don't know.

EDIT: Actually, now that I think about it, even right now the user could pass the appropriate cell element to your function, for example table.cell when you're going to place something in a table, and that could cause your table to misbehave / look differently.

The workaround? When placing content in your table which comes from a parameter, write table.cell(content) instead of just content. That way, you are explicitly opting out of external users fiddling with your table's layout.

As such, perhaps adding an error is appropriate, to force library authors to think about this case.

They are now pushed into vectors of shapes which are prepended in bulk.
Depending on its stroke's source, a line segment may be drawn on top of
others.
Per-cell stroke overrides are now assigned a higher priority than
global grid stroke.
@laurmaedje
Copy link
Member

It's really just a property of #[fold], which folds with whatever the default value is. I think it's consistent with other places in Typst where we have a default value for a parameter. But I could be wrong.

#table(
  columns: (1cm, 1cm, 1cm),
  stroke: (y: red),
  [a], [b], [c],
  [a], [b], [c],
)

#block(stroke: (y: red), inset: 5pt)[hello]
#rect(stroke: (y: red))[hello]

shows that something is different with tables for some reason.

I think removing Show from cells would probably mess up show rules. If not for that, it would be doable (we could just manually apply the "default show rule" then).

Manually applying the default show rule would indeed mess with show rules.

Perhaps we could just reject cells of the wrong type for table and grid. Or at least add a warning for it. Not sure.

That does sound reasonable (making it error).

Though I guess a problem with adding an error or warning is that you might be using content from an external source (a parameter or something), and you wouldn't want to inadvertently error deep down based on some decision by the user at the highest level...

I think that would be very rare as most functions wouldn't expect users to pass grid or table cells in the first place. And even then the error would be at least somewhat helpful.

The workaround? When placing content in your table which comes from a parameter, write table.cell(content) instead of just content. That way, you are explicitly opting out of external users fiddling with your table's layout. As such, perhaps adding an error is appropriate, to force library authors to think about this case.

I don't think library authors should have to think about this case or do that. There is just no sane reason for a user to do this and if they do, it's on them.

@PgBiel
Copy link
Contributor Author

PgBiel commented Feb 18, 2024

It's really just a property of #[fold], which folds with whatever the default value is. I think it's consistent with other places in Typst where we have a default value for a parameter. But I could be wrong.

#table(
  columns: (1cm, 1cm, 1cm),
  stroke: (y: red),
  [a], [b], [c],
  [a], [b], [c],
)

#block(stroke: (y: red), inset: 5pt)[hello]
#rect(stroke: (y: red))[hello]

shows that something is different with tables for some reason.

block's default is Sides::splat(None), so it's consistent with grid, which has the same default - here we fold with none; and rect's default is Smart::Auto, since it takes auto stroke as an option to mean "default to 1pt + black if fill is none, otherwise no stroke", so it's folding with auto. For tables in particular, the default is 1pt + black, so it folds with that. (At least that's what the code generated by the elem proc macro seems to do.)

We could add auto as an option like rectangle if that'd be desired, though in principle it feels like we lose information about a table's stroke like that (it.stroke in some table show rule would just return auto, unless we use some form of synthesis like cells).

I think that would be very rare as most functions wouldn't expect users to pass grid or table cells in the first place. And even then the error would be at least somewhat helpful.
--snip--
I don't think library authors should have to think about this case or do that. There is just no sane reason for a user to do this and if they do, it's on them.

I generally agree, but I think the vast majority of cases where this would be relevant would be in show rules for grid.cell and table.cell - think strong(it) , for example.

Either way, as you say, the error would be helpful enough to let you work around it by using the appropriate cell function. Or even just wrapping your cell in a single-element sequence or some other element.

@laurmaedje laurmaedje added this pull request to the merge queue Feb 20, 2024
Merged via the queue into typst:main with commit 4873312 Feb 20, 2024
@PgBiel PgBiel deleted the line-customization branch February 20, 2024 15:46
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.

2 participants