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: AnnotationBbox extents before draw #26184

Merged
merged 1 commit into from Jun 30, 2023

Conversation

rcomer
Copy link
Member

@rcomer rcomer commented Jun 25, 2023

PR summary

Fixes #24453

Calling update_positions before working out the bbox within get_tightbbox means constrained layout will get sensible numbers to work with so the example from the issue works as expected. We can now also have constrained layout make space for the annotation if it is not contained within the axes:

import matplotlib.pyplot as plt
from matplotlib.offsetbox import AnnotationBbox, TextArea

fig, ax = plt.subplots(layout="constrained")

ab = AnnotationBbox(
    TextArea("Some text", textprops={"size": 42}),
    (1, 0.5),
    xycoords="axes fraction",
    box_alignment=(0.5, 0.5),
    pad=0
)

ax.add_artist(ab)
fig.savefig("annotation_box.png", dpi=100)

annotation_box

I'm not sure where get_window_extent is used, but I updated that in the same way for consistency (and to get the test passing after I removed the draw from it).

Also split the test into two so that the two methods can be checked independently of each other.

PR checklist

@rcomer rcomer added PR: bugfix Pull requests that fix identified bugs topic: annotation labels Jun 25, 2023
oscargus
oscargus previously approved these changes Jun 25, 2023
Copy link
Contributor

@oscargus oscargus left a comment

Choose a reason for hiding this comment

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

Minor suggestion, but otherwise looks good!

lib/matplotlib/tests/test_offsetbox.py Outdated Show resolved Hide resolved
@rcomer rcomer added this to the v3.7.2 milestone Jun 25, 2023
@rcomer
Copy link
Member Author

rcomer commented Jun 25, 2023

I don't know what's going on with the Azure Linux tasks. If I click through to Azure it says they all succeeded. But they are currently showing for me as in progress below.

tacaswell
tacaswell previously approved these changes Jun 29, 2023
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.

This seems correct to me, but I am a bit concerned about how to drive these objects to a state where they do not know what Figure they are part of.

@tacaswell tacaswell dismissed their stale review June 29, 2023 18:50

I'm confused

@tacaswell

This comment was marked as outdated.

@tacaswell
Copy link
Member

Sorry, hid my comment was working on the wrong branch 🤦🏻

@rcomer rcomer marked this pull request as draft June 29, 2023 20:26
@rcomer
Copy link
Member Author

rcomer commented Jun 30, 2023

@tacaswell has pointed out that if you take the code from the OP and use plt.show(), then resize the window, the result jumps around. I welcome any guidance/suggestions about where to look to understand that.

@QuLogic
Copy link
Member

QuLogic commented Jun 30, 2023

I can't reproduce that, and also @tacaswell mentioned that he was on the wrong branch?

@rcomer
Copy link
Member Author

rcomer commented Jun 30, 2023

@tacaswell mentioned at the start of yesterday's call that he then tried with the right branch and it was still a problem (and I also reproduced it after the call). I have also now reproduced with multiple calls to savefig and set_size_inches. Everything seems fine if you don't resize.

import matplotlib.pyplot as plt
from matplotlib.offsetbox import AnnotationBbox, TextArea

fig, ax = plt.subplots(layout="constrained")
fig.set_facecolor('lightgray')

ab = AnnotationBbox(
    TextArea("Some text", textprops={"size": 42}),
    (1, 0.5),
    xycoords="axes fraction",
    box_alignment=(0.5, 0.5),
    pad=0
)

ax.add_artist(ab)

plt.savefig("test1.png")
fig.set_size_inches(8, 3)
plt.savefig("test2.png")
plt.savefig("test3.png")

test1
test2
test3

I'm using QtAgg.

Edit: if I run this with main, the second and third images are the same as with my branch...

@oscargus oscargus dismissed their stale review June 30, 2023 08:48

As there are issues I'm dismissing the review for now.

@QuLogic
Copy link
Member

QuLogic commented Jun 30, 2023

Oh, I see it's because it's at (1, 0.5), not (0.5, 0.5) as in the original issue. I thought that's where you meant by OP and not from here.

@rcomer
Copy link
Member Author

rcomer commented Jun 30, 2023

Ok I think I have it. I probably need to go educate myself better about what stale actually means....

@tomicapretto I think this also now fixes the case from tomicapretto/flexitext#11 (comment)

@rcomer rcomer marked this pull request as ready for review June 30, 2023 10:28
@rcomer
Copy link
Member Author

rcomer commented Jun 30, 2023

Should I add a test for the draw-then-resize case? I'm not sure how realistic it is that a future contributor would break it in that specific way.

@tacaswell
Copy link
Member

I think this is fine without an additional test.

@QuLogic QuLogic merged commit b71260a into matplotlib:main Jun 30, 2023
39 checks passed
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Jun 30, 2023
rcomer added a commit that referenced this pull request Jul 1, 2023
…184-on-v3.7.x

Backport PR #26184 on branch v3.7.x (FIX: AnnotationBbox extents before draw)
@rcomer rcomer deleted the annbbox-get_tightbbox branch July 1, 2023 06:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: bugfix Pull requests that fix identified bugs topic: annotation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: AnnotationBbox does not return correct window_extent before first draw
5 participants