remove speech-rule-engine integration

[pkra]

Resolves #207

[pkra]

Ready for review.

[pkra]

@dpvc @zorkow I did a poll on twitter (some people responded in tweets, but those were all against "prefixes"); not exactly scientific but I think we should drop the equation title. What do you think?

(FWIW I have been thinking about proposing a redefinition of role=math, and prefixes would be one item that AT could provide an opt-in for.)

[pkra]

Also, https://github.com/pkra/mathjax-node-sre is now actually working.

[pkra]

I think we should drop the equation title. What do you think?

I've updated the commit.

[zorkow]

Certainly weird. It seems to duplicate comments.

[pkra]

I mostly dislike that it opens a new review when I'm just trying to respond to your comments. Especially since I'd have assumed that I can't review my own PR.

[zorkow]

Well, I sometimes review my own PRs in SRE, but remember github complaining, that I can't open a review on my own PR. But apparently you can review someone's review on your PR. Is that a meta-review???

Anyway, I would have thought that it clearly groups replies chronologically, if that makes sense.

[pkra]

@zorkow I've refactored the test.

[dpvc]

Here are a few other comments that I couldn't add as line comments, since they are not about lines in the change file.

• The blank line at 600 is not needed. (There are some other spacing issues that Volker may complain about as well...)

• Lines 740 to 742 are no longer needed, since the results.mml is only created when actually needed (it used to be created for speech creation even when not requested by data.mml, and had to be cleaned up here). If you end up going with optional speech string creation, as I requested above, then you might use similar code to this to remove results.speakText when not needed.

• The speech options need to be removed from the command-line tools.

I looked over the file as a whole, and found some issues that don't come directly from this pull request, but should be handled at some point anyway:

• At line 606, you no longer need a separate variable (it used to be used to be needed for writing the external files, but that was removed earlier).

• At line 573 and line 615, the nodes being returned should be removed from the DOM before being sent back in the results object (otherwise the caller then has access to the internal jsdom document). Alternatively, they could be cloned rather than disrupting the DOM.

• At line 718, MathJax.OutputJax.CommonHTML can be replaced by CHTML, since that is defined earlier.

• Similarly, at line 750, MathJax.ElementJax.mml can be replaced by MML.

• In lines 616 through 618, should these be svgWidth, svgHeight, and svgStyles, since they are specific to the SVG output?

These are what I see at this point. Hope that helps.

[pkra]

The speech options need to be removed from the command-line tools.

Since you suggested the switch should stay, they should stay as well.

I looked over the file as a whole, and found some issues that don't come directly from this pull request, but should be handled at some point anyway:

I suggest we file those separately.

[pkra]

I hope I've addressed all review comments.

[pkra]

Sorry. I broke the tests (again).

[pkra]

Fixed the tests.

[dpvc]

The speech options need to be removed from the command-line tools.

Since you suggested the switch should stay, they should stay as well.

There were several options to control SRE that are no longer needed and should be removed from them. Also, now that the default for speakText is true, you may want the command line tools to default that way as well, which they currently don't.

[dpvc]

Also, you will want to remove the *2stree command-line tools, since those functions have been removed.

[pkra]

There were several options to control SRE that are no longer needed and should be removed from them.

Right. But cf #208, I still think they should all live in an entirely separate package anyway.

[pkra]

Also, now that the default for speakText is true, you may want the command line tools to default that way as well, which they currently don't.

Looks like they do?

[pkra]

PTAL.

[pkra]

Looks like they do?

Ah, I was wrong about that. Confusing the type with the default. Will add a default.

[pkra]

PTAL.

[pkra]

I've been doing a bit of testing today and realized that we should change the SVG approach: a title tag has a better chance of actually producing a result on AT.

[pkra]

PTAL.

[dpvc]

I'm OK with it at this point. @zorkow needs to accept your changes to complete his review. Then we can merge.

[zorkow]

Get rid of the superfluous "var" declarations. Then it's OK.

[pkra]

Get rid of the superfluous "var" declarations. Then it's OK.

Uhm. Which commit were you seeing these on? I have trouble spotting them :-(

[pkra]

Get rid of the superfluous "var" declarations. Then it's OK.

Cleared up F2F.