Fix drag and drop on LineEdit #54339

ConteZero · 2021-10-28T07:14:58Z

LineEdit contains the code to handle drag and drop but it never gets executed, I don't know if it's excluded on purpose or it's a bug.
I have improved the existing code to make it possible to cut and paste the selection (the current code only makes a copy).
I don't know if this is the best way to implement this functionality, but from my tests it works and with some tweaks it could be used for the TextEdit node as well.

Bugsquad edit: Fixes #54744.

KoBeWi · 2021-10-31T16:17:59Z

Nice feature, I'd definitely like to see it in TextEdit too.

Dropping doesn't properly support undo/redo though:

ConteZero · 2021-10-31T18:19:18Z

Nice feature, I'd definitely like to see it in TextEdit too.

Dropping doesn't properly support undo/redo though

Yes, you are right, I force-pushed a change that should fix undo/redo.

scene/gui/line_edit.cpp

KoBeWi

Seems ok, but core changes need additional review.

btw, the unreported issue fixed by this PR is actually a regression, because drag and drop works in 3.x (except undo/redo is broken).

ConteZero · 2021-11-01T08:51:26Z

Seems ok, but core changes need additional review.

btw, the unreported issue fixed by this PR is actually a regression, because drag and drop works in 3.x (except undo/redo is broken).

Are you sure? I've tested in 3.3.4 and drag and drop in lineedit does not work for me, and this is confirmed by looking at the code, the shift_selection_check_pre that causes the deselect is still there so the condition

} else if (selection.enabled) {
	selection.drag_attempt = true;
}

is never executed.

ConteZero · 2021-11-01T08:58:30Z

I've added some lines of code to LineEdit::drop_data to handle the case where deselect_on_focus_loss_enabled is false.
I've tried to mimic the behavior of classic cut and paste, so if you drag the selection in a different lineedit that have already a text selected, then the grabbed text will replace the one in the existing selection.

KoBeWi · 2021-11-01T09:37:23Z

Are you sure?

3.4 rc1

ConteZero · 2021-11-01T10:08:00Z

Well, I think we can call it the "Schrödinger's Drag and Drop", it works and doesn't work at the same time.
If you do a double or triple click and you keep the left mouse button pressed on your last click then you can grab the selection.
If you don not keep the left mouse button pressed or if you try to only select some letters of a word than the grab operation does not start.
In your example try to drag only a part of the word "test", like "st" for example.

Paulb23 · 2021-11-03T17:39:51Z

scene/gui/line_edit.cpp

+					delete_text(selection.begin, selection.end);
+					deselect();


Can call selection_delete()

Paulb23 · 2021-11-03T17:40:47Z

scene/gui/line_edit.cpp

+			if (b->is_shift_pressed()) {
+				shift_selection_check_pre(true);
+			}


Would it be worth checking drag_action inside shift_selection_check_pre so we don't have to special case it here?

Wouldn't moving the control inside shift_selection_check_pre cause unwanted side effects?

Shouldn't do as this is the only call that is used for the mouse, though that does raise a good point, we should probably prevent any another input whilst dragging text.

Yes, preventing other input while dragging will avoid unwanted behavior, but I believe this needs to be done in another pull request because it should be done in a generic way, not limited to just Lineedit.

Paulb23 · 2021-11-03T17:58:20Z

scene/gui/line_edit.cpp

-		selection.begin = caret_column - selected;
-		selection.end = caret_column;


Seems as though the old code tried to persevere the selection, should this behaviour be kept?

I've restored the previous behavior and I've also fixed a bug in the undo/redo code by adding a deselect()

Paulb23 · 2021-11-03T18:06:39Z

scene/gui/line_edit.cpp

+				set_caret_column(caret_column_tmp);
+				insert_text_at_caret(p_data);
+			}
+		} else if (!deselect_on_focus_loss_enabled && selection.enabled && caret_column >= selection.begin && caret_column <= selection.end) {


Doesn't selection.enabled tell us if deselect_on_focus_loss_enabled is enabled?

Paulb23 · 2021-11-07T17:42:55Z

scene/gui/line_edit.cpp

+			delete_text(selection.begin, selection.end);
+			deselect();


Can call selection_delete.

Paulb23 · 2021-11-07T17:43:00Z

scene/gui/line_edit.cpp

+				delete_text(selection.begin, selection.end);
+				deselect();


Can call selection_delete.

Paulb23 · 2021-11-07T17:44:16Z

scene/gui/line_edit.cpp

+		if (selecting_enabled) {
+			selection.enabled = true;
+			selection.begin = caret_column_tmp;
+			selection.end = caret_column;
+		}


Can call select()

Paulb23 · 2021-11-07T18:22:00Z

scene/gui/line_edit.cpp

+			insert_text_at_caret(p_data);
+			grab_focus();
+		} else {
+			deselect();


Can remove this call to deselect as we're immediately selecting afterwords.

Paulb23 · 2021-11-07T18:34:27Z

scene/gui/line_edit.cpp

+			if (b->is_shift_pressed()) {
+				shift_selection_check_pre(true);
+			}


Shouldn't do as this is the only call that is used for the mouse, though that does raise a good point, we should probably prevent any another input whilst dragging text.

akien-mga · 2021-11-08T12:13:36Z

Be sure to check how it behaves with non editable LineEdits too (see #54744).

ConteZero · 2021-11-08T15:05:32Z

Be sure to check how it behaves with non editable LineEdits too (see #54744).

#54744 should be fixed by this this commit.
If this pull request will be accepted my plan is to do a backport to 3.x

Paulb23

Line edit changes look okay.

However, I'm not sure on the new NOTIFICATIONs, kind of makes NOTIFICATION_DRAG_END redundant. We can also handle quite easily inside of LineEdit rather then pushing up to viewport, though I can see the use case for it.

Would need some other opinions on this.

Paulb23 · 2021-11-09T20:06:56Z

scene/gui/line_edit.cpp

 }

 void LineEdit::drop_data(const Point2 &p_point, const Variant &p_data) {
 	Control::drop_data(p_point, p_data);

-	if (p_data.get_type() == Variant::STRING) {
+	if (is_editable() && p_data.get_type() == Variant::STRING) {


Nit: Can invert this.

akien-mga · 2021-11-09T21:10:20Z

However, I'm not sure on the new NOTIFICATIONs, kind of makes NOTIFICATION_DRAG_END redundant. We can also handle quite easily inside of LineEdit rather then pushing up to viewport, though I can see the use case for it.

Indeed, I don't see much added value for those notifications, and changing the core like this requires thorougher design review than only ad-hoc LineEdit changes. See https://docs.godotengine.org/en/latest/community/contributing/best_practices_for_engine_contributors.html#prefer-local-solutions

The two new notifications only serve to say "drag end with success" or "drag end with failure", and success or failure is determined by a method implemented in LineEdit. So LineEdit already knows if it's drag is a success or a failure, and can pass this information (e.g. via a member variable) to the existing NOTIFICATION_DRAG_END notification. All the Viewport changes seem unnecessary here.

ConteZero · 2021-11-09T21:46:38Z

The two new notifications only serve to say "drag end with success" or "drag end with failure", and success or failure is determined by a method implemented in LineEdit. So LineEdit already knows if it's drag is a success or a failure, and can pass this information (e.g. via a member variable) to the existing NOTIFICATION_DRAG_END notification. All the Viewport changes seem unnecessary here.

I've not understood what you are suggesting when you talk about a member variable, you mean a member variable of Lineedit?
If this is the case, it will not work when you want to drag and drop from different nodes, as example from two Lineedit nodes, or from Lineedit and Textedit.
Currently the node where the drag operation starts do not know if at the end of the drag the drop is successful or not, this is only known from the node where the drop happens (or from the Viewport that handle the drag and drop at a higher level).
Without this information it is impossible to react accordingly, in case of Lineedit you do not know if you can delete the selected text or not.
Perhaps I'm missing something obvious, but I've not found a way to get this info without passing from Viewport.

ConteZero · 2021-11-10T06:53:22Z

However, I'm not sure on the new NOTIFICATIONs, kind of makes NOTIFICATION_DRAG_END redundant. We can also handle quite easily inside of LineEdit rather then pushing up to viewport, though I can see the use case for it.

As I've tried to explain in the previous comment I don't think it is possible to handle the drag and drop operation without using Viewport.

ConteZero · 2021-11-10T08:54:04Z

I'll try to make it clearer with an example, suppose we have two LineEdit nodes

LineEdit_A contains textA
LineEdit_B contains textB

This is what happens during a drag and drop operation (only relevant method calls are shown):

# select "textA" on LineEdit_A and press Left Mouse Button
LineEdit_A --> selection.drag_attempt = true

# start Mouse Move
Viewport::_gui_input_event
LineEdit_A --> LineEdit::get_drag_data

# move mouse on LineEdit_B and release Left Mouse Button with caret at the end of "textB"
Viewport::_gui_input_event
Viewport::_gui_drop
LineEdit_B --> LineEdit::can_drop_data --> LineEdit::drop_data

# now LineEdit_B contains "textBtextA" but LineEdit_A do not know what to do until it receives the NOTIFICATION_DROP_DATA_SUCCESS
# LineEdit_A only know that the drag operation is started (selection.drag_attempt variable)
# if we send NOTIFICATION_DROP_DATA_SUCCESS from Viewport
# then LineEdit_A can delete "textA" and the drag and drop operation is completed

Paulb23 · 2021-11-10T17:52:33Z

An alternative solution, which already has a precedent in Viewport is gui.dragging, with a getter gui_is_dragging.

So we could add gui.drag_successful with a getter gui_drag_successful(), and a wrapper in Control to call get_viewport()->gui_drag_successful(). Then check that in LineEdit on NOTIFICATION_DRAG_END.

This way we don't have to touch notifications.

ConteZero · 2021-11-10T18:45:46Z

An alternative solution, which already has a precedent in Viewport is gui.dragging, with a getter gui_is_dragging.

So we could add gui.drag_successful with a getter gui_drag_successful(), and a wrapper in Control to call get_viewport()->gui_drag_successful(). Then check that in LineEdit on NOTIFICATION_DRAG_END.

This way we don't have to touch notifications.

At first I thought about using a getter, but then I chose to create the two new notifications because it seemed simpler to implement and easier for users to understand.

If you think that the getter is a better choice I can modify the code to use it instead of notifications, in the end the thing that interests me most is to have a drag and drop functionality that works (especially for the 3.x branch which is what I use for my projects).

ConteZero · 2021-11-15T11:17:06Z

@Paulb23 I've removed the notifications and added the getter as you suggested

akien-mga

Looks good to me as far as Control/Viewport changes are concerned.

Paulb23

LGTM.

akien-mga · 2021-11-22T11:20:05Z

Thanks!

ConteZero requested review from a team as code owners October 28, 2021 07:14

akien-mga added bug enhancement topic:gui labels Oct 28, 2021

akien-mga added this to the 4.0 milestone Oct 28, 2021

ConteZero force-pushed the line_edit_drag_and_drop branch from ea7742a to 696583c Compare October 30, 2021 13:29

ConteZero force-pushed the line_edit_drag_and_drop branch from 696583c to 9593d15 Compare October 31, 2021 18:07

KoBeWi reviewed Oct 31, 2021

View reviewed changes

scene/gui/line_edit.cpp Outdated Show resolved Hide resolved

KoBeWi reviewed Oct 31, 2021

View reviewed changes

scene/gui/line_edit.cpp Outdated Show resolved Hide resolved

ConteZero force-pushed the line_edit_drag_and_drop branch from 9593d15 to 7a04245 Compare October 31, 2021 20:59

KoBeWi reviewed Oct 31, 2021

View reviewed changes

scene/gui/line_edit.cpp Outdated Show resolved Hide resolved

KoBeWi reviewed Oct 31, 2021

View reviewed changes

scene/gui/line_edit.cpp Outdated Show resolved Hide resolved

KoBeWi approved these changes Oct 31, 2021

View reviewed changes

ConteZero force-pushed the line_edit_drag_and_drop branch from 7a04245 to b9c65aa Compare November 1, 2021 08:41

KoBeWi mentioned this pull request Nov 1, 2021

Add the ability to drag and drop text in the script editor with the mouse godotengine/godot-proposals#3488

Closed

Paulb23 reviewed Nov 3, 2021

View reviewed changes

ConteZero force-pushed the line_edit_drag_and_drop branch from b9c65aa to 9765e33 Compare November 3, 2021 21:49

Paulb23 reviewed Nov 7, 2021

View reviewed changes

ConteZero force-pushed the line_edit_drag_and_drop branch from 9765e33 to 272b48a Compare November 8, 2021 07:59

akien-mga requested a review from Paulb23 November 8, 2021 23:15

Paulb23 reviewed Nov 9, 2021

View reviewed changes

ConteZero force-pushed the line_edit_drag_and_drop branch from 272b48a to a70834e Compare November 10, 2021 14:36

ConteZero force-pushed the line_edit_drag_and_drop branch from a70834e to 8d86ad9 Compare November 15, 2021 11:12

ConteZero force-pushed the line_edit_drag_and_drop branch 2 times, most recently from 413915f to d1b2c75 Compare November 15, 2021 13:14

Fix drag and drop on LineEdit

2b1787b

ConteZero force-pushed the line_edit_drag_and_drop branch from d1b2c75 to 2b1787b Compare November 15, 2021 16:11

akien-mga approved these changes Nov 18, 2021

View reviewed changes

akien-mga requested a review from Paulb23 November 18, 2021 12:24

Paulb23 approved these changes Nov 20, 2021

View reviewed changes

akien-mga merged commit 78dbe4e into godotengine:master Nov 22, 2021

This was referenced Nov 26, 2021

[3.x] Add drag and drop to TextEdit, LineEdit, RichTextLabel #55355

Merged

Add drag and drop to TextEdit #55294

Merged

ConteZero deleted the line_edit_drag_and_drop branch March 15, 2022 08:07

		selection.begin = caret_column - selected;
		selection.end = caret_column;

Fix drag and drop on LineEdit #54339

Fix drag and drop on LineEdit #54339

Conversation

ConteZero commented Oct 28, 2021 • edited by akien-mga Loading

KoBeWi commented Oct 31, 2021

ConteZero commented Oct 31, 2021

KoBeWi left a comment • edited Loading

Choose a reason for hiding this comment

ConteZero commented Nov 1, 2021

ConteZero commented Nov 1, 2021

KoBeWi commented Nov 1, 2021

ConteZero commented Nov 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ConteZero Nov 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akien-mga commented Nov 8, 2021

ConteZero commented Nov 8, 2021

Paulb23 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akien-mga commented Nov 9, 2021

ConteZero commented Nov 9, 2021

ConteZero commented Nov 10, 2021

ConteZero commented Nov 10, 2021

Paulb23 commented Nov 10, 2021

ConteZero commented Nov 10, 2021

ConteZero commented Nov 15, 2021

akien-mga left a comment

Choose a reason for hiding this comment

Paulb23 left a comment

Choose a reason for hiding this comment

akien-mga commented Nov 22, 2021

ConteZero commented Oct 28, 2021 •

edited by akien-mga

Loading

KoBeWi left a comment •

edited

Loading

ConteZero Nov 3, 2021 •

edited

Loading