-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
FIX: layout for mixed descent multiline text objects #11499
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: layout for mixed descent multiline text objects #11499
Conversation
8df6787
to
6b75057
Compare
2f32a88
to
2b30036
Compare
2b30036
to
e7420c6
Compare
Squashed, and added boxes around the text in the mulitline test per @efiring suggestion... |
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.
This is making important fixes so it should get in ASAP. I have only some trivial cleanup suggestions, mainly pointing to a few comments that seem both verbose and unclear.
lib/matplotlib/tests/test_text.py
Outdated
renderer = fig.canvas.get_renderer() | ||
|
||
def draw_box(ax, tt): | ||
if 1: |
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.
Looks like this conditional is left over from development.
lib/matplotlib/text.py
Outdated
ymin = horizLayout[-1][1] | ||
ymax = horizLayout[0][1] + horizLayout[0][3] | ||
# Bounding box definition: | ||
# by defeinition, ymax is 0 |
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.
Typo in comment. Also, a single line of comment should be enough here.
lib/matplotlib/text.py
Outdated
# by defeinition, ymax is 0 | ||
ymax = 0 | ||
# ymin is the baseline of the last line subtract the descent of the | ||
# last line. |
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 don't understand the comment; can you condense to one line, and clarify?
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.
Reworded, though I admit that I don't completely remember what this meant. This code is very convoluted, and could maybe do with a refactor now that we know what its supposed to do. ...
TST: add another multiline test DOC: fix comments
e7420c6
to
e3fd995
Compare
Pinging for a second review on this... |
PR Summary
Closes #11498 partially closes #11468
WIP: this'll fail some tests that depended on the previous bad behaviour...EDIT This indeed failed some tests, but I'd pretty strongly argue the tests were broken and this PR makes them correct...
Test:
Before:
With PR:
PR Checklist