Skip to content

Refactor parts of initial KaTeX handling #1478

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

Merged
merged 12 commits into from
Apr 25, 2025
Merged

Refactor parts of initial KaTeX handling #1478

merged 12 commits into from
Apr 25, 2025

Conversation

gnprice
Copy link
Member

@gnprice gnprice commented Apr 22, 2025

This follows up on #1408 (KaTeX part 1) with some refactors that I felt were easiest to express by demonstration. I think it'd be good for the next PR in the series, #1452, to be rebased atop these.

This doesn't include the changes I've sketched for passing the font size down directly through the recursion in the parser and widgets. I'll want to look a bit more at how those would interact with the changes in #1452 (which introduce a number of additional length-valued properties) before I'm confident just what shape that part of the API should take.

Selected commit messages

3ac0037 katex [nfc]: Increment class index at top of loop

This way there's nothing that needs to happen at the bottom of the
loop, if any of the cases matched.

b003242 katex [nfc]: Join the CSS-class switch statements into one

Conveniently, the two redundant rules say the exact same thing
when they apply. So the first one has no effect, and we can
ignore it.

b8186a7 katex [nfc]: Replace classFound local with a default case

Before this change, all cases of this switch statement either
continue the loop, or throw, or set classFound to true.
The error therefore gets logged just if none of the cases matched.

So we can express the same behavior with a default case.

9854bb8 katex [nfc]: Increment class index immediately on dereference

This makes the reasoning about these index values more local.

31e0bf0 katex [nfc]: Handle error immediately on spanClasses overrun

Like an early return, this (a) brings the consequence of the error
immediately next to the condition defining it, and (b) lets the normal
happy case continue vertically down without adding indentation.

d76120a katex [nfc]: Make KatexSpanStyles immutable

Once the parsing is done, we want these to remain unchanged,
just like the other objects in the parse tree.

So, like ContentNode and its subclasses, make the class immutable.
The parser needs to mutate its own draft of what styles to apply
to a given span; but it can do that with its own local variables
corresponding to the fields, and construct a styles object at the
end of the loop.

6bb726a katex [nfc]: Factor out _KatexNodeList widget

This deduplicates the logic for the particular way that a list of
KaTeX nodes get combined into a single widget.

0eb1410 katex [nfc]: Rename _KatexSpan field to "node"

This makes it more uniform with our other content widgets.

Copy link
Member

@rajveermalviya rajveermalviya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this @gnprice! Added one comment about another possible cleanup in the tests, otherwise LGTM.


KatexSpanStyles({
const KatexSpanStyles({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, now that this is immutable, we can also change the ContentExample tests back to const in test/model/content_test.dart:

-  static final mathBlock = ContentExample(
+  static const mathBlock = ContentExample(

I'll add a commit that does that in the next PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sent that as d3b21fd in #1452.

@gnprice
Copy link
Member Author

gnprice commented Apr 25, 2025

Thanks for the review! Merging.

gnprice added 12 commits April 24, 2025 17:22
This way there's nothing that needs to happen at the bottom of the
loop, if any of the cases matched.
Conveniently, the two redundant rules say the exact same thing
when they apply.  So the first one has no effect, and we can
ignore it.
Before this change, all cases of this switch statement either
continue the loop, or throw, or set classFound to true.
The error therefore gets logged just if none of the cases matched.

So we can express the same behavior with a default case.
This makes the reasoning about these index values more local.
Like an early return, this (a) brings the consequence of the error
immediately next to the condition defining it, and (b) lets the normal
happy case continue vertically down without adding indentation.
Same motivation as in the parent commit.
Once the parsing is done, we want these to remain unchanged,
just like the other objects in the parse tree.

So, like ContentNode and its subclasses, make the class immutable.
The parser needs to mutate its own draft of what styles to apply
to a given span; but it can do that with its own local variables
corresponding to the fields, and construct a styles object at the
end of the loop.
This deduplicates the logic for the particular way that a list of
KaTeX nodes get combined into a single widget.
This makes it more uniform with our other content widgets.
@gnprice gnprice merged commit 6852eaa into zulip:main Apr 25, 2025
1 check passed
@gnprice gnprice deleted the pr-katex branch April 25, 2025 00:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintainer review PR ready for review by Zulip maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants