Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add toggle to automatically connect new vertex to edge underneath it #337

Merged
merged 13 commits into from
Aug 7, 2024

Conversation

jvdwetering
Copy link
Collaborator

@jvdwetering jvdwetering commented Jul 22, 2024

Adds a button next to the Selection/Vertex/Edge buttons which when toggled makes the 'add vertex' automatically connect on an edge underneath it.

To make clear it snapped I added an animation to the edge, for which I added a new EItemAnimation class.

@jvdwetering jvdwetering marked this pull request as ready for review July 22, 2024 15:34
@jvdwetering
Copy link
Collaborator Author

jvdwetering commented Jul 22, 2024

This is related to #311, but then kind of the converse. It is essentially the same feature as the 'magic slice' identity addition, but then using the tools native to the edit window.

@RazinShaikh
Copy link
Collaborator

This is very hit or miss. Can you do something about that?

snap-to-edge.mp4

@jvdwetering
Copy link
Collaborator Author

I've mostly been dealing with this by zooming in more, so that the edge is wider. An alternative solution I was thinking of, was to make an edge you are hovering over more thick so that it is easier to click on it.
There is also the manual solution, where we don't use the built-in Qt collision features, but instead check ourselves across all edges whether there is an intersection (or something close to it), this might get slow for large diagrams though.
I should also add that the current implementation suffers from the same issue described in #302.

zxlive/rewrite_action.py Outdated Show resolved Hide resolved
zxlive/graphscene.py Outdated Show resolved Hide resolved
zxlive/eitem.py Outdated Show resolved Hide resolved
zxlive/eitem.py Show resolved Hide resolved
@jvdwetering
Copy link
Collaborator Author

This now also implements #311.
I think this is ready for merging

Copy link
Collaborator

@RazinShaikh RazinShaikh left a comment

Choose a reason for hiding this comment

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

looks good to me

zxlive/editor_base_panel.py Outdated Show resolved Hide resolved
zxlive/graphscene.py Outdated Show resolved Hide resolved
@jvdwetering
Copy link
Collaborator Author

This is giving mypy a hard time:

def it(self) -> EItem:
        if self._it is None and self.scene is not None and self.e is not None:
            self._it = self.scene.edge_map[self.e]
        assert self._it is not None
        return self._it

we have _it: Optional[EItem].
The error is Incompatible types in assignment (expression has type "Dict[int, EItem]", variable has type "Optional[EItem]").
I don't understand why it is an error to assign a EItem to something of type Optional[EItem].

@jvdwetering jvdwetering merged commit 47f9271 into master Aug 7, 2024
2 checks passed
@RazinShaikh
Copy link
Collaborator

Hi @jvdwetering , thanks for merging this. For the next time, do you mind merging the PRs without squashing the commits. I think for a big PR like this, it is quite useful to have the full commit history for future reference/debugging instead of a single commit containing all the changes.

@jvdwetering
Copy link
Collaborator Author

Okay that makes sense. I wasn't sure what a good policy would be.

@jvdwetering jvdwetering deleted the feature/vertex-snap-to-edge branch August 7, 2024 19:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants