Render basic tables and detect them in rules

[masonium]

Addresses issue #283 .

This PR adds some internal functions to compute table dimensions (rows and columns) taking rowspans and colspans into account, and uses them to render tables with the :array intent as "table with m rows and n columns".

I'm not super happy with the current logic to detect that an unspecified mtable should have an :array intent. Right now, it assumes that any mtable that either renders its frame as solid or dashed or has an mrow or mtd that specifies a rowspan or colspan should be an :array. I think we want something more general, though.

[rileeki]

@Jeffrey-Kuan

I'm not super happy with the current logic to detect that an unspecified mtable should have an :array intent. Right now, it assumes that any mtable that either renders its frame as solid or dashed or has an mrow or mtd that specifies a rowspan or colspan should be an :array. I think we want something more general, though.

Any thoughts?

[rileeki]

Also, just a note: this PR currently fails one of the CI checks, but so does the main branch. @masonium submitted a separate PR (#506) to fix the test that's failing.

[NSoiffer]

Using frame, etc., is a good idea and will work for legacy MathML so we should keep it. However, in MathML core (which is the future), mtable are supposed to be styled with CSS so that info won't likely be around unless it is a local CSS style (style="...."). If you want, you could add a check for that. But people will likely use a class for that so there is no hope to get that info in general.

One option is to look at the parent of the table and see if has delimiters around it (parens, brackets, vertical bars, double vertical bars, ???).

Comments on the code:
speech.rs:

• why add std::format::Debug and pub trait TreeOrString<'c, 'm:'c, T: Debug> : Debug {?

xpath-functions.rs:

• why is struct CountTableDims pub?
• at least when looking in github's PR files changed tab, the indenting is messed up making it hard to read the code.
• the spanning column code only looks a colspan in the first row. If you are supporting colspan, shouldn't you get this right no matter where the colspan is located. It isn't easy. That's Handle rowspan/columnspan #33.
• rowspan isn't handled at all. No need for CountTableRows if you're not handling these.

mtable.rs:

• there aren't any tests that use rowspan or colspan. It is good that you have a unit test in xpath-functions.rs, but you need to test the actual speech also.

[masonium]

I'll try to address everything. For code-specific comments, it's probably easier for both of us to use the commenting feature on Github (pressing the + next to the line numbers on the diffs) so that it's a bit easier to track individual threads.

Using frame, etc., is a good idea and will work for legacy MathML so we should keep it. However, in MathML core (which is the future), mtable are supposed to be styled with CSS so that info won't likely be around unless it is a local CSS style (style="...."). If you want, you could add a check for that. But people will likely use a class for that so there is no hope to get that info in general.

One option is to look at the parent of the table and see if has delimiters around it (parens, brackets, vertical bars, double vertical bars, ???).

I'll play around with looking for delimiters.

Comments on the code: speech.rs:

* why add `std::format::Debug` and `pub trait TreeOrString<'c, 'm:'c, T: Debug> : Debug {`?

I wanted to debug some of the functions that take a T: TreeOrString using println!("{:?}", ...). Since TreeOrString is meant to be a closed trait, I don't think this hinders future implementation.

xpath-functions.rs:

* why is `struct CountTableDims` pub?

It shouldn't be. Fixed.

* at least when looking in github's PR files changed tab, the indenting is messed up making it hard to read the code.

Fixed. For what it's worth, the existing code also contains some mix of tabs and spaces (see fn test_is_simple in xpath_functions.rs, for instance), causing rendering issues in e.g. Github. My editor usually does a good job of matching what's already in the file, but it's not foolproof. Another benefit of #273 .

* the spanning column code only looks a `colspan` in the first row. If you are supporting `colspan`, shouldn't you get this right no matter where the `colspan` is located. It isn't easy. That's [Handle rowspan/columnspan #33](https://github.com/daisy/MathCAT/issues/33).

* `rowspan` isn't handled at all. No need for `CountTableRows` if you're not handling these.

I adjusted the code so it does a more thorough accounting here, now that I've read the MathML spec and have a reasonable handle on this.

One caveat is that, though the spec doesn't seem to fully address it, extra rows that are implied by a dangling rowspan (for instance, an <mtd rowspan="2"> in the last row) does not seem to rendered, so I'm not counting them towards the row total. If they were, then the CountTableRows function would be necessary. If we agree on assuming that behavior, I can get rid of CountTableRows.

The column count is more explicitly defined as the maximum across all rows.

mtable.rs:

* there aren't any tests that use rowspan or colspan. It is good that you have a unit test in xpath-functions.rs, but you need to test the actual speech also.

I still need to add these.

[Jeffrey-Kuan]

I think @NSoiffer 's suggestion about delimeters is good

[NSoiffer]

I'm not sure if "if row_name== "..." then ... else if ... " or
let row_type = match row_name {
"mlabeledtr" => CTDRowType::Labeled,
"mtr" => CTDRowType::Normal,
"mtd" => CTDRowType::Implicit,
_ => continue,
};
is more idiomatic. It's fine to leave it as is, but I'm throwing that out as alternative as I think I've seen "match" used more often in Rust code.

[NSoiffer]

Maybe I missed it, but I don't think anywhere is checking
name(e) == "mtable"

[NSoiffer]

Apologies for not using the code comment feature. I did this time. In general, it looks much better.

The logic is more complicated, so I feel less sure that I can be confident with my review. Adding some more tests, both unit tests with various rowspan/columnspan settings, and some speech tests would add confidence that the code is correct. A few tests for cases when row/columnspan overlap would be very useful since that is a tricky case.