Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
text update properties does not handle bbox properly#4942
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
probably to be safe, this should beif 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 processedafter 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 theinit 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.