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

@moritz-gross moritz-gross commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator

No description provided.

moritz-gross and others added 2 commits August 12, 2026 00:20
@moritz-gross
Add HasVisibleColumnLine XPath function to handle table separators …
dff37bf 
…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
Merge branch 'daisy:main' into announce-column-separator
9b5f9ab
@moritz-gross

moritz-gross commented Aug 13, 2026 •
edited
Loading

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

NSoiffer commented Aug 15, 2026

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

moritz-gross commented Aug 15, 2026

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

NSoiffer commented Aug 15, 2026

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 •
edited
Loading

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.

@moritz-gross moritz-gross mentioned this pull request Aug 16, 2026
Open
@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
@michaldziwisz michaldziwisz mentioned this pull request Aug 31, 2026
Merged
NSoiffer pushed a commit that referenced this pull request Sep 2, 2026
@michaldziwisz
pl: fix five speech/navigation defects and catch up with recent en ch…
9ea98c2 
…anges (#738)

* pl: speak up tack (U+22A5) as perpendicular, matching en

Follow-up to 45aefb2 for Polish. Editors commonly emit U+22A5 where
perpendicularity is meant, so both U+27C2 and U+22A5 now say
"jest prostopadle do". LiteralSpeak keeps reading U+22A5 literally
as "dol".

Previously Polish said "dol" for U+22A5, so the same formula was read
differently in Polish than in English.

Adds up_tack_330 to tests/Languages/pl/alphabets.rs, mirroring the
English test.

* pl: adopt the `|` argument-glue syntax from definitions.yaml

Follow-up to 4347d88 for Polish. That commit added the `|` syntax and
updated the Polish tests, but not the Polish rule files, so several
intents lost their function name in speech.

Fixed, with the spoken output before -> after:

  quotient        "podzielone przez z x przecinek, y"
               -> "czesc calkowita z x podzielone przez y"
  remainder       "podzielone przez z x przecinek, y"
               -> "reszta z x podzielone przez y"
  set-difference  "i z wielka a przecinek, wielka b"
               -> "roznica zbiorow z wielka a i wielka b"
  polar-coordinate "przecinek z x przecinek, y"
               -> "wspolrzedna biegunowa z x przecinek, y"

The `|` syntax needs the function-intent rule to call
IntentFunctionUseArityPath / IntentFunctionGlueBefore. Only en and hu
had it, so it is now ported to pl (with "of" -> "z").

Also here:
* SharedRules/geometry.yaml: the `coordinate` rule matched "." instead
  of "not(*[@arg])", so it swallowed coordinate($x,...) intents that
  should fall through to IntentMappings. Its name was also mistranslated
  as "przecinek" (comma) rather than "punkt" (point).
* transpose: fixity order now matches en (postfix first). The
  function form is tested explicitly via intent='transpose:function($x)',
  as en does.
* empty-set: added, it was missing.

NOT adopted: arity templates ("| po | od,do") for sum/product.
intent_function_glue_before in src/infer_intent.rs hardcodes the English
word "of" for the last argument, which yields "suma po i of x" in Polish.
This affects every non-English language; hu avoids it the same way. The
binary separator form works correctly and is what this commit uses.

* pl: fix navigation direction words, unreachable for any translation

Follow-up to ec36e05 and 080ca16 for Polish, but it also fixes a
long-standing Polish bug rather than only porting the new code.

navigate.yaml compared the suffix of $NavCommand (always English, e.g.
"ZoomIn") against a substring offset by the length of the SPOKEN $Prefix.
For English those are the same; for any translation they are not. With
"przybliz" (8 chars) vs "Zoom" (4), "ZoomIn" was cut to "" instead of
"In", so ALL 16 branches were dead:

  ZoomIn        -> ""            (want "In")
  ZoomOutAll    -> "ll"          (want "OutAll")
  MoveNext      -> "t"           (want "Next")
  DescribeNext  -> "ibeNext"     (want "Next")

Users heard "przejdz; do mianownika" with no direction, never
"przejdz w prawo". Using $CommandOffset, as ec36e05 introduced, fixes
all of them.

Two more things here:

* Polish needs two verbs where English reuses "zoom": "przybliz na
  zewnatrz" (zoom in outwards) is self-contradictory, so ZoomOut* now
  says "oddal". The direction word for plain In/Out is dropped, as the
  verb already carries it: "przybliz" / "oddal", and
  "przybliz maksymalnie" / "oddal maksymalnie".
* Ports into-or-out-of-prefix-or-silent-without-parts (080ca16) and the
  SpeakIntentName fallbacks in into-or-out-of-default, keeping our own
  Polish preposition logic (including "ze stopnia" euphony).

Adds zoom_speech_pl and move_char_speech_pl. Both fail on the old
formula, showing the missing direction word, so they do discriminate.

* pl: announce table column and row separators

Follow-up to fbc49bb (#679) for Polish. The HasVisibleColumnLine /
HasVisibleRowLine rules were missing from pl/SharedRules/default.yaml,
so visible lines in a matrix were silent for Polish users, e.g. an
augmented matrix was read exactly like a plain one.

  before: "2 na 3 macierz rozszerzona; wiersz 1; 3, 1, 4; ..."
  after:  "2 na 3 macierz rozszerzona; wiersz 1; 3, 1, separator, 4; ..."

Two existing tests (augmented_matrix_2x3, augmented_matrix_3x4_end_matrix)
were pinning the pre-#679 output; their English counterparts already
expect "separator", so they are updated rather than worked around.

Adds dashed_augmented_matrix_separator and matrix_row_separator,
ported from tests/Languages/en/mtable.rs. Removing either rule fails
all four tests, so they discriminate.

* pl: clear the remaining untranslated unicode entries

The audit tool reported 81 unicode entries whose text equals the English
source. Reviewing them one by one, only two were actually untranslated:

  U+2127  "mhos"  -> "mho"    (unit, as nb and sv have it)
  U+2644  "Saturn"           (Polish spelling is the same; key raised)

The other 75 are correct as-is and only needed the verified key:

  * 30 Roman numerals (U+2160..U+217F) spelled out letter by letter
  * 29 space, PUA and zero-width entries with no speech at all
  *  6 typographic ligatures (ff, fl, ffi, ffl, ft, st)
  * 10 proper names and symbols (spesmilos, paragraphos, hypodiastole,
       digamma, differential d, imaginary j, oV, pH)

Four more in unicode.yaml (digit separator, space, U+2062, U+2063)
likewise carry no translatable speech.

Raising the key on an entry whose text legitimately matches English is
what the convention is for; it is not the same as marking English text
as verified. Only the wording of "mhos" changed - the diff is otherwise
key case only, checked line by line.

Audit's "untranslated" for unicode files: 81 -> 0.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

MathCAT Project Board
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@moritz-gross @NSoiffer