-
-
Notifications
You must be signed in to change notification settings - Fork 7.6k
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
Reverted a previous change to artist transform setting. Fixes legend bug. #1176
Conversation
That's really strange. I would have considered the "t is not None" test as being superior to blindly setting the boolean to True. Maybe some code somewhere is fiddling around with the _transform attribute directly when it shouldn't? |
Me too! The |
Thanks for finding this and tracking this down. I think what's not clear is that a transform of None is set, that's equivalent to an identity transform, and should be treated as an explicit transform having been set as far as the offsetbox is concerned. I think this change is correct, though I haven't run it against the test suite to see if it produces additional failures. |
You were right to have your suspicions. I do get failures as a result of this. Back to the drawing board... might not find a solution to this until tomorrow. |
Ok. I have solved this issue the proper way. I did a lot of reading of the legend code (phew, its not particularly easy to follow), found the problem (the transforms weren't being set correctly) and fixed a couple of typos. Unfortunately, my editor had a setting enabled to fix the whitespace problems automatically, which adds a little bit of noise to this PR. It wouldn't be unreasonable to ask me to revert them, but I am keeping them for now as they do improve the overall readability of the code (and hence my sanity while I was debugging this). |
@mdboom any advice on how best to go about this? |
Maybe the best/easiest thing to do is to just take the text out -- the legend should layout good enough for the purposes of the test without it. |
On master, the following code fails to put the markers in a legend in the correct place:
It turns out this is a result of a previous pull request of mine (#1090). This PR fixes this regression (and adds a suitable test). I will milestone this for 1.2 as it is a pretty critical bug fix.