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

Fix reparenting after hover delay #91265

Merged
merged 1 commit into from
Jun 10, 2024

Conversation

AeioMuch
Copy link
Contributor

@AeioMuch AeioMuch commented Apr 28, 2024

Fix #91254
Also keep all nodes selected after reparenting and not just top level ones (before hover delay, because after it's the hovered/inspected node that should be the one selected)

@AeioMuch AeioMuch requested a review from a team as a code owner April 28, 2024 00:37
@Hilderin
Copy link
Contributor

Seems to work perfectly. Thanks!
The only difference with before is that when hovered the rectangle around the node is gone. I think it's brilliant because the rectangle around the node was interfering with the "drop line" making it difficult to see.

@ajreckof
Copy link
Member

One thing that makes me ticks is that the bug was supposedly introduced by #90653 but this fix does not touch code related. This might still be the proper fix but ask the question on why it does not implicate the code that broke it. I'll try to take a look when I have time most likely sometime between monday and tuesday.

@AeioMuch
Copy link
Contributor Author

AeioMuch commented Apr 28, 2024

Hello 👋

Seems to work perfectly. Thanks! The only difference with before is that when hovered the rectangle around the node is gone. I think it's brilliant because the rectangle around the node was interfering with the "drop line" making it difficult to see.

Thanks for your test and feedback!

One thing that makes me ticks is that the bug was supposedly introduced by #90653 but this fix does not touch code related. This might still be the proper fix but ask the question on why it does not implicate the code that broke it. I'll try to take a look when I have time most likely sometime between monday and tuesday.

The way I was ensuring that the inspected/hovered Node/TreeItem got selected and added to history when the drag ended was by using scene_tree->set_selected(node_hovered_now);, but since the changes in your PR apparently, it look like it update while the drag is happening or something, causing the issue. Just preventing the signal to be emited by passing false scene_tree->set_selected(node_hovered_now, false); avoided the issue, but then the problem was that it was not added to the editor selection history anymore after, so I did it when the drag end notification happen instead. I don't think that your PR is problematic per se, just that it changed the previous behaviour which I was relying on initially, but would be better if you can check that there's not more to it indeed.

While I was at it, I removed the use of the marked property and directly changed the text color to accent color which visually achieve the same result, with the bonus that it does not lose the accent outline around the target node for the reparent. set_marked is forcing the tree to update, causing it to lose the outline, and since now they stay visible I felt it was also better to not set the cursor on TreeItem because that gray outline was overlapping the other, as pointed by @Hilderin ... 😅

Anyway, if you can take a look at it when you have the time that would be greatly appreciated ! If you think you need to superseed (I think that's the term) it to make some more in depth changes, no problem, or if I need to make more corrections, let me know 👍.

Thanks a lot !

@AeioMuch
Copy link
Contributor Author

AeioMuch commented Apr 30, 2024

Retested this, and realized this approach still have issue with the path(s) used in the history when reparenting.

EDIT : The issue had nothing to do with this fix

@AThousandShips
Copy link
Member

Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look fine. There is a minor issue where the inspected node does not appear at the top, but it can be fixed later:

godot.windows.editor.dev.x86_64_HVzun4zCyh.mp4

@AeioMuch
Copy link
Contributor Author

AeioMuch commented May 31, 2024

The changes look fine. There is a minor issue where the inspected node does not appear at the top, but it can be fixed later:

godot.windows.editor.dev.x86_64_HVzun4zCyh.mp4

Yes indeed, right now it will do it only once the drag is ended. Tho I've realized another small issue caused with those new changes is that if you inspect a node by hovering it, then while dragging towards the inspector you somehow pass the mouse over another node it would not select the inspected node at the end of the drag but the last one hovered instead, so I've made another small change : instead of selecting the node_hovered_now at the end of the drag, I select the edited object in the inspector which should be the one selected at the end of the drag in this case.

Copy link
Member

@ajreckof ajreckof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really good! There is a problem with selection history but this can be fixed later as it is non blocking and only slightly related

Comment on lines +2144 to +2147

for (Node *E : full_selection) {
editor_selection->add_node(E);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this needed? I'm wondering as this might make #91860 resurface

Copy link
Contributor Author

@AeioMuch AeioMuch May 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case it is for consistency so that when you reparent using the right click option Reparent it also keep all the reparented nodes selected. I will test with the fix for the issue you pointed to make sure it does not brake it again, if it does tho I will just remove that for now since it is not that important

EDIT : Tested with the fix and it does not make the issue you've mentionned resurface

@akien-mga akien-mga merged commit 553f776 into godotengine:master Jun 10, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

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.

Scene tree drag&drop does not work after delay
7 participants