Skip to content
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

Fix throw exception by \, #214

Merged
merged 8 commits into from
Jan 11, 2020
Merged

Fix throw exception by \, #214

merged 8 commits into from
Jan 11, 2020

Conversation

rstm-sf
Copy link
Contributor

@rstm-sf rstm-sf commented Oct 18, 2019

Fix throw exception by \, (#139 )
Add test for \,

@rstm-sf
Copy link
Contributor Author

rstm-sf commented Oct 19, 2019

Or, there seems to be another solution -- make non-static

@ForNeVeR
Copy link
Owner

Honestly I fail to understand how is this supposed to fix any errors. Could you please explain? What was the initial issue?

@rstm-sf
Copy link
Contributor Author

rstm-sf commented Oct 19, 2019

Initial issue was that a value is added on an existing key
https://github.com/ForNeVeR/wpf-math/blob/351e010a3e76a82001dc337e822d82ef8012321f/src/WpfMath/TexPredefinedFormulaParser.cs#L177

But these are just the consequences of another issue -- argValueParsers and actionParsers is static
https://github.com/ForNeVeR/wpf-math/blob/351e010a3e76a82001dc337e822d82ef8012321f/src/WpfMath/TexPredefinedFormulaParser.cs#L18-L19

In case of parsing \,, we get into the next loop
https://github.com/ForNeVeR/wpf-math/blob/351e010a3e76a82001dc337e822d82ef8012321f/src/WpfMath/TexPredefinedFormulaParser.cs#L111-L122

where calling parse \thinspace
https://github.com/ForNeVeR/wpf-math/blob/351e010a3e76a82001dc337e822d82ef8012321f/src/WpfMath/Data/PredefinedTexFormulas.xml#L48-L50

in CreateTeXFormulaParser.Parse(SourceSpan, XElement)
https://github.com/ForNeVeR/wpf-math/blob/351e010a3e76a82001dc337e822d82ef8012321f/src/WpfMath/TexPredefinedFormulaParser.cs#L169-L170

and CreateTeXFormulaParser.TempFormulas (for \,) assigned the reference to the another dictionary. And, after returning the result of parsing \thinspace, the reference to another dictionary is returned back, and the worker is over
https://github.com/ForNeVeR/wpf-math/blob/351e010a3e76a82001dc337e822d82ef8012321f/src/WpfMath/TexPredefinedFormulaParser.cs#L118

@rstm-sf
Copy link
Contributor Author

rstm-sf commented Oct 29, 2019

@ForNeVeR, hello!

Do you think there is a better way to solve the problem, or did I explain it poorly?

@ForNeVeR
Copy link
Owner

@rstm-sf I am sorry for delay, I was rather occupied (happens to me triple a year these days), so I haven't properly reviewed these changes yet. Consider the ball on my side now.

@rstm-sf
Copy link
Contributor Author

rstm-sf commented Oct 30, 2019

@ForNeVeR ok, np ;)

@rstm-sf rstm-sf mentioned this pull request Nov 5, 2019
Copy link
Contributor Author

@rstm-sf rstm-sf left a comment

Choose a reason for hiding this comment

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

Hmm, it was able to pending, but I still want it to be \,

@ForNeVeR ForNeVeR self-assigned this Dec 29, 2019
Copy link
Owner

@ForNeVeR ForNeVeR left a comment

Choose a reason for hiding this comment

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

I still very much dislike the whole PredefinedFormulas business. This PR doesn't solve the root cause of the issue (it is a bit deeper), but I think it makes the resulting behavior more correct.

I tend to approve that and leave everything else for #180.

src/WpfMath/TexPredefinedFormulaParser.cs Outdated Show resolved Hide resolved
@ForNeVeR
Copy link
Owner

Thanks for your contribution!

@ForNeVeR ForNeVeR merged commit 3670761 into ForNeVeR:master Jan 11, 2020
@rstm-sf rstm-sf deleted the bugfix/thinspace_exception_ branch January 11, 2020 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

\thinspace exception
2 participants