Skip to content

Announce column separator - #679

Merged
NSoiffer merged 16 commits into
daisy:mainfrom
moritz-gross:announce-column-separator
Aug 16, 2026
Merged

Announce column separator#679
NSoiffer merged 16 commits into
daisy:mainfrom
moritz-gross:announce-column-separator

Conversation

@moritz-gross

Copy link
Copy Markdown
Collaborator

No description provided.

moritz-gross and others added 2 commits August 12, 2026 00:20
…in speech output

- Replace `count_table_dims` return type with `usize` values for clarity.
- Update function registration to include `HasVisibleColumnLine`.
- Modify speech tests to reflect separator usage in matrix descriptions.
@moritz-gross

moritz-gross commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator Author

@NSoiffer some questions:

  • how should we announce the separator? Just as "separator" ?
  • the way add_builtin_functions works is a bit unclear to me. First off, shouldn't it be called something like register_mathcat_xpath_functions, as we register MathCAT-specific XPath functionality? Also, I think it's a bit clunky that these functions need to be attached to an otherwise empty struct each time. Is there no way around that?
  • Isn't the signature cleaner when count_table_dims returns a tuple of usize or i32 etc? idk how I feel about Value<'d> (tbh I need to learn a bit more Rust in the first place to understand what 'd is doing in the first place. I'm leaving that part unchanged for now, but I feel like the way we use the Rust type system is more laborious than it should be in some places.

@moritz-gross
moritz-gross marked this pull request as ready for review August 14, 2026 15:28
@NSoiffer

Copy link
Copy Markdown
Collaborator

"seperator" sounds reasonable to me.

register_mathcat_xpath_functions is better. ASFAIK, this is how you do scope the implementation in Rust. I believe has_visible_column_line should be in the "impl".

It seems like you only have a partial implementation. Block/partitioned matrices have both row and column lines. See https://en.wikipedia.org/wiki/Block_matrix.

The <'d> in Value<'d> is the lifetime of that value. When you have inputs and outputs, Rust needs to make sure that the lifetime of the output doesn't exceed the lifetime of the input (dangling pointer). Lifetimes are (I think) among the more complicated parts of Rust. They have improved the compiler over the years so that you don't need to specify the lifetime as much now as you use to have to do.

@moritz-gross

Copy link
Copy Markdown
Collaborator Author

It seems like you only have a partial implementation. Block/partitioned matrices have both row and column lines. See https://en.wikipedia.org/wiki/Block_matrix.
So you mean we should announce both vertical and horizontal separators? So we need to use rowlines in addition to columnlines?

@moritz-gross moritz-gross moved this from Triage to In progress in MathCAT Project Board Aug 15, 2026
@NSoiffer

Copy link
Copy Markdown
Collaborator

Yes, both row and column lines should be announced.

Although saying "partition" is good, I want to mention another alternative: say where they are upfront. For example "the 3 by 3 matrix with partitions after row 1 and after column 2". If there is more than one row or column partition, then "... after rows 1 and 3...". Again, I'm not saying you should switch, I'm just mentioning an alternative that might be easier. Potentially both could be used. Although redundant, the first serves as part of an overview.

@moritz-gross

moritz-gross commented Aug 16, 2026

Copy link
Copy Markdown
Collaborator Author

I've added support for row separators, and they are announced as "row separator", as imo they should be clearly distinguishable from column separators, which are more common from my experience (just think of linear systems for example).
Also, as we iterate row-wise, the row-separator is only called out once anyway, so it's not so problematic if it takes up more time.

the overview also sounds good, but I'd scope it into a next issue.

@NSoiffer
NSoiffer merged commit fbc49bb into daisy:main Aug 16, 2026
8 checks passed
@github-project-automation github-project-automation Bot moved this from In progress to Done in MathCAT Project Board Aug 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants