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

When deleting a node, the neighboring edge deletes #291

Closed
insssomniac opened this issue Oct 4, 2022 · 11 comments · Fixed by #293
Closed

When deleting a node, the neighboring edge deletes #291

insssomniac opened this issue Oct 4, 2022 · 11 comments · Fixed by #293
Assignees
Labels
bug Something isn't working

Comments

@insssomniac
Copy link

insssomniac commented Oct 4, 2022

Issue tracker is ONLY used for reporting bugs.

New features should be discussed in the discussions.

Title

When deleting a node, the neighboring edge deletes also.

Expected Behavior

When you delete a node -- only edges, connected to it, should be deleted also.

Current Behavior

When deleting a node, the neighboring edge deletes also, but only visually. When you add new node or change something, it's appearing back.

Possible Solution

Steps to Reproduce

  1. Create any type of node. Then create the four nodes below.
  2. Create the edges from the first top node to the every bottom node, in order from left to right. So you should have one node on the top and four edges from it to the every bottom node
  3. Start deleting bottom nodes, from left to right.
  4. You will see what when node deleted, the edge to this node is also deleted (it is normal), but previously created edge from the top node to the neighbour node deletes also, and this is seems a bug.

Context (Environment)

Detailed Description

Possible Implementation

@bcakmakoglu
Copy link
Owner

Can you add a sandbox as well?
Here's a template: https://codesandbox.io/s/frosty-minsky-20b0ge

That would be very helpful thank you :)

@bcakmakoglu bcakmakoglu self-assigned this Oct 4, 2022
@bcakmakoglu bcakmakoglu added the bug Something isn't working label Oct 4, 2022
@bcakmakoglu
Copy link
Owner

Using your instructions doesn't yield the results you describe.
All the edges seem to be deleted properly.

As mentioned, please provide a sandbox reproduction please :)

@insssomniac
Copy link
Author

insssomniac commented Oct 4, 2022

For some reason i have an error during running the sandbox, so i just created the repository and recorded the video.
Repo: https://github.com/insssomniac/vue-flow-example , just clone it, install and run npm run serve
Video: https://drive.google.com/file/d/1qvd8SP2lU--ZHR3uxPgONOPWZ3L3q503/view?usp=sharing

To create a node, just drag&drop it from the top pane
To remove a node, dbl-click to it

@bcakmakoglu
Copy link
Owner

Mh....I am using your repo and can't seem to be able to reproduce the issue as in your video.

Screen.Recording.2022-10-04.at.16.02.10.mov

Are there any specific logs when the issue appears?
What browser are you using?

@insssomniac
Copy link
Author

Very strange. I'm on Chrome Version 106.0.5249.61 (Official Build) (64-bit) also tried on Firefox, and had the same issue. Also my colleague faced to the same issue
I recorded again with console opened, maybe it will help
vue-flow-error-2nd.webm

@bcakmakoglu
Copy link
Owner

I can't view the video you linked, can you link it in a different way? 😄

@bcakmakoglu
Copy link
Owner

Also can you make sure that your browser isn't somehow overwriting the css rule for .vue-flow__edges?

It needs to have the overflow rule

.vue-flow__edges {
  overflow: visible;
}

@insssomniac
Copy link
Author

If you just open the video in the new tab, it doesn't show? :))
prev. video, from post above (2): https://drive.google.com/file/d/1fcfoT21kOW3LFSsh6nvCAYat2Hrxqfb3/view?usp=sharing

3-rd video, i showed here elements list, and it seems edge really disapearing https://drive.google.com/file/d/1bl7xeJh-rqbzBMbrKaepwP3Y4j5sT1VR/view?usp=sharing

@bcakmakoglu
Copy link
Owner

@insssomniac

Replace the following code:

const {
  onNodeDoubleClick,
  onNodeClick,
  onPaneClick,
  onEdgeClick,
  toObject,
  onConnect,
  addNodes,
  addEdges,
  getSelectedNodes,
  applyEdgeChanges,
  applyNodeChanges,
  project
} = useVueFlow()

// ...

onNodeDoubleClick((e) => {
  if (confirm('Delete node?')) {
    applyNodeChanges([{type: 'remove', id: e.node.id}])
  }
})

onEdgeClick((e) => {
  if (confirm('Delete edge?')) {
    applyEdgeChanges([{ type: 'remove', id: e.edge.id }])
  }
})

with

const {
  onNodeDoubleClick,
  onNodeClick,
  onPaneClick,
  onEdgeClick,
  toObject,
  onConnect,
  addNodes,
  addEdges,
  getSelectedNodes,
  removeEdges,
  removeNodes,
  project
} = useVueFlow()

onNodeDoubleClick((e) => {
  if (confirm('Delete node?')) {
		removeNodes([e.node])
  }
})

onEdgeClick((e) => {
  if (confirm('Delete edge?')) {
    removeEdges([e.edge])
  }
})

In most cases you should not be using applyNodeChanges or applyEdgeChanges yourself and if you do then only to intercept the change handlers (for some possible validation for example).
Instead use the appropriate actions for that like addNodes or removeNodes etc. which will emit these changes through the proper channels and you should not have any issues as seen in the clips.

What happens here is that you remove the node by applying the change yourself, thus Vue Flow never checks if any edges should be removed as well.
After the node gets removed, the edge is seen as "orphaned" and there will be an attempt to remove it.
That apparently doesn't work properly in this case, which I will fix, and instead it removes an edge at another index position.

Anyway, these issues don't appear when you use the actions instead :)

@insssomniac
Copy link
Author

@bcakmakoglu removeNodes([e.node]) instead applyNodeChanges solved the issue, thanks!
I don't remeber why i decided to use applyNodeChanges, but i'll replace as you said for nodes and for edges.

insssomniac added a commit to insssomniac/vue-flow-example that referenced this issue Oct 5, 2022
@bcakmakoglu
Copy link
Owner

@insssomniac
With 0.4.41 it should work with applyEdgeChanes as well - you should still use removeEdges instead though 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants