search

setzer22 / egui_node_graph

Build your node graph applications in Rust, using egui
MIT License
711 stars 132 forks source link

return disconnect responses before removal response #61

Closed bpostlethwaite closed 1 year ago

bpostlethwaite commented 1 year ago

Currently egui_node_graph returns the node removal response before disconnection responses during node deletion. User code is likely to want to clean up connections before running any logic that handles node removal directly. This PR re-orders the responses so that disconnect responses are emitted before the removal response. This PR should be considered as introducing a breaking change, and one that isn't protected by types. As such it could lead to bugs in user code that depended on the old order. Still I think this is a more logical ordering so I am submitting this PR for consideration. Thank you.

setzer22 commented 1 year ago

This makes sense. Thanks for the PR! :+1:

setzer22 commented 1 year ago

This PR should be considered as introducing a breaking change, and one that isn't protected by types.

That is correct, but the new DeleteNodeFull response is not yet published in any crates.io release (AFAIK), so this change only breaks users of the master branch. I think that's a reasonable tradeoff, and the new ordering makes more sense.

setzer22 commented 1 year ago

This makes me thing we may want to document the order in which events are sent at some point, so that users can rely on it.

An alternative would be to document the order as being explicitly unspecified, so that users are aware of it and sort them in their preferred way (if needed). But I'm not sure this is a good idea :thinking:

bpostlethwaite commented 1 year ago

awesome thanks, that removes some unnecessary cloning and munging of the responses on my end