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

Clear drag preview nodes on NOTIFICATION_DRAG_END #87125

Conversation

ryevdokimov
Copy link
Contributor

This didn't really work before. It took seconds for the preview node to actually delete when 'ui_cancel' was pressed before dropping, and there are certain situations where you could keep the preview node in the viewport indefinitely, such as panning around.

2D Before:

2024-01-12.12-53-53.mp4

2D After:

2024-01-12.12-57-40.mp4

3D Before:

2024-01-12.12-55-31.mp4

3D After:

2024-01-12.12-57-51.mp4

@ryevdokimov ryevdokimov requested review from a team as code owners January 12, 2024 18:03
@AThousandShips AThousandShips added this to the 4.3 milestone Jan 12, 2024
@ryevdokimov ryevdokimov force-pushed the fix-removing-preview-nodes-on-ui-cancel branch from 24f777a to 346e3fa Compare January 12, 2024 18:05
@YuriSizov YuriSizov requested a review from Sauermann January 12, 2024 19:51
Copy link
Contributor

@Sauermann Sauermann left a comment

Choose a reason for hiding this comment

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

In its current form, this PR adds an additional call to the Controls can_drop_data() just to check within this call, if ui_cancel is pressed. This adds unnecessary can_drop_data() calls just to get notified, that dragging was cancelled.
When dragging ends, there is a NOTIFICATION_DRAG_END sent to the nodes. It would be better to listen for that notification and do something along the idea of the following lines:

diff --git a/editor/plugins/canvas_item_editor_plugin.cpp b/editor/plugins/canvas_item_editor_plugin.cpp
index f16aff555c..f8f2f0da7d 100644
--- a/editor/plugins/canvas_item_editor_plugin.cpp
+++ b/editor/plugins/canvas_item_editor_plugin.cpp
@@ -6027,6 +6027,10 @@ void CanvasItemEditorViewport::_notification(int p_what) {
                case NOTIFICATION_EXIT_TREE: {
                        disconnect("mouse_exited", callable_mp(this, &CanvasItemEditorViewport::_on_mouse_exit));
                } break;
+
+               case NOTIFICATION_DRAG_END: {
+                       _remove_preview();
+               }
        }
 }

It is likely that with this approach other calls to hide the label could be removed from the code.

This approach has the advantage, that it contains no changes to the viewport core class and aligns with the "Prefer local solutions" best practice.

@ryevdokimov ryevdokimov force-pushed the fix-removing-preview-nodes-on-ui-cancel branch from 346e3fa to 1022c19 Compare January 13, 2024 00:17
@ryevdokimov
Copy link
Contributor Author

ryevdokimov commented Jan 13, 2024

That makes a lot of sense, thank you. Changes applied and tested.

Copy link
Contributor

@Sauermann Sauermann left a comment

Choose a reason for hiding this comment

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

It looks right, but a review from someone with more expertise regarding editor plugins would help more, since I'm not familiar with them.

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.

Commit message/title probably needs a tweak.

@ryevdokimov
Copy link
Contributor Author

Could be. I'm open to suggestions, how can I improve it?

@KoBeWi
Copy link
Member

KoBeWi commented Feb 3, 2024

You could use the title of my PR - "Clear editor drag preview when drag ends".

@ryevdokimov
Copy link
Contributor Author

Yeah, I can see that.

I wanted to point out this issue occurs specifically when ui_cancel is pressed, but your suggestion explains more directly what is happening. I'll go ahead and change it.

@ryevdokimov ryevdokimov force-pushed the fix-removing-preview-nodes-on-ui-cancel branch from 1022c19 to 7a2c386 Compare February 3, 2024 23:50
@ryevdokimov ryevdokimov changed the title Fix removing preview nodes on 'ui_cancel' Clear drag preview nodes on NOTIFICATION_DRAG_END Feb 3, 2024
@akien-mga akien-mga merged commit e096be8 into godotengine:master Feb 5, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@ryevdokimov ryevdokimov deleted the fix-removing-preview-nodes-on-ui-cancel branch May 12, 2024 13:30
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.

6 participants