text update properties does not handle bbox properly #4942
Conversation
| @@ -240,7 +240,8 @@ def update(self, kwargs): | |||
| """ | |||
| bbox = kwargs.pop('bbox', None) | |||
| super(Text, self).update(kwargs) | |||
| self.set_bbox(bbox) # depends on font properties | |||
| if bbox: | |||
There was a problem hiding this comment.
probably to be safe, this should be if bbox is not None:. I can't be sure if bbox is ever a numpy array or not, and doing a test like that on a numpy array will result in errors in some versions, I think.
There was a problem hiding this comment.
If that is the case, we have a whole bunch of other places in the code have the same issue.
|
@tacaswell, I put in the override update method to ensure the bbox setting is processed after everything else; setting the bbox depends on the font size, so if both are changed, we have to process the font change first. |
|
I think this PR is fine. I originally overlooked the case that it is addressing. When bbox is supplied as a kwarg, it has to be a dictionary, not a numpy array. Using |
|
What is a little confusing is that there used to be a bbox and/or a bbox_patch, and I eliminated the former--but the API still uses set_bbox. This makes it look at first glance like a "bbox" attribute is not getting initialized with this PR. But it's OK, because _bbox_patch is initialized to None in the init method. Maybe it would all be clarified if the |
|
@jrevans Can you add a test for this? It does't need to be an image test, just that calling update does not blow away a previously set bbox. @efiring If I am following you, we use |
FIX: only update bbox_patch if passed in to `Text.update`
|
@jrevans Can you add the test in a new PR? |
|
On 2015/08/22 8:19 AM, Thomas A Caswell wrote:
Exactly. It's really a background patch, potentially a very fancy one. |
Added a check to make sure any existing bbox is not overridden.
The previous behavior would overwrite the bbox property to None even if it was not specified. This now keeps the bbox property as it was unless bbox is specified as a parameter to 'update'.
This addresses an issue in #4897.