Annotation refactor #2351
Annotation refactor #2351
Conversation
|
One test failed on 3.3 and I think it is unrelated to these changes. Everything else passed. |
|
@mdboom Can you take a look at this? |
|
|
||
| # property so we don't break everything that assumed this existed | ||
| textcoords = property(_get_anncoords, _set_anncoords) | ||
| anncoords = property(_get_anncoords, _set_anncoords) |
There was a problem hiding this comment.
We should probably use the decorator syntax for properties. We only support back to Python 2.6 now, so it's available everywhere.
If xytext is being deprecated, we should add our deprecated decorator to those so the user will get feedback.
There was a problem hiding this comment.
I wanted some feed back on if it was a good idea to change text* -> ann* and then make the sub classes provide the proper getter/setter properties. Not sure I am comfortable unilaterally changing the api with no discussion.
|
This looks fine (other than my style comments). It will need a test, and an entry in |
|
What sort of test do you want here? Would it be enough to just get the extent before and after a move or do you want an image test here? |
|
I think the test can just check the extent before and after the move. I think it's fine to change the API in this case, but we should go one cycle deprecating the old before removing it. The |
|
Got the tests working. Re-based on to master to pick up the locale fix so travis will run on 2.x Do you want any of these commits squashed? |
|
Travis failures are un-related to this (tight bounding box on 2.7 and a strange syntax error on 3.2). |
|
Does this need anything else to get merged? |
|
It looks like it needs a rebase -- that will give Travis one more go at it. |
All of the work done in _get_xy_legacy can be done via transforms returned by _get_xy_transform. Removed the sole usage of this function and the function.
secondary object is to the sub-classes. Added properties to maintain back-compatibility added properties to replace xytext and textcoords (xyann, anncoords) properly deprecated `textcoord` and `xytext` in favor of `anncoords` and `xyann`.
|
re-based I squashed some of the commits. |
Deprecated in PR matplotlib#2351 Merged to master as da6c6b5
Deprecated in PR matplotlib#2351 Merged to master as da6c6b5
Deprecated in PR matplotlib#2351 Merged to master as da6c6b5
…rds" This reverts commit d6e1577, reversing changes made to b280f7d. For 1.4.0 tacaswell refactored annotations (matplotlib#2351 ) but missed that {'axes points', 'axes pixel', 'figure points', 'figure pixel'} were special cased to wrap. Presumably from the function name to maintain back compatibility. This was an unintentional and undocumented API break. This API break was noticed in matplotlib#4012 and fixed in matplotlib#4019 but that catches too many of the coordinate systems (should not be all things that start with 'axes') so fixed one API, but broke others. There were two reasonable courses of action: 1. revert back to 1.4.2 behavior with nothing wrapping. 2. revert back to 1.3.1 behavior with somethings wrapping. In the discussion in matplotlib#4292 where the consensus was to go with option 1, hence this reversion.
This is codifying the API change that went it in matplotlib#2351
Addresses issue #2329
The
xyannandanncoordsproperties (if people think that is a good thing to rename them to) should probably be pushed up to the_AnnotationBaselevel.