Add drag and drop to TextEdit #55294

ConteZero · 2021-11-24T21:27:12Z

I've implemented the drag and drop operation on TextEdit.

Update: I've added support for copy when CTRL is pressed in both TextEdit and LineEdit as suggested by @KoBeWi

ConteZero · 2021-11-24T22:02:56Z

This commit should solve godotengine/godot-proposals#3488

KoBeWi · 2021-11-24T23:14:30Z

Nice! This wasn't apparent with the previous PRs, but it would be useful if holding Ctrl would make the text being copied instead of moved.

ConteZero · 2021-11-25T10:20:02Z

Nice! This wasn't apparent with the previous PRs, but it would be useful if holding Ctrl would make the text being copied instead of moved.

I've added support for copy when CTRL is pressed in both TextEdit and LineEdit

KoBeWi · 2021-11-26T15:56:29Z

When moving a text within one TextEdit, it's registered as 2 operations (remove -> add), so you need to undo twice to revert moving. This should be merged into one undo step (check "complex operation" stuff in TextEdit).

ConteZero · 2021-11-26T16:46:28Z

When moving a text within one TextEdit, it's registered as 2 operations (remove -> add), so you need to undo twice to revert moving. This should be merged into one undo step (check "complex operation" stuff in TextEdit).

Fixed

KoBeWi · 2021-11-26T16:55:31Z

This is not very important, but it's not possible to copy-drop at the end of selection:

While expected is this:

Also for some reason, when I create a TextEdit and run the project, drag and drop doesn't work.

ConteZero · 2021-11-26T17:21:47Z

This is not very important, but it's not possible to copy-drop at the end of selection:

I can try to fix it, but I suspect that this is tricky to implement.

Also for some reason, when I create a TextEdit and run the project, drag and drop doesn't work.

Yes, TextEdit has many problems when you run the project, but it is not caused by my changes.
I think it is related to #54955

KoBeWi · 2021-11-26T17:25:36Z

I can try to fix it, but I suspect that this is tricky to implement.

Well, if it's too difficult then it's not worth it.

KoBeWi

I tested and it works fine (except the runtime bug, but it's unrelated issue). The code is similar to LineEdit changes, so it's likely good to, but @Paulb23 might want to take a look too.

ConteZero · 2021-11-26T20:09:19Z

I can try to fix it, but I suspect that this is tricky to implement.

Well, if it's too difficult then it's not worth it.

I've done some small changes to handle the copy-drop at the beginning and at the end of selection.

Paulb23 · 2021-11-28T16:52:05Z

scene/gui/text_edit.cpp

+		if (!text_changed_dirty) {
+			if (is_inside_tree()) {
+				MessageQueue::get_singleton()->push_call(this, "_text_changed");
+			}
+			text_changed_dirty = true;
+		}


No need to call the MessageQueue in TextEdit

Paulb23 · 2021-11-28T16:52:50Z

scene/gui/text_edit.cpp

@@ -5691,6 +5825,43 @@ void TextEdit::_post_shift_selection() {
 	selection.selecting_text = true;
 }

+bool TextEdit::_is_caret_inside_selection(bool p_edge) const {
+	Point2i pos = get_line_column_at_pos(get_local_mouse_pos());


We should check the selection here rather then in external ifs.

Paulb23 · 2021-11-28T16:57:30Z

scene/gui/text_edit.cpp

@@ -5691,6 +5825,43 @@ void TextEdit::_post_shift_selection() {
 	selection.selecting_text = true;
 }

+bool TextEdit::_is_caret_inside_selection(bool p_edge) const {


This checks the mouse not not the caret, would be better called is_mouse_over_selection or is_cursor_over_selection, also would be nice to expose to the API.

Paulb23 · 2021-11-28T17:07:06Z

scene/gui/text_edit.cpp

@@ -1538,6 +1564,9 @@ void TextEdit::gui_input(const Ref<InputEvent> &p_gui_input) {

 						update();
 					}
+				} else if (is_editable() && has_selection() && _is_caret_inside_selection()) {


We should allow dragging from a TextEdit even if it's not editable.

Paulb23 · 2021-11-28T17:30:02Z

scene/gui/text_edit.cpp

+				if (is_editable() && has_selection() && _is_caret_inside_selection()) {
+					selection.drag_attempt = true;
+				}


Not sure on the use case this is solving, as it should be caught in the above statement, should be safe to remove this?
Testing, locally, it seems to prevent word and line selection modes.

This is needed when you do a double or triple click and you keep the left mouse button pressed on your last click to start dragging.
The code here is really tricky, but I've added the drag and drop feature trying to avoid dangerous refactoring of the actual code.

This is needed when you do a double or triple click and you keep the left mouse button pressed on your last click to start dragging.

We should't start dragging in this use case, it should continue to select text in the given mode.

This behavior has been kept from original LineEdit before my changes, this is the discussion #54339 (review)
I can change it in both LineEdit and TextEdit (and perhaps also in my commit for RichTextLabel), I've implemented it only because of KoBeWi comment.
Let me know what should be the expected behavior in these cases in Godot.

Looking around most editors have text dragging as a separate action, so I would expect Godot to do the same.

I only hope that users do not complain about that change in the previous behavior (I'm referring to LineEdit)

I've removed the drag start in case of double or triple click from TextEdit and LineEdit

Paulb23 · 2021-11-28T17:49:06Z

scene/gui/text_edit.cpp

@@ -2406,6 +2460,86 @@ bool TextEdit::is_text_field() const {
 	return true;
 }

+Variant TextEdit::get_drag_data(const Point2 &p_point) {
+	if (selection.drag_attempt && selection.active) {


Nit: Can invert this.

Paulb23 · 2021-11-28T17:50:32Z

scene/gui/text_edit.cpp

+void TextEdit::drop_data(const Point2 &p_point, const Variant &p_data) {
+	Control::drop_data(p_point, p_data);
+
+	if (p_data.get_type() == Variant::STRING && is_editable()) {


Can invert this.

Paulb23 · 2021-11-28T17:52:55Z

scene/gui/text_edit.h

 	virtual String get_tooltip(const Point2 &p_pos) const override;
+


Accidental new line.

Paulb23 · 2021-11-28T17:56:18Z

scene/gui/text_edit.cpp

+			}
+			text_changed_dirty = true;
+		}
+		update();


No need to call update here.

Paulb23 · 2021-11-28T18:11:32Z

scene/gui/text_edit.cpp

+			}
+		}
+	}
+	return false;


This seems overly complex, would something like the following work?

if (p_edge) { if ((row == selection.from_line && col == selection.from_column) || (row == selection.to_line && col == selection.to_column)) { return true } } return (row > selection.from_line && row <= selection.to_line && (row > selection.from_line || col >= selection.from_column) && (row < selection.to_line || col < selection.to_column))

I've used the first part of your code to simplify the current code.
But the second part

return (row > selection.from_line && row <= selection.to_line && (row > selection.from_line || col >= selection.from_column) && (row < selection.to_line || col < selection.to_column))

is not correct, it does not handle all the possible cases.

What use cases are missing from that statement?

To be honest I didn't thoroughly analyze that line of code, I stopped at
row > selection.from_line &&
that exclude the first line of selection, so I assumed you misunderstood the meaning of the p_edge parameter and used my code.

Looking back at your code I now understand what you wanted to do, but to make it work some changes are needed:

row > selection.from_line --> row >= selection.from_line

and

col >= selection.from_column --> col > selection.from_column

ConteZero · 2021-11-29T08:31:13Z

@Paulb23 as we discussed here #54339 (comment)
I've submitted this pull request #55411

Paulb23 · 2021-12-02T17:02:38Z

scene/gui/text_edit.cpp

+			if (pos.y <= get_first_visible_line()) {
+				_scroll_lines_up();
+			} else if (pos.y >= get_last_full_visible_line()) {
+				_scroll_lines_down();
+			}


We shouldn't need to do this, set_caret_column should adjust the viewport for us.

Yes, in theory it shouldn't be necessary, but in practice the scroll won't work without this code.
I have the possibility to test only on Linux, so I don't know if the problem is limited to this platform, but without the code I added the scroll only works if you move the selection from top to bottom, if you move from the bottom upward the scroll does not work.
I can add a comment at the beginning of the code to indicate that this is a temporary workaround, if you remove that code the drag functionality would be almost unusable.

I would still remove it for this PR, not a keen on adding temporary workarounds as they quickly become permanent. The base issue should be fixed instead.

Paulb23 · 2021-12-02T17:17:10Z

scene/gui/text_edit.cpp

+void TextEdit::drop_data(const Point2 &p_point, const Variant &p_data) {
+	Control::drop_data(p_point, p_data);
+
+	if (is_editable() && p_data.get_type() == Variant::STRING) {


Nit: Can invert this

Paulb23 · 2021-12-02T17:18:59Z

scene/gui/text_edit.cpp

+				set_caret_line(caret_row_tmp, true, false);
+				set_caret_column(caret_column_tmp);
+				insert_text_at_caret(p_data);
+				end_complex_operation();


We shouldn't need to call delete_selection here as insert_text_at_caret will handle that. This should also remove the need to make this a complex operation. Similar for the else if below.

I've removed the second delete_selection after the else if
The first one instead is needed, because is true that insert_text_at_caret call delete_selection, but delete_selection reset the caret position at the beginning of the selection and this break the code for the drag and drop.

akien-mga · 2021-12-02T22:19:59Z

Thanks!

ConteZero requested a review from a team as a code owner November 24, 2021 21:27

ConteZero force-pushed the text_edit_drag_and_drop branch from 3e901a5 to bdddf2d Compare November 24, 2021 21:41

KoBeWi added enhancement topic:gui usability labels Nov 24, 2021

KoBeWi added this to the 4.0 milestone Nov 24, 2021

ConteZero force-pushed the text_edit_drag_and_drop branch from bdddf2d to 48e652e Compare November 25, 2021 10:17

ConteZero mentioned this pull request Nov 26, 2021

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

Merged

ConteZero force-pushed the text_edit_drag_and_drop branch from 48e652e to 314d091 Compare November 26, 2021 16:40

KoBeWi approved these changes Nov 26, 2021

View reviewed changes

ConteZero force-pushed the text_edit_drag_and_drop branch from 314d091 to b351576 Compare November 26, 2021 20:06

ConteZero force-pushed the text_edit_drag_and_drop branch from b351576 to 31892eb Compare November 26, 2021 20:16

Paulb23 requested changes Nov 28, 2021

View reviewed changes

ConteZero force-pushed the text_edit_drag_and_drop branch from 31892eb to ec142a2 Compare November 28, 2021 22:10

ConteZero requested a review from a team as a code owner November 28, 2021 22:10

ConteZero force-pushed the text_edit_drag_and_drop branch from ec142a2 to 6fddac9 Compare November 29, 2021 06:49

ConteZero force-pushed the text_edit_drag_and_drop branch 3 times, most recently from e3085c0 to b7ad3dd Compare December 1, 2021 21:46

Paulb23 reviewed Dec 2, 2021

View reviewed changes

Add drag and drop to TextEdit

0699941

ConteZero force-pushed the text_edit_drag_and_drop branch from b7ad3dd to 0699941 Compare December 2, 2021 21:01

Paulb23 approved these changes Dec 2, 2021

View reviewed changes

akien-mga merged commit 543462e into godotengine:master Dec 2, 2021

CrezyDud mentioned this pull request Dec 3, 2021

Double clicking on a word is broken #55588

Closed

Calinou mentioned this pull request Jan 11, 2022

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

Closed

ConteZero deleted the text_edit_drag_and_drop branch March 15, 2022 08:04

		virtual String get_tooltip(const Point2 &p_pos) const override;

Add drag and drop to TextEdit #55294

Add drag and drop to TextEdit #55294

Conversation

ConteZero commented Nov 24, 2021 • edited Loading

ConteZero commented Nov 24, 2021

KoBeWi commented Nov 24, 2021

ConteZero commented Nov 25, 2021

KoBeWi commented Nov 26, 2021 • edited Loading

ConteZero commented Nov 26, 2021

KoBeWi commented Nov 26, 2021

ConteZero commented Nov 26, 2021 • edited Loading

KoBeWi commented Nov 26, 2021

KoBeWi left a comment

Choose a reason for hiding this comment

ConteZero commented Nov 26, 2021

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

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

ConteZero commented Nov 29, 2021

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 Dec 2, 2021

ConteZero commented Nov 24, 2021 •

edited

Loading

KoBeWi commented Nov 26, 2021 •

edited

Loading

ConteZero commented Nov 26, 2021 •

edited

Loading