-
-
Notifications
You must be signed in to change notification settings - Fork 21.3k
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
Overhaul multicaret editing and selection in TextEdit #86978
Conversation
1fe9bc8
to
6c2e429
Compare
You're breaking compatibility and need to implement compatibility methods, if you need help I can instruct when I have the time 🙂 Here you go: diff --git a/misc/extension_api_validation/4.2-stable.expected b/misc/extension_api_validation/4.2-stable.expected
index a8b3af7891..49d7841693 100644
--- a/misc/extension_api_validation/4.2-stable.expected
+++ b/misc/extension_api_validation/4.2-stable.expected
@@ -28,3 +28,11 @@ GH-86687
Validate extension JSON: Error: Field 'classes/AnimationMixer/methods/_post_process_key_value/arguments/3': type changed value in new API, from "Object" to "int".
Exposing the pointer was dangerous and it must be changed to avoid crash. Compatibility methods registered.
+
+
+GH-86978
+--------
+Validate extension JSON: Error: Field 'classes/TextEdit/methods/set_line/arguments': size changed value in new API, from 2 to 3.
+Validate extension JSON: Error: Field 'classes/TextEdit/methods/swap_lines/arguments': size changed value in new API, from 2 to 3.
+
+YOUR DESCRIPTION. Compatibility methods registered.
diff --git a/scene/gui/text_edit.compat.inc b/scene/gui/text_edit.compat.inc
new file mode 100644
index 0000000000..708b9d9a9d
--- /dev/null
+++ b/scene/gui/text_edit.compat.inc
@@ -0,0 +1,46 @@
+/**************************************************************************/
+/* text_edit.compat.inc */
+/**************************************************************************/
+/* This file is part of: */
+/* GODOT ENGINE */
+/* https://godotengine.org */
+/**************************************************************************/
+/* Copyright (c) 2014-present Godot Engine contributors (see AUTHORS.md). */
+/* Copyright (c) 2007-2014 Juan Linietsky, Ariel Manzur. */
+/* */
+/* Permission is hereby granted, free of charge, to any person obtaining */
+/* a copy of this software and associated documentation files (the */
+/* "Software"), to deal in the Software without restriction, including */
+/* without limitation the rights to use, copy, modify, merge, publish, */
+/* distribute, sublicense, and/or sell copies of the Software, and to */
+/* permit persons to whom the Software is furnished to do so, subject to */
+/* the following conditions: */
+/* */
+/* The above copyright notice and this permission notice shall be */
+/* included in all copies or substantial portions of the Software. */
+/* */
+/* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, */
+/* EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF */
+/* MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. */
+/* IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY */
+/* CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, */
+/* TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE */
+/* SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. */
+/**************************************************************************/
+
+#ifndef DISABLE_DEPRECATED
+
+void TextEdit::_set_line_bind_compat_86978(int p_line, const String &p_new_text) {
+ set_line(p_line, p_new_text, true);
+}
+
+void TextEdit::_swap_lines_bind_compat_86978(int p_from_line, int p_to_line) {
+ swap_lines(p_from_line, p_to_line, true);
+}
+
+void TextEdit::_bind_compatibility_methods() {
+ ClassDB::bind_compatibility_method(D_METHOD("set_line", "line", "new_text"), &TextEdit::_set_line_bind_compat_86978);
+ ClassDB::bind_compatibility_method(D_METHOD("swap_lines", "from_line", "to_line"), &TextEdit::_swap_lines_bind_compat_86978);
+}
+
+#endif // DISABLE_DEPRECATED
diff --git a/scene/gui/text_edit.cpp b/scene/gui/text_edit.cpp
index e774722c86..ab6333ef0e 100644
--- a/scene/gui/text_edit.cpp
+++ b/scene/gui/text_edit.cpp
@@ -29,6 +29,7 @@
/**************************************************************************/
#include "text_edit.h"
+#include "text_edit.compat.inc"
#include "core/config/project_settings.h"
#include "core/input/input.h"
diff --git a/scene/gui/text_edit.h b/scene/gui/text_edit.h
index 8fdf05c73e..09de71dfc9 100644
--- a/scene/gui/text_edit.h
+++ b/scene/gui/text_edit.h
@@ -624,6 +624,12 @@ protected:
void _notification(int p_what);
static void _bind_methods();
+#ifndef DISABLE_DEPRECATED
+ void _set_line_bind_compat_86978(int p_line, const String &p_new_text);
+ void _swap_lines_bind_compat_86978(int p_from_line, int p_to_line);
+ static void _bind_compatibility_methods();
+#endif // DISABLE_DEPRECATED
+
virtual void _update_theme_item_cache() override;
/* Internal API for CodeEdit, pending public API. */ |
Many of the comment changes here are in unrelated areas of code and while appreciated they do complicate the git history, I think it should be limited to areas where you're already changing things to avoid chaos |
6c2e429
to
3f6b104
Compare
3f6b104
to
713460f
Compare
Added compat file and removed comment changes. |
713460f
to
130019b
Compare
I'm insanely impressed and very excited about this. I just want to point out that LineEdit has similar problems around its text selection logic, so we'll need to address that too, down the... line. |
130019b
to
c1bcd2b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's been a while, always enjoy your PRs :)
Haven't had a chance to get fully into the details here, but I like the direction.
Given the range / scope and size of changes here, probably best not to try and do this in one go. Is there any chance you could spilt some of these change up into multiple PR / commits?
Otherwise a few notes on an initial look:
In
set_caret_line()
the p_wrap_index param now accepts -1 to only clamp the column
Have thought about adding a caret_set_pos(line, col)
to get around some of this API awkwardness.
Added
queue_merge_carets()
to be used when merging should happen at the end of the multicaret edit.
If we're trying to simply the interface could we always call merge_overlapping_carets
? We kind of do this for *_move_caret_*
methods, then the API users don't have to care about detecting overlaps? Given the caret ignoring / merging should now be taken care of for them via collapse_carets
(should this be exposed?).
O(1) if the caret index is passed in instead of O(n). I didn't do this for all of them since there would be lots of copy pasted code.
Yeah the only way I was thinking is to create a bunch of internal methods. But I can't see users using that many carets that it becomes an immediate issue.
Removed get_caret_index_edit_order(), because it is no longer needed since carets can be edited in any order.
The intention behind this was to make editing consistent regardless of order the carets where placed down. Plus processing carets twice. I guess due to delaying the merge / ignore carets this becomes a non-issue?
begin/end_multicaret_edit() vs begin/end_complex_operation()
Would we want to force all multicaret edits to be complex operations? Otherwise the undo / redo will not be great. Though you might want a complex op to consist of multiple multicaret edits, so don't think we can combine them entirely.
is_selection_direction_right
I do wonder if there's another way to name this because with right-to-left
, right is not always right.
is_selection_direction_right -> is_caret_after_selection_origin ? |
Rebased to fix conflicts.
|
dceaddd
to
954406f
Compare
The caret is now a part of the selection.
Use a multicaret edit to delay merging overlapping carets until the end.
954406f
to
773a473
Compare
rebased to fix merge conflicts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, can fix any further issues once merged
Really appreciate the effort put into the unit tests as well :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested locally (rebased on top of master
89850d5), it works as expected. (I've tried things such as Ctrl + D, Ctrl + Alt + D, drag-and-drop with multiple selections and so on.)
Great work! Thanks for putting up with the exhaustive review process :) |
CTRL+/
on Windows #83410CodeEdit
functions crashes Godot #73202Selection now goes from selection origin to the caret, so that the caret is always attached to the selection.
All TextEdit APIs now handle multiple carets. This should make it easier to use and use correctly.
If a method is called alone, it will merge overlapping carets. If it is called in a loop for multiple carets as part of a multicaret edit, the merge will happen afterwards to not affect caret indexes.
All text changes and caret changes now
queue_redraw()
.Implementation details
General
remove_line_at()
andinsert_text()
in TextEdit to flesh out the API because methods now affects carets in different ways, so the way it is used and the context matters.set_line()
was often used to insert text, but this doesn't allow for much context of how the carets on that line should be affected.move_lines_up/down()
,delete_lines()
,duplicate_selection()
, anddelete_lines()
fromCodeEditor
toCodeEdit
since the implementations changed so they can be tested, and based on this Add Duplicate Lines shortcut to CodeTextEditor #66553 (review).get_caret_count()
and many other methods now ignores the drag caret if it exists, as it shouldn't be used as a regular caret in most cases.carets
vector at all, but I also didn't want to copy lots of drawing and scrolling code._text_changed()
,_caret_changed()
, and_selection_changed()
to have one clear place after a change that will be called immediately. These are used to redraw, defer emit signals, and cancel drag and drop if needed.selection_changed
signal should be easy to add now, if it is needed in the future._cancel_drag_and_drop_text()
to cancel the Viewport's drag and drop in certain circumstances.Viewport::gui_cancel_drag()
to support this, since I couldn't find an existing way of cancelling a drag and drop operation._unhide_carets()
because sometimes the TextEdit needs to unfold a line, and it can't just use unhide the line._cut_internal()
to use_copy_internal()
then just remove the selection/lines as it is much simpler.set_caret_line()
thep_wrap_index
param now accepts-1
to only clamp the column and not try to adjust it. In many casesset_caret_column()
is called immediately afterward, so calculating the column so it lines up visually was unnecessary, and sometimes needed a workaround to avoid.cancel_ime()
andapply_ime()
for more control over the IME._close_ime_window()
,_update_ime_window_position()
, and_update_ime_text()
to reduce code duplication and increase clarity.get/set_carets_state()
for the variousreload_text()
methods and for getting/setting the entire state.GotoLineDialog::popup_find_line()
to use aCodeTextEditor
instead of aCodeEdit
so it can use the existinggoto_line()
functionality and not need to recreate it.TextEdit Multicaret
begin/end_multicaret_edit()
to be able to allow merging the carets when the edit is complete.queue_merge_carets()
to be used when merging should happen at the end of the multicaret edit.multicaret_edit_ignore_caret()
that should be used when iterating over carets when editing them and text is removed. This will return true when the given caret would have been removed due to it merging with another caret, or it was just added.collapse_carets()
to move all carets in a region of deleted text to one overlapping location and add to the ignore list. These will be merged at the end of the edit unless they are separated.get_caret_index_edit_order()
, because it is no longer needed since carets can be edited in any order.get_sorted_carets()
to replace some functionality fromget_caret_index_edit_order
, but in the opposite order. It sorts by selection_from_line instead of selection_to_line, in ascending order (top of page to bottom).get_sorted_carets()
is const and doesn't cache because it is usually only needed after a caret is moved which would invalidate the cache. I also use a Comparator so it should be faster, O(log(n)) instead of O(n^2). If there are still performance concerns, we can change it to use a cache.get_line_ranges_from_carets()
to get a vector of all the lines that are part of a selection or have a caret on them. This is useful for many common editing tasks._get_affected_lines_from/to()
, but more comprehensive.adjust_carets_after_edit()
because it is no longer needed now that carets are automatically handled._offset_carets_after()
is similar (though not the same) and is used internally for a similar purpose, to move all carets after some text was added/removed.TextEdit::merge_overlapping_carets()
,TextEdit::_new_line()
,TextEdit::_do_backspace()
,TextEdit::_delete()
,TextEdit::insert_text_at_caret()
,TextEdit::add_caret_at_carets()
,TextEdit::adjust_carets_after_edit()
,TextEdit::get_selected_text()
,TextEdit::delete_selection()
,TextEdit::_handle_unicode_input_internal()
,TextEdit::_backspace_internal()
,TextEdit::_cut_internal()
,TextEdit::_copy_internal()
,TextEdit::_paste_internal()
,CodeEdit::_handle_unicode_input_internal()
,CodeEdit::_backspace_internal()
,CodeEdit::do_indent()
,CodeEdit::indent_lines()
,CodeEdit::unindent_lines()
,CodeEdit::_new_line()
,CodeEdit::create_code_region()
,CodeEdit::confirm_code_completion()
,CodeEdit::duplicate_lines()
,TextEditor::_edit_option(EDIT_TOGGLE_FOLD_LINE)
,ScriptTextEditor::_edit_option(EDIT_TOGGLE_FOLD_LINE)
,ScriptTextEditor::_edit_option(DEBUG_TOGGLE_BREAKPOINT)
,CodeTextEditor::convert_case()
,CodeTextEditor::move_lines_up()
,CodeTextEditor::move_lines_down()
,CodeTextEditor::delete_lines()
,CodeTextEditor::duplicate_selection()
,CodeTextEditor::toggle_inline_comment()
,CodeTextEditor::toggle_bookmark()
TextEdit Selection
select()
to now select from a 'selection origin' to the caret. Calling it will move the caret.get_selection_from/to_line/column
still gives the start/end of the selection and can be used like before. Since these values are now calculated and its a common use case, they return the caret line/column if there is no active selection.set_selection_origin_line/column()
to be able to set the origin of the selection directly, similar toset_caret_line/column()
.get_selection_origin_line/column()
. Deprecatedget_selection_line/column()
because the functionality is a bit different, and I wanted a more clear name. It didn't always return the origin of the selection, it was just used to setup the different selection modes._pre_shift_selection()
oradd_caret_at_carets()
.merge_overlapping_carets()
, I used VSCode for reference and that is how it is done there. It is also useful in some scenarios.has_selection()
,get_selected_text()
, anddeselect()
to be faster O(1) if the caret index is passed in instead of O(n). I didn't do this for all of them since there would be lots of copy pasted code.is_selection_direction_right()
to be able to tell if the selection is left to right (top to bottom) or the reverse, since it now always has a direction._post_shift_selection()
as it wasn't needed anymore._click_selection_held()
is no longer true:Warning: is_mouse_button_pressed(MouseButton::LEFT) returns false for double+ clicks...
selection_contains()
,get_selection_at()
, andis_line_col_in_range()
as helper functions to reduce duplicate code.Tests
build_array()
to TestTextEdit and TestCodeEdit to be able to easily create arrays, copied fromvariant/test_array.h
.SEND_GUI_KEY_UP_EVENT
test macro, for some text drag tests where the key up event was needed.insert text
,remove text
,remove line at
// Add undo / redo tests?
line 1368 by adding undo redo tests for each input subcase.lines_edited_args
had to be changed since the order edits are done in are different, but this shouldn't be an issue.// FIXME: Remove after GH-77101 is fixed.
to some input tests, since a new action should not need to be started in between edits with different carets. related Script editor undo sometimes merges unrelated actions #77101.caret index edit order
tests, Addedsort carets
andmerge carets
tests.text manipulation
test case with subcasesbackspace
,new line
,move lines up
,move lines down
,delete lines
,duplicate selection
,duplicate lines
.