8000 Add parse_math in Text and default it False for TextBox by aitikgupta · Pull Request #20367 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]
Skip to content

Add parse_math in Text and default it False for TextBox #20367

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 10 commits into from
Jun 29, 2021
Merged

Add parse_math in Text and default it False for TextBox #20367

merged 10 commits into from
Jun 29, 2021

Conversation

aitikgupta
Copy link
Contributor

PR Summary

Following up the comment: #20266 (comment), this PR adds a override_math to Text objects, and make TextBox's text object default it to True.

This resolves #20266.
It does not allow GUI to go in invalid state when there's invalid mathtext in it, because it isn't parsed at all.
(also allows folks to override mathtext at all in their text objects)

Interactive trial:

import matplotlib.pyplot as plt
from matplotlib.widgets import TextBox

fig, ax = plt.subplots()
fig.subplots_adjust(left=0.25, top=0.55, bottom=0.45)
text_box = TextBox(ax, "Something in '\$ \$'")                       # try typing wrong mathtext here

plt.show()

Static trial:

import matplotlib.pyplot as plt
fig, ax = plt.subplots()
ax.text(1, 2, "$$")
plt.show()

Needs a changenote (API change) and tests, but let's wait for a review before that.

PR Checklist

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (run flake8 on changed files to check).
  • New features are documented, with examples if plot related.
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • Conforms to Matplotlib style conventions (install flake8-docstrings and run flake8 --docstring-convention=all).
  • New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).

QuLogic
QuLogic reviewed Jun 4, 2021
View reviewed changes
tacaswell
tacaswell previously requested changes Jun 5, 2021
View reviewed changes
Copy link
Member
@tacaswell tacaswell left a comment

Choose a reason for hiding this comment

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

👍 to theory of the implementation

Needs a better name and to be added at the end of the parameter list.

Should also get a test, maybe write $$ to SVG and make sure the text is actually in the svg?

@tacaswell
Copy link
Member

Anyone can dismiss my review.

@aitikgupta
Copy link
Contributor Author

Should also get a test, maybe write $$ to SVG and make sure the text is actually in the svg?

How about just catching an error? Probably simpler and will save us space:

import matplotlib.pyplot as plt
fig, ax = plt.subplots()
ax.text(0, 0, r"$ \wrong{5} $", override_math=True)
fig.canvas.draw()                                               # won't throw error

ax.text(0, 0, r"$ \wrong{5} $", override_math=False)
fig.canvas.draw()                                               # will throw error

^with the right parameter name, ofcourse.

@aitikgupta
Copy link
Contributor Author

Do we not test widgets like TextBox?
I don't see a single test for it in tests/test_widgets.py :/

@QuLogic
Copy link
Member
QuLogic commented Jun 5, 2021

If we don't, then you can be the first to add them. 😛

@aitikgupta aitikgupta mentioned this pull request Jun 5, 2021
Closed
@anntzer
Copy link
Contributor
anntzer commented Jun 5, 2021

I would suggest e.g. set_usemath (for consistency with set_usetex).

@QuLogic
Copy link
Member
QuLogic commented Jun 7, 2021 •
edited
Loading

But set_usetex is named that way because it (over)sets the usetex rcParam; it's not a great name otherwise and there isn't a similar rcParam.

@anntzer
Copy link
Contributor
anntzer commented Jun 7, 2021

Fair enough, I don't have a very strong opinion there...

@timhoffm
Copy link
Member
timhoffm commented Jun 7, 2021

parse_math is a good parameter name if we stick too a boolean. But note the suggestion of parse_mode in #20266 (comment).

@aitikgupta aitikgupta mentioned this pull request Jun 8, 2021
Closed
@aitikgupta aitikgupta changed the title Add override_math in Text and default it True for TextBox Add parse_math in Text and default it False for TextBox Jun 8, 2021
@jklymak jklymak requested a review from tacaswell June 17, 2021 14:38
jklymak
jklymak approved these changes Jun 17, 2021
View reviewed changes
Copy link
Member
@jklymak jklymak left a comment

Choose a reason for hiding this comment

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

This looks good to me. Probably needs a what's-new entry though since this is new API

QuLogic
QuLogic reviewed Jun 22, 2021
View reviewed changes
@QuLogic QuLogic dismissed tacaswell’s stale review June 29, 2021 08:48

Test added, and parameter was renamed.

@QuLogic QuLogic merged commit 833658b into matplotlib:master Jun 29, 2021
@QuLogic QuLogic added this to the v3.5.0 milestone Jun 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@QuLogic QuLogic QuLogic approved these changes

@jklymak jklymak jklymak approved these changes

@tacaswell tacaswell Awaiting requested review from tacaswell

Assignees
No one assigned
Labels
topic: text topic: widgets/UI
Projects
None yet
Milestone
v3.5.0
Development

Successfully merging this pull request may close these issues.

"$$" cannot be displayed by ax.text()
6 participants
@aitikgupta @tacaswell @QuLogic @anntzer @timhoffm @jklymak
0