-
Notifications
You must be signed in to change notification settings - Fork 57
Speech API without global state #374
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
src/interface.rs
Outdated
| } | ||
|
|
||
| fn cleanup_mathml_with_rules<'a>(_rules: &SpeechRules, mathml: Element<'a>) -> Result<Element<'a>> { | ||
| // TODO: Canonicalization does not seem to actually use rules? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a question about this: I haven't been able to find where canonicalization actually uses rules, but the comment here seems to indicate that it does?
Commenting out rule loading in the canonicalize tests also doesn't cause any test failures.
Lines 109 to 111 in ca4b933
| // We need the main definitions files to be read in so canonicalize can work. | |
| // This call reads all of them for the current preferences, but that's ok since they will likely be used | |
| crate::speech::SPEECH_RULES.with(|rules| rules.borrow_mut().read_files())?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Canonicalization uses the preference files (e.g., for decimal and block separators to fix up numbers). It also reads the definition files so it knows what are function names (and other things I think) for restructuring the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I think I have something sensible now.
The new stateless interface in the stateless_interface.rs. It is speech-only but it'll be possible to extend to Braille in the future if someone needs it.
The other new files, such as element_util.rs, contain functions moved out of interface.rs so that they can be reused in the stateless version.
Canonicalization code is modified to allow for stateless use (it is also deduplicated a bit thanks to the new CanonicalizeContextPatternsOptions struct)
06e9380 to
e14807a
Compare
|
@NSoiffer I got somewhere with this but running the example I wrote results in a strange error: cargo build --examples
target/debug/examples/stateless-example
I'd appreciate help figuring this one out. The code still uses global variables in some places (e.g. xpath) but I somewhat expected it to run at this point. |
Makes `interface.rs` file easier to navigate. Also extracted `enable_logs` to `logs.rs`. This is a refactoring that I've extracted from NSoiffer#374, where these functions are used by both the stateful and the stateless interface and I wanted to avoid any dependencies from `stateless_interface` on `interface`.
Extracted from NSoiffer#374.
|
This PR has become too huge to review, so I've started sending bits of it that are not about statelessness as separate PRs with the hope of reducing the diff here. |
|
I apologize that it has taken forever for me to get to looking at this... Instead of pushing to main, the work needs to be in a branch. Since I don't think you can create a branch in the MathCAT repo, I've created one for this work: stateless. I looked at some of the conflicts and I don't understand how you plan to make this work and still have (normal) MathCAT run. For example, it appears that one of the conflicts in interface.rs is that you have removed the functions that add ids. Those need to stick around for standard MathCAT functionality (they provide a hook for synchronized speech highlighting). Maybe you should add a "stateless" "feature" to MathCAT similar to "include-zip" or "enable-logs". This allows you to include or exclude code based on whether that feature is set or not. I'll try to be more proactive about your needs going forward. Again, apologies about my slowness. |
This is a draft PR for #372 to demonstrate a Speech API that does not rely on mutable global state, while preserving the existing APIs that do rely on global state.