For a text artist, if it has a _bbox_patch associated with it, the conta... #1072

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
@dhyams
Contributor

dhyams commented Aug 12, 2012

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

@@ -216,7 +221,7 @@ def contains(self,mouseevent):
x, y = mouseevent.x, mouseevent.y
inside = nxutils.pnpoly(x, y, xyverts)
- return inside,{}
+ return inside or patch_inside,{}

This comment has been minimized.

Show comment Hide comment
@WeatherGod

WeatherGod Aug 12, 2012

Member

Instead of the empty dictionary, shouldn't it return patch_cattr?

@WeatherGod

WeatherGod Aug 12, 2012

Member

Instead of the empty dictionary, shouldn't it return patch_cattr?

This comment has been minimized.

Show comment Hide comment
@dhyams

dhyams Aug 12, 2012

Contributor

Don't know...I was trying to not modify the existing behavior as much as possible. I've never been very clear on what that dictionary is used for, and wasn't sure if it was OK to "merge" the dictionaries from two different artist's contains method. (I know that the text in this case has an empty dict, but what if it didn't ;) )

@dhyams

dhyams Aug 12, 2012

Contributor

Don't know...I was trying to not modify the existing behavior as much as possible. I've never been very clear on what that dictionary is used for, and wasn't sure if it was OK to "merge" the dictionaries from two different artist's contains method. (I know that the text in this case has an empty dict, but what if it didn't ;) )

@@ -216,7 +221,7 @@ def contains(self,mouseevent):
x, y = mouseevent.x, mouseevent.y
inside = nxutils.pnpoly(x, y, xyverts)

This comment has been minimized.

Show comment Hide comment
@WeatherGod

WeatherGod Aug 12, 2012

Member

Note, this may be why you need to rebase, as we are deprecating the usage of nxutils.

@WeatherGod

WeatherGod Aug 12, 2012

Member

Note, this may be why you need to rebase, as we are deprecating the usage of nxutils.

This comment has been minimized.

Show comment Hide comment
@dhyams

dhyams Aug 12, 2012

Contributor

OK, can you (or someone) give me a hint as to how to rebase properly? The pull request was branched off of the tag "v1.1.1".

@dhyams

dhyams Aug 12, 2012

Contributor

OK, can you (or someone) give me a hint as to how to rebase properly? The pull request was branched off of the tag "v1.1.1".

@dhyams dhyams closed this Aug 12, 2012

@dhyams dhyams reopened this Aug 12, 2012

@pelson

This comment has been minimized.

Show comment Hide comment
@pelson

pelson Aug 12, 2012

Member

The reason this cannot be merged automatically is that I changed the contains code a couple of months ago on master to fix a bug. I'm not entirely sure what the _bbox_patch is, but it sounds like a sensible thing to check the contains of that, if it exists, before checking if the text is contained (which has limitations if the text is rotated).

Perhaps the logic should go:

if not visible: return False
elif has bbox: return bbox.contains(event)
else return contains(self.get_window_extent())

Rather than

if not visible: return False
elif has bbox: return bbox.contains(event) or contains(self.get_window_extent())
else return contains(self.get_window_extent())
Member

pelson commented Aug 12, 2012

The reason this cannot be merged automatically is that I changed the contains code a couple of months ago on master to fix a bug. I'm not entirely sure what the _bbox_patch is, but it sounds like a sensible thing to check the contains of that, if it exists, before checking if the text is contained (which has limitations if the text is rotated).

Perhaps the logic should go:

if not visible: return False
elif has bbox: return bbox.contains(event)
else return contains(self.get_window_extent())

Rather than

if not visible: return False
elif has bbox: return bbox.contains(event) or contains(self.get_window_extent())
else return contains(self.get_window_extent())

@dhyams dhyams closed this Aug 12, 2012

@dhyams

This comment has been minimized.

Show comment Hide comment
@dhyams

dhyams Aug 12, 2012

Contributor

I guess the proper thing is to close the pull request, and resubmit against master.

Contributor

dhyams commented Aug 12, 2012

I guess the proper thing is to close the pull request, and resubmit against master.

@efiring

This comment has been minimized.

Show comment Hide comment
@efiring

efiring Aug 12, 2012

Member

Right, but what you will resubmit will be your topic branch rebased against master (updated from upstream). This can be done using "git rebase --onto master v1.1.1 text_contains", but you will have to resolve a conflict. It may be as easy, or easier, to simply make a new topic branch, relative to current master, in which you re-do your changes, since they involve only a few lines.

Member

efiring commented Aug 12, 2012

Right, but what you will resubmit will be your topic branch rebased against master (updated from upstream). This can be done using "git rebase --onto master v1.1.1 text_contains", but you will have to resolve a conflict. It may be as easy, or easier, to simply make a new topic branch, relative to current master, in which you re-do your changes, since they involve only a few lines.

@dhyams

This comment has been minimized.

Show comment Hide comment
@dhyams

dhyams Aug 13, 2012

Contributor

Egads, I really hate git sometimes. I tried and tried to do this the "right way" with the rebase, but I think I only succeeded in completely screwing up several branches. I just thought that this would be a teachable moment for me to figure out how to use rebase correctly. Failed.

I'll just redo the patch against master later.

Contributor

dhyams commented Aug 13, 2012

Egads, I really hate git sometimes. I tried and tried to do this the "right way" with the rebase, but I think I only succeeded in completely screwing up several branches. I just thought that this would be a teachable moment for me to figure out how to use rebase correctly. Failed.

I'll just redo the patch against master later.

@pelson

This comment has been minimized.

Show comment Hide comment
@pelson

pelson Aug 15, 2012

Member

Seems like this PR is now rebased at #1088

Member

pelson commented Aug 15, 2012

Seems like this PR is now rebased at #1088

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment