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

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

Merged
merged 3 commits into from
Oct 27, 2023
Merged

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

merged 3 commits into from
Oct 27, 2023

Conversation

K0369
Copy link
Contributor

@K0369 K0369 commented Oct 23, 2023

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
Copy link
Collaborator

zHaytam commented Oct 26, 2023

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
Copy link
Contributor Author

K0369 commented Oct 26, 2023

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.

@zHaytam zHaytam merged commit 605028d into Blazor-Diagrams:develop Oct 27, 2023
1 check failed
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