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

Add logic to disallow child items in tree views #453

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

ltrzesniewski
Copy link
Contributor

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

ptdev commented Nov 21, 2022

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

ptdev commented Nov 21, 2022

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

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

ptdev commented Nov 21, 2022

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

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

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

ptdev commented Nov 21, 2022

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

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

@danweav1
Copy link

danweav1 commented Oct 8, 2024

Any updates on this?

@punker76
Copy link
Owner

punker76 commented Oct 8, 2024

Sorry if it looks like I'm ignoring it. But it's not like that. It's on my TODO list.

@punker76 punker76 added this to the 4.0.0 milestone Oct 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TreeView insert index is wrong when item can't have children
5 participants