Skip to content

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

Merged
merged 1 commit into from
Sep 9, 2018

Conversation

jklymak
Copy link
Member

@jklymak jklymak commented Jun 24, 2018

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:

import matplotlib.pyplot as plt
import matplotlib.patches as mpatches

fig, ax = plt.subplots(constrained_layout=False)

ax.set_xlim([0, 1.4])
ax.set_ylim([0, 2])
ax.axhline(0.5, color='C2', linewidth=0.3)
sts = ['Line', '2 Lines g\n 2nd Line g', '$\sum_i x $', 'hi $\sum_i x $\ntest', 'test\n $\sum_i x $', '$\sum_i x $\n $\sum_i x $']

for nn, st in enumerate(sts):
    tt = ax.text(0.2 * nn + 0.1, 0.5, st, horizontalalignment='center',
        verticalalignment='bottom')
    bb = tt.get_window_extent(fig.canvas.get_renderer())
    bbt = bb.inverse_transformed(ax.transAxes)
    r = mpatches.Rectangle((0,0), 1, 1, clip_on=False, transform=ax.transAxes)
    r.set_bounds(bbt.bounds)
    ax.add_patch(r)
ax.text(1.2, 0.5, 'Bottom align', color='C2')


if 1:

    ax.axhline(1.3, color='C2', linewidth=0.3)
    for nn, st in enumerate(sts):
        tt = ax.text(0.2 * nn + 0.1, 1.3, st, horizontalalignment='center',
            verticalalignment='top')
        bb = tt.get_window_extent(fig.canvas.get_renderer())
        bbt = bb.inverse_transformed(ax.transAxes)
        r = mpatches.Rectangle((0,0), 1, 1, clip_on=False, transform=ax.transAxes)
        r.set_bounds(bbt.bounds)
        ax.add_patch(r)
    ax.text(1.2, 1.3, 'Top align', color='C2')


    ax.axhline(1.8, color='C2', linewidth=0.3)
    for nn, st in enumerate(sts):
        tt = ax.text(0.2 * nn + 0.1, 1.8, st, horizontalalignment='center',
            verticalalignment='baseline')
        bb = tt.get_window_extent(fig.canvas.get_renderer())
        bbt = bb.inverse_transformed(ax.transAxes)
        r = mpatches.Rectangle((0,0), 1, 1, clip_on=False, transform=ax.transAxes)
        r.set_bounds(bbt.bounds)
        ax.add_patch(r)
    ax.text(1.2, 1.8, 'Baseline align', color='C2')


    ax.axhline(0.1, color='C2', linewidth=0.3)

    for nn, st in enumerate(sts):
        tt = ax.text(0.2 * nn + 0.1, 0.1, st, horizontalalignment='center',
            verticalalignment='bottom', rotation=20)
        bb = tt.get_window_extent(fig.canvas.get_renderer())
        bbt = bb.inverse_transformed(ax.transAxes)
        r = mpatches.Rectangle((0,0), 1, 1, clip_on=False, transform=ax.transAxes)
        r.set_bounds(bbt.bounds)
        ax.add_patch(r)
    ax.text(1.2, 0.1, 'Bot align, rot20', color='C2')


bb = tt.get_window_extent(fig.canvas.get_renderer())
bbt = bb.inverse_transformed(ax.transAxes)
r = mpatches.Rectangle((0,0), 1, 1, clip_on=False, transform=ax.transAxes)
r.set_bounds(bbt.bounds)
ax.add_patch(r)

fig.savefig('Old.png')

plt.show()

Before:

old

With PR:

new

PR Checklist

  • FIX baseline alignment for multiline
  • Add blue boxes to test...
  • Has Pytest style unit tests
  • Code is PEP 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@jklymak
Copy link
Member Author

jklymak commented Aug 16, 2018

Squashed, and added boxes around the text in the mulitline test per @efiring suggestion...

Copy link
Member

@efiring efiring left a 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.

renderer = fig.canvas.get_renderer()

def draw_box(ax, tt):
if 1:
Copy link
Member

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.

ymin = horizLayout[-1][1]
ymax = horizLayout[0][1] + horizLayout[0][3]
# Bounding box definition:
# by defeinition, ymax is 0
Copy link
Member

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.

# by defeinition, ymax is 0
ymax = 0
# ymin is the baseline of the last line subtract the descent of the
# last line.
Copy link
Member

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?

Copy link
Member Author

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
@jklymak jklymak force-pushed the fix-mixed-descent-multiline branch from e7420c6 to e3fd995 Compare August 19, 2018 04:03
@jklymak
Copy link
Member Author

jklymak commented Aug 29, 2018

Pinging for a second review on this...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants