punker76 / gong-wpf-dragdrop

The GongSolutions.WPF.DragDrop library is a drag'n'drop framework for WPF
BSD 3-Clause "New" or "Revised" License
2.25k stars 392 forks source link

Add logic to disallow child items in tree views #453

Open ltrzesniewski opened 1 year ago

ltrzesniewski commented 1 year ago

What changed?

This fixes #344, in a different way than #345.

I wanted to fix this issue, but I felt it's the role of the drop handler to decide whether an item should accept children or not (instead of handling this through a dependency property of the item). This also enables providing custom logic.

Problem is, at that point the IDropInfo is already populated with the "wrong" TargetCollection/InsertIndex info because everything is decided upfront in the constructor of DropInfo.

I didn't want to introduce a breaking change, so this solution isn't ideal. Please tell me if this approach is OK or if you'd prefer something else.

This adds a new AcceptChildItem property in DropInfo (specifically not in IDropInfo in order to avoid the breaking change), which will recalculate the relevant properties when set to false by the user.

Here's the result:

files-example

/cc @jizc

ptdev commented 1 year ago

This solves this exact issue I was having.

But I may have found an issue. When using this change, on the drop handler, dropInfo.TargetItem will still reference the model that didn't allow for the drop and not the model where the source was eventually dropped (usually its parent).

Can you confirm this is happening? And is it by design for some reason?

ptdev commented 1 year ago

Here's some more info on this with a couple of screenshots:

On this case: ss1

Note the cursor's position on top of the "Calculator" item even though the drop indicator shows that it will be dropped as a sibling (inside "Example sub-folder"). On the Drop handler, the dropinfo.Target here is "Calculator" (when I believe it should be "Example Sub-Folder")

And on this case: ss2

Again, almost the same as above but note the cursor's position on top of "CMD". The dropinfo.Target will be "CMD" (when again, I'm guessing it should be "Example Sub-Folder")

Hope this helps, cheers.

ltrzesniewski commented 1 year ago

I sent this PR a while ago, but I believe this is by design. The code in the demo uses TargetItem to identify whether the target item accepts children:

if (dropInfo is DropInfo { TargetItem: not TreeNode { Icon: PackIconMaterialKind.Folder } } typedDropInfo)
    typedDropInfo.AcceptChildItem = false;

It wouldn't feel right for TargetItem to change when AcceptChildItem changes.

Also, InsertPosition should contain either BeforeTargetItem or AfterTargetItem in that case (the TargetItemCenter flag should not be set), which makes the role of TargetItem clear.

ptdev commented 1 year ago

Hey, thanks for replying.

That's fine if it's by design and it's working great for the dropover event.

The issue I see is that on the drop event (which is where we would eventually do any needed model updates or any extra work after the drop is completed) there is apparently no direct reference on the dropInfo parameter to where the drag source was actually eventually dropped when using AcceptChildItem=false (which will drop it as a sibling and not a child).

So if we then follow something like the drop event example on the documentation, that dropInfo.TargetItem reference will always reference the exact item that we did not want to drop on by setting AcceptChildItem=false and therefore reference the wrong target.

But I get what you said, it's not really the wrong target. It's the correct target the mouse was over when it dropped, but just not the target where the object was eventually dropped, since it had AcceptChildItem=false. I would still suggest that it would be very useful to add a reference to where it was actually dropped if this PR or something similar is ever merged.

Thanks again for this PR and for replying 👍

ltrzesniewski commented 1 year ago

I would still suggest that it would be very useful to add a reference to where it was actually dropped if this PR or something similar is ever merged.

Yes, I agree this would be useful, but the API this PR currently provides doesn't make that easy, as DropInfo contains lots of properties, and I wouldn't like to add a RealTargetItem or similar.

Maybe changing the AcceptChildItem property into a RejectChildItem() method would solve the issue though. I suppose it wouldn't feel wrong to change TargetItem through a method, and you'd be able to call it several times to go up the hierarchy as needed (not sure how viable/useful that may be).

I'd like some input from @punker76 before I make this change though.

ltrzesniewski commented 1 year ago

Though, it seems the current library design may make it necessary to keep the TargetItem as-is because of the InsertPosition property. I'm not sure how to solve this properly... 😕

ptdev commented 1 year ago

No problem of course.

Thanks anyway for tackling this issue in the first place. The dropover behavior this PR adds and the AcceptChildItem=false parameter is working great and hopefully will eventually be added in one way or another.

In the meantime I'll probably just actually add some sort of RealTargetItem/FinalTargetItem property on my local copy since, like you, I was already pondering it as the quickest fix for my case, even if it may not be ideal for this project as a final solution.

Thanks again, cheers 👍

gaetandezeiraud commented 9 months ago

Any news on that? I encounter the same problem solved by this PR.