For a text artist, if it has a _bbox_patch associated with it, the contains test should reflect this. #1088

Merged
merged 3 commits into from Aug 20, 2012

Projects

None yet

4 participants

@dhyams
Contributor
dhyams commented Aug 15, 2012

For a text artist, if it has a _bbox_patch associated with it, the contains test should reflect this.

Daniel Hyams For a text artist, if it has a _bbox_patch associated with it, the co…
…ntains test should reflect this.
be50789
@dhyams
Contributor
dhyams commented Aug 15, 2012

This pull request is identical to #1072 but based on master.

@mdboom
Member
mdboom commented Aug 15, 2012

Seems fine to me.

@pelson pelson and 2 others commented on an outdated diff Aug 15, 2012
lib/matplotlib/text.py
@@ -206,12 +206,18 @@ def contains(self,mouseevent):
if not self.get_visible() or self._renderer is None:
return False,{}
+ if self._bbox_patch:
+ patch_inside, patch_cattr = self._bbox_patch.contains(mouseevent)
@pelson
pelson Aug 15, 2012 Member

One idiom which I think helps clarity in the case where you are forced to handle data that you don't ever use is to call your variable _. In this case:

patch_inside, _ =  self._bbox_patch.contains(mouseevent)
@dhyams
dhyams Aug 15, 2012 Contributor

Nice tip! I like it....but, in this case, I would rather see the unused variable stay as is, as a reminder to future readers that "maybe" it should be used. The "_" implies that it "definitely" shouldn't be used.

@WeatherGod
WeatherGod Aug 15, 2012 Member

Of course, a comment would also work fine to explain that...

@pelson
pelson Aug 15, 2012 Member

But my stupid IDE can't read comments :-)

@WeatherGod WeatherGod and 1 other commented on an outdated diff Aug 15, 2012
lib/matplotlib/text.py
l,b,w,h = self.get_window_extent().bounds
r, t = l+w, b+h
x, y = mouseevent.x, mouseevent.y
inside = (l <= x <= r and b <= y <= t)
- return inside, {}
+ return (inside or patch_inside), {}
@WeatherGod
WeatherGod Aug 15, 2012 Member

Same question as before, shouldn't this return statement use patch_cattr instead of the empty dictionary? Otherwise, what is the point of saving patch_cattr?

@dhyams
dhyams Aug 15, 2012 Contributor

I'll yield to whatever you guys want me to do there...as I had mentioned before, I am not sure exactly what role the second return value from contains() plays. I looked at all of the patches, and it looks to me like they all return an empty dict for that second arg anyway.

If you want me to return dict({}.items() + patch_cattr.items()) I can add that. I wasn't sure what the worse thing was to do...just return an empty dict straightaway (possibly has a problem in the future if patches start to return useful data there), or do the dict update (possibly has a problem in the future if the same key shows up in the two dictionaries).

@WeatherGod
WeatherGod Aug 15, 2012 Member

I prefer to have all my piping set correctly now rather than calling a plumber to find and fix the problem later. Unless there is a specific reason why we don't ever want the patch_cattr information (and that may very well be the case), I would pass it along so that it is available to the end user if they want it.

@dhyams
dhyams Aug 15, 2012 Contributor

OK, will do. Just a nitpick though...this is not a "pass it along" situation, it's a "merge with existing results" situation...which could open the door to different, and harder to find, problems down the road.

@WeatherGod
WeatherGod Aug 15, 2012 Member

Merge with what other existing results? We have an empty dictionary and patch_cattr. If patch_inside is false, then patch_cattr is also empty. It is only if patch_inside is True that any possibility of a non-empty dictionary comes back, in which case, I may want that information. A particular usecase I can think of is selecting the contour labels interactively.

@dhyams
dhyams Aug 15, 2012 Contributor

That's the way it is right now, but I thought we were talking about future proofing? In the future, someone might add attrs to return for the text part of the text object, and at that point, if you don't merge the two dicts, it gets lost.

The text object is made up (possibly) of two parts...the text and the bbox_patch. The contains test should return the union of those two results. Union of booleans, then union of the attrs.

Daniel Hyams Refinement to the contains test; returns the _bbox_patch's containment
attributes if they are present.  It does this by merging those results with
the (presently empty) attributes for the text object.

Code also restructured slightly so that it will be more efficient.
41a3d80
@dhyams
dhyams commented on 41a3d80 Aug 15, 2012

Is this one better?

@WeatherGod
Member

@dhyams, you raise a good point that I didn't quite grok at first. We have a situation where we are treating the combination of two artist objects as one, and both of them has the potential of having user-supplied pick functions that can return their own dictionaries. These dictionaries may have common keys, in which case, naively merging them would produce an intelligible mess.

Just as a reference, I am referring to this feature of mpl: http://matplotlib.sourceforge.net/users/event_handling.html#object-picking

The problem here is that pretty much all of mpl never defines this dictionary, and the feature is designed totally for the end-user to take advantage of. However, I don't think the interface was completely thought through. I think the solution in this case is that it is the responsibility of the combined text/patch object to return a single dictionary. In other words, this is the proper place to address this issue.

Lacking any sort of precedence that I can see, I would suggest that the patch's dictionary is added as an entry to the text's dictionary, probably keyed as "patch". Assuming that the text object itself never has an entry called "patch", this should prevent any key collisions, and still provide useful information downstream to the caller. Thoughts?

@dhyams
Contributor
dhyams commented Aug 16, 2012

This sounds fine to me....maybe call the new key "_bbox_patch" or "bbox_patch"? Just trying to decrease the possibility of key collision.

@WeatherGod
Member

Yes, I like that better.

Daniel Hyams Instead of trying to merge the containment attribute dicts from both …
…the text

and the bounding box, the bounding box's attributes are now a key within the
returned attribute dict.  The key is "bbox_patch".
834f4cb
@dhyams
Contributor
dhyams commented Aug 17, 2012

This one OK to merge now?

@WeatherGod
Member

Yes, I guess so.

@WeatherGod WeatherGod merged commit 963506f into matplotlib:master Aug 20, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment