Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix selection issues with pins overlapping unrelated links. Fix not being able to start new links when there is an existing link on a pin #126

Merged
merged 4 commits into from
Sep 28, 2021

Conversation

Auburn
Copy link
Collaborator

@Auburn Auburn commented Sep 5, 2021

Includes changes from #124

In v0.4 of ImNodes it is possible to start a link on an pin that already has a link. As part of the input handling changes this is no longer possible, attempting to start a new link simply selects the existing link.

If there is a hovered pin a link should only be considered hovered if that link connects to the hovered pin. This avoids issues where if there is a link behind an unrelated hovered pin it would be considered hovered and cause odd interactions.

@Auburn Auburn changed the title Fix selection issues with pins overlapping unrelated links. Fix not being able to start new links when there is an existing link on an pin Fix selection issues with pins overlapping unrelated links. Fix not being able to start new links when there is an existing link on a pin Sep 5, 2021
Copy link
Owner

@Nelarius Nelarius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix! Finally had some time to go over this. This is a useful fix, so I would happy to merge it as is. However, I'm curious to hear what you think.

BeginLinkInteraction is getting complex. I'm wondering if it might make more sense to handle the "pin is hovered" interactions before handling "link is hovered" in EndNodeEditor. I.e., first handle the (pin hovered | pin and link hovered) case, since we should never end up considering link selection when a pin is hovered.

Would this be something you'd have time to experiment with in this pr, or should we leave it for another time?

@Auburn
Copy link
Collaborator Author

Auburn commented Sep 28, 2021

I'm happy to leave it as is for now considering you are working on a large refactor. Can always revisit it in the future when it breaks 😛

@Nelarius
Copy link
Owner

Can always revisit it in the future when it breaks 😛

That sounds like a plan 👍 😄

@Nelarius Nelarius merged commit 6a8c130 into Nelarius:master Sep 28, 2021
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants