search

Blazor-Diagrams / Blazor.Diagrams

A fully customizable and extensible all-purpose diagrams library for Blazor
https://blazor-diagrams.zhaytam.com
MIT License
919 stars 176 forks source link

Bugfix/#359 should delete constraint not checked on remove control #371

Closed K0369 closed 8 months ago

K0369 commented 8 months ago

I have added a small fix for #359.

The RemoveControl is now checking if the "ShouldDelete" constraint is fulfilled before deleting node-models or links.

I have actually prepared the same change for groups too, but not added it to the PR because the behavior is actually different for groups, depending on the children (should they be removed, what if they have constraints, ...).

The current behavior for groups is to use the same path as node models (because they effectively are), which means that they aren't deleted because they are not in the NodeLayer. Also they don't use the "ShouldDeleteGroup" constraint, but the "ShouldDeleteNode" constraint.

Please let me know which group behavior you want to have implemented:

  1. Delete checks the "ShouldDeleteGroup"-constraint and if fulfilled removing group from group layer
  2. Checking the "ShouldDeleteGroup"-constraint and if fulfilled deleting group with all children recursively
  3. A different behavior
  4. No changes
zHaytam commented 8 months ago

Hey, thanks for yet another PR!

I would say that the behavior of the control should be the same as the keyboard shortcut (so delete group, not remove).
https://github.com/Blazor-Diagrams/Blazor.Diagrams/blob/master/src/Blazor.Diagrams.Core/Behaviors/KeyboardShortcutsDefaults.cs#L17

Now that I see this, I even forgot to check if the model is locked, could you also add that?

K0369 commented 8 months ago

Sure.

Seeing that these deletion checks are used at least two times (in the KeyboardShortcutBehavior and now in the RemovalControl), it would maybe make sense to add this functionality to the diagram (e.g. as an extension). I'll see what I can come up with.