-
-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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... |
efiring
left a comment
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