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

Remove editor dependencies from LineEdit #89448

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Mar 13, 2024

@KoBeWi KoBeWi added this to the 4.x milestone Mar 13, 2024
@KoBeWi KoBeWi requested a review from a team as a code owner March 13, 2024 16:36
Comment on lines +258 to +261
#ifdef TOOLS_ENABLED
static inline Object *editor_settings = nullptr;
void _editor_settings_changed(bool p_force_update);
#endif
Copy link
Member

Choose a reason for hiding this comment

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

That still feels quite hacky IMO.

I would go more for having static methods for the caret blink interval and enabled status, which can be set from the editor where relevant.

Like we do for these in editor_node.cpp:

	int swap_cancel_ok = EDITOR_GET("interface/editor/accept_dialog_cancel_ok_buttons");
	if (swap_cancel_ok != 0) { // 0 is auto, set in register_scene based on DisplayServer.
		// Swap on means OK first.
		AcceptDialog::set_swap_cancel_ok(swap_cancel_ok == 2);
	}

	int ed_root_dir = EDITOR_GET("interface/editor/ui_layout_direction");
	Control::set_root_layout_direction(ed_root_dir);
	Window::set_root_layout_direction(ed_root_dir);

It's probably fine if we make these editor settings require a restart to take action if it avoids unncessary complexity in re-setting this state.

Copy link
Member

Choose a reason for hiding this comment

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

We could move this stuff away from editor_node.cpp and centralize it in editor_settings.cpp or a related class that would take care of this Node behavior (re)configuration based on editor settings.

Copy link
Member Author

@KoBeWi KoBeWi Mar 13, 2024

Choose a reason for hiding this comment

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

This property can be configured per instance. How a static method can handle it?
root_layout_direction in Window and Control are static properties inherited by their children and swap_cancel_ok is a global variable.

Copy link
Member

Choose a reason for hiding this comment

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

Well it's configured per instance but in the editor, all instances will be updated whenever the editor settings change, no?

So it can be LineEdit::set_default_caret_interval called from EditorNode and default_caret_interval is used in LineEdit's constructor to set the caret interval (which can still be overridden by calling the existing method to set the interval per instance).

Copy link
Member

@bruvzg bruvzg Mar 13, 2024

Choose a reason for hiding this comment

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

I would probably subclass LineEdit and keep editor specific stuff in the EditorLineEdit, but I'm not sure if it worth it, since it will require a lot of changes to replace all uses of the LineEdit in the editor code.

Also, won't play nice with other controls using it, like SpinBox.

Copy link
Member Author

@KoBeWi KoBeWi Mar 13, 2024

Choose a reason for hiding this comment

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

Ok so I can handle default_caret_interval with -1 meaning "unconfigured", but how to do the same with default_caret_blink_enabled? I'd need some 3rd variable, like configured_from_editor 🤔

Also making it require restart kinda sucks, because the setting is shared by LineEdits and script editor, and for the latter the restart is not required.

would probably subclass LineEdit and keep editor specific stuff in the EditorLineEdit

Well that was my original approach I tried long time ago, but gave up due to how much changes it requires. We did something similar with ColorPicker (#67253), but every picker is configured via a single method and there isn't many of them.

Copy link
Member

Choose a reason for hiding this comment

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

Ok so I can handle default_caret_interval with -1 meaning "unconfigured", but how to do the same with default_caret_blink_enabled? I'd need some 3rd variable, like configured_from_editor 🤔

My idea was not to have an "unconfigured" state, but simply something like this (pseudo-diff):

diff --git a/editor/editor_node.cpp b/editor/editor_node.cpp
index 89e5e3f3b0..b4dcc84b9e 100644
--- a/editor/editor_node.cpp
+++ b/editor/editor_node.cpp
@@ -6320,6 +6320,9 @@ EditorNode::EditorNode() {
 	Control::set_root_layout_direction(ed_root_dir);
 	Window::set_root_layout_direction(ed_root_dir);
 
+	LineEdit::caret_blink_enabled_default = EDITOR_GET("text_editor/appearance/caret/caret_blink");
+	LineEdit::caret_blink_interval_default = EDITOR_GET("text_editor/appearance/caret/caret_blink_interval");
+
 	ResourceLoader::set_abort_on_missing_resources(false);
 	ResourceLoader::set_error_notify_func(&EditorNode::add_io_error);
 	ResourceLoader::set_dependency_error_notify_func(&EditorNode::_dependency_error_report);
diff --git a/scene/gui/line_edit.cpp b/scene/gui/line_edit.cpp
index 1a94d92855..57df534210 100644
--- a/scene/gui/line_edit.cpp
+++ b/scene/gui/line_edit.cpp
@@ -2727,7 +2727,8 @@ LineEdit::LineEdit(const String &p_placeholder) {
 	set_mouse_filter(MOUSE_FILTER_STOP);
 	set_process_unhandled_key_input(true);
 
-	set_caret_blink_enabled(false);
+	set_caret_blink_enabled(caret_blink_enabled_default);
+	set_caret_blink_interval(caret_blink_interval_default);
 
 	set_placeholder(p_placeholder);
 
diff --git a/scene/gui/line_edit.h b/scene/gui/line_edit.h
index 993bc727e4..247411f7c9 100644
--- a/scene/gui/line_edit.h
+++ b/scene/gui/line_edit.h
@@ -170,6 +170,11 @@ private:
 	uint64_t last_dblclk = 0;
 	Vector2 last_dblclk_pos;
 
+	// Those two are overridden from the editor to apply editor settings
+	// to all LineEdit instances in the editor.
+	static bool caret_blink_enabled_default = false;
+	static float caret_blink_interval_default = 0.65;
+
 	bool caret_blink_enabled = false;
 	bool caret_force_displayed = false;
 	bool draw_caret = true;

If it's the editor, it will set a new default value from the editor setting. Otherwise, it will use the hardcoded default value which matches the current defaults. And if it's set by the user in the scene / script, the setter will be called after the constructor and override whatever value was passed.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can do that, but what about the editor restart? As I said, the setting is shared by script editor, which does not require restarting. So we either make the users restart needlessly (for script editor) or expose them to setting that has no immediate effect (for LineEdits).

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.

3 participants