Fix connecting a signal with a double click is too difficult #95044

Hilderin · 2024-08-02T00:26:02Z

The main issue was that when showing the tooltip, the editor was losing focus due to the custom tooltip popup. After that, the first mouse click was used to refocus the editor, making a double-click impossible.

Setting FLAG_POPUP to false solved the focus issue but created a new problem where the "normal" tooltip with the empty Control returned by make_custom_tooltip was also displayed. I modified Viewport::_gui_show_tooltip to skip creating the tooltip if the returned control is not visible. This is a bit of a hack; I did not know a better way. It's was not possible in Viewport::_gui_show_tooltip to have a tooltip text but preventing the tooltip to be showned.

Additionally, it created another problem where EditorHelpBitTooltip::_target_gui_input was triggered when the tooltip was shown, even if the mouse did not move. I don't know exactly why this happens. I added a flag to skip the first _target_gui_input.

Finally, without the "real" tooltip, the Viewport tries to create it every few seconds. I added the method EditorHelpBitTooltip::is_tooltip_visible to prevent displaying multiple custom tooltips in ConnectionsDockTree::make_custom_tooltip, and I created an _invisible_tooltip control global to the class to prevent the creation of a new Control every time.

Tested only on Windows 11 on Linux.

That's the result:

godot.windows.editor.dev.x86_64_VI9VD9voL0.mp4

Edit: Tested on Linux
Edit 2: Added #94030 and #96008 to the fixed issue list.
Edit 3: Added #94615 to the fixed issue list.

Hilderin · 2024-08-02T00:33:43Z

This issue that this PR fixes is in the Release Blocker for 4.3, is it normal that it has been put in 4.4 Milestone?

Giganzo · 2024-08-02T08:51:37Z

I wanted to see if this also solved a similar problem with the revert button (you have to click the button two times because of the tooltip)

Screencast_20240725_155139.webm

But that problem is still there. Should I open another issues for that?

Giganzo · 2024-08-02T09:11:30Z

Also with this fix, I don't seem to be able to open documentation when clicking inside the tooltips for signals, instead it highlights the text.

System info: Fedora Linux 40 (KDE Plasma) - Wayland - Vulkan (Forward+) - (software emulation on CPU)

Hilderin · 2024-08-02T11:12:35Z

Thanks for testing @Giganzo

But that problem is still there. Should I open another issues for that?

I'll make the same fix for the tooltip in the Inspector to fix that.

Also with this fix, I don't seem to be able to open documentation when clicking inside the tooltips for signals, instead it highlights the text.

Hum... I feared that could cause issues on other platforms. On Windows, as you can see in my video, click on the tooltip on open documentation works on Windows. I'm setuping my Linux machine to test that right now, I'll check if I can find a work around. Unfortunately, I don't have access to a Mac to test it.

Hilderin · 2024-08-02T13:06:20Z

I debugged it on Ubuntu Wayland and I have the same issue where I cannot click on documentation link. The problem comes from the fact that the mouse up event is not received by the RichTextLabel when the popup does not have focus (due to the FLAG_POPUP at false).

If someone with more experience can help on Wayland, I would appreciate it.

EDITED: I finally found a way to fix the problem.

editor/editor_help.cpp

editor/connections_dialog.cpp

editor/editor_help.cpp

scene/main/viewport.cpp

platform/linuxbsd/x11/display_server_x11.cpp

akien-mga · 2024-08-07T21:54:28Z

This issue that this PR fixes is in the Release Blocker for 4.3, is it normal that it has been put in 4.4 Milestone?

The issue isn't critical, and the fix is not trivial, so at this stage (mere days to stable release) we prefer to be cautious and merge after the 4.3 release. It can then be cherry-picked for 4.3.1 once we've confirmed in the master branch that there are no obvious regressions.

matheusmdx · 2024-08-23T09:46:37Z

This pr also fixes #94030

Giganzo · 2024-08-25T21:33:02Z

This pr looks like it also fixes #94615
There seems to be a difference compared to 4.3.stable on Windows, with this pr on Linux you can trigger tooltips in the inspector outside of the popup.

Hilderin · 2024-08-25T22:47:54Z

Wonderful, thanks for testing!

Parsnip · 2024-09-12T04:20:56Z

Similar issue happens with menu items that have sub menus, such as the Editor Docks or the Editor Layout items in the Editor menu.
Eg, hovering mouse over the Editor Layout item so that the sub menu opens and then moving mouse downwards to other menu items (not the sub menu items), the mouse hover effect isn't displayed for as long as the sub menu remains open.
I have assumed this was a case of the submenu grabbing the focus, and then "releasing" it when it closes.

But to me at least it seems incorrect for the submenu to grab the mouse focus as soon as the menu opens, instead it should only take the mouse focus when mouse moves over the items. Mouse focus should always be where the cursor is, no? But maybe there's a reason it's this way.

To be clear I don't know if the tooltip handling is different from how the editor handles opening its submenus, so this might be an off-topic comment so pardon if that is the case. I was however reminded of this while reading this issue and figured I'd mention it all the same. I also don't know if the behavior is the same on other platforms, I've only used Godot on Windows.

kitbdev · 2024-11-21T21:38:13Z

Needs rebase to fix conflicts.

Seems to fix Inspector tooltips are blinking #99287 for me after I rebase manually.

akien-mga · 2024-12-03T22:22:02Z

The merge conflict seems to have been caused by #98160, which also seemed to aim at fixing jittery tooltips. I'm not sure whether that change should be kept in this PR, or reverted (i.e. keeping the original code from this PR without #98160). CC @Rindbee @KoBeWi

Rindbee · 2024-12-03T23:36:21Z

keeping the original code from this PR without #98160

Doing this will re-open #98135 if p_target->get_viewport() is the root window.

1024 is a hard-coded minimum window width, but in tiled windows the actual width may be smaller than 1024. If the root window is recalculated, the width will first be set to 1024 and then forced to the actual width. This can cause visible jitter on i3wm/sway.

Hilderin · 2024-12-04T00:39:05Z

Doing this will re-open #98135 if p_target->get_viewport() is the root window.

I agree with @Rindbee, I just pushed a fix for the merge conflict including the modification from #98160. It seems to work fine without extensive tests.

dalexeev

Overall looks good to me. 👍 I tested it on Linux and everything seems to work correctly. There is a comment about custom descriptions in the inspector and minor code style suggestions.

dalexeev · 2024-12-09T14:52:36Z

editor/editor_help.cpp

+bool EditorHelpBitTooltip::_is_tooltip_visible = false;
+


I don't know if there is a convention, but I would move this below /// EditorHelpBitTooltip ///.

dalexeev · 2024-12-09T14:53:03Z

editor/editor_help.cpp

@@ -3835,6 +3837,14 @@ void EditorHelpBitTooltip::_safe_queue_free() {
 void EditorHelpBitTooltip::_target_gui_input(const Ref<InputEvent> &p_event) {
 	const Ref<InputEventMouse> mouse_event = p_event;
 	if (mouse_event.is_valid()) {
+		// For some unknown reason, we receive mouse motion of zero when the tooltip
+		// is opened even if the mouse is not moving on Windows now that the toolip
+		// FLAG_POPUP is false.


Suggested change

// FLAG_POPUP is false.

// `FLAG_POPUP` is false.

dalexeev · 2024-12-09T14:53:48Z

editor/editor_help.cpp

-	ERR_FAIL_NULL(p_help_bit);
+Control *EditorHelpBitTooltip::show_tooltip(const String &p_symbol, Control *p_target, const String &p_description) {
+	// Show the custom tooltip only if it is not already visible.
+	// The Viewport will retrigger make_custom_tooltip every few seconds


Suggested change

// The Viewport will retrigger make_custom_tooltip every few seconds

// The viewport will retrigger `make_custom_tooltip()` every few seconds

dalexeev · 2024-12-09T14:55:27Z

editor/editor_help.cpp

+	EditorHelpBit *help_bit = memnew(EditorHelpBit(p_symbol));
+	if (!p_description.is_empty()) {
+		help_bit->set_description(p_description);
+	}


Suggested change

}

}

dalexeev · 2024-12-09T14:59:28Z

editor/editor_help.cpp

+	// When FLAG_POPUP is false, it prevents the editor from losing focus when displaying the tooltip.
+	// This way, clicks and double-clicks are still available outside the tooltip.
+	tooltip->set_flag(Window::FLAG_POPUP, false);


Suggested change

// When FLAG_POPUP is false, it prevents the editor from losing focus when displaying the tooltip.

// This way, clicks and double-clicks are still available outside the tooltip.

tooltip->set_flag(Window::FLAG_POPUP, false);

// When `FLAG_POPUP` is false, it prevents the editor from losing focus when displaying the tooltip.

// This way, clicks and double-clicks are still available outside the tooltip.

tooltip->set_flag(Window::FLAG_POPUP, false);

I would move this to popup_under_cursor() for consistency, since FLAG_NO_FOCUS is set there.

dalexeev · 2024-12-09T15:05:51Z

editor/editor_help.h

 	Timer *timer = nullptr;
 	int _pushing_input = 0;
 	bool _need_free = false;

 	void _start_timer();
 	void _safe_queue_free();
 	void _target_gui_input(const Ref<InputEvent> &p_event);
+	static Control *_make_invisible_control();


I don't know if it's a best practice, but I personally prefer to place static methods before non-static ones.

dalexeev · 2024-12-09T15:13:01Z

editor/editor_inspector.cpp

 	if (has_doc_tooltip || !custom_warning.is_empty()) {
-		EditorHelpBit *help_bit = memnew(EditorHelpBit);
-
+		String symbol;
+		String description;
 		if (has_doc_tooltip) {
-			help_bit->parse_symbol(p_text);
+			symbol = p_text;

 			const EditorInspector *inspector = get_parent_inspector();
 			if (inspector) {
 				const String custom_description = inspector->get_custom_property_description(p_text);
 				if (!custom_description.is_empty()) {
-					help_bit->set_description(custom_description);
+					description = custom_description;
 				}
 			}
 		}

 		if (!custom_warning.is_empty()) {
-			String description = "[b][color=" + get_theme_color(SNAME("warning_color")).to_html(false) + "]" + custom_warning + "[/color][/b]";
-			if (!help_bit->get_description().is_empty()) {
-				description += "\n" + help_bit->get_description();
+			const String custom_warning_description = "[b][color=" + get_theme_color(SNAME("warning_color")).to_html(false) + "]" + custom_warning + "[/color][/b]";
+			if (description.is_empty()) {
+				description = custom_warning_description;
+			} else {
+				description = custom_warning_description + "\n" + description;
 			}
-			help_bit->set_description(description);
 		}


This changes the logic, since parse_symbol() set the description, it is read using get_description(). The previous logic appends/prepends the symbol description, rather than replacing it.

dalexeev · 2024-12-11T09:46:27Z

I didn't notice that copying text in the tooltip and hiding it by pressing Escape stopped working. This is because with the FLAG_POPUP and FLAG_NO_FOCUS flags, the tooltip only receives mouse events. Thus, forwarding keyboard events in _input_from_window() is no longer needed. In #91060, I disabled text selection in tooltips and found a workaround to hide the tooltip with Escape.

Hilderin requested a review from a team as a code owner August 2, 2024 00:26

Calinou added bug topic:editor labels Aug 2, 2024

Calinou modified the milestones: 4.3, 4.4 Aug 2, 2024

Calinou added the cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release label Aug 2, 2024

dalexeev self-requested a review August 2, 2024 06:02

Hilderin force-pushed the fix-double-click-signal-connection branch from f8d4fad to a56efe3 Compare August 2, 2024 12:12

dalexeev reviewed Aug 2, 2024

View reviewed changes

editor/editor_help.cpp Outdated Show resolved Hide resolved

editor/connections_dialog.cpp Outdated Show resolved Hide resolved

editor/editor_help.cpp Show resolved Hide resolved

scene/main/viewport.cpp Outdated Show resolved Hide resolved

Hilderin force-pushed the fix-double-click-signal-connection branch from a56efe3 to d88765a Compare August 2, 2024 14:56

Hilderin requested review from a team as code owners August 2, 2024 14:56

Hilderin force-pushed the fix-double-click-signal-connection branch 3 times, most recently from 94763f1 to fa718ea Compare August 2, 2024 15:37

Hilderin commented Aug 2, 2024

View reviewed changes

platform/linuxbsd/x11/display_server_x11.cpp Show resolved Hide resolved

Hilderin force-pushed the fix-double-click-signal-connection branch from fa718ea to 7289305 Compare August 3, 2024 12:40

Hilderin mentioned this pull request Aug 23, 2024

Fix mouse move over an unfocused window #95972

Merged

Hilderin force-pushed the fix-double-click-signal-connection branch 2 times, most recently from 254f0e1 to f2dfa65 Compare August 23, 2024 02:04

Hilderin mentioned this pull request Aug 25, 2024

Left click not working correctly after scroll and click outside PopupMenu #96008

Open

kitbdev mentioned this pull request Nov 21, 2024

Inspector tooltips are blinking #99287

Closed

deviousdaemon added a commit to deviousdaemon/godot that referenced this pull request Nov 22, 2024

Manual Merge from PR godotengine#95044

4365c0d

Fix connecting a signal with a double click is too difficult

afce73e

Hilderin force-pushed the fix-double-click-signal-connection branch from f2dfa65 to afce73e Compare December 4, 2024 00:36

Hilderin requested a review from a team as a code owner December 4, 2024 00:36

dalexeev approved these changes Dec 9, 2024

View reviewed changes

dalexeev mentioned this pull request Dec 11, 2024

Code Editor: Add documentation tooltips #91060

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix connecting a signal with a double click is too difficult #95044

Fix connecting a signal with a double click is too difficult #95044

Hilderin commented Aug 2, 2024 •

edited by akien-mga

Loading

Hilderin commented Aug 2, 2024

Giganzo commented Aug 2, 2024

Giganzo commented Aug 2, 2024 •

edited

Loading

Hilderin commented Aug 2, 2024

Hilderin commented Aug 2, 2024 •

edited

Loading

akien-mga commented Aug 7, 2024

matheusmdx commented Aug 23, 2024

Giganzo commented Aug 25, 2024

Hilderin commented Aug 25, 2024

Parsnip commented Sep 12, 2024

kitbdev commented Nov 21, 2024

akien-mga commented Dec 3, 2024

Rindbee commented Dec 3, 2024 •

edited

Loading

Hilderin commented Dec 4, 2024

dalexeev left a comment

dalexeev Dec 9, 2024

dalexeev Dec 9, 2024

dalexeev Dec 9, 2024

dalexeev Dec 9, 2024

dalexeev Dec 9, 2024

dalexeev Dec 9, 2024

dalexeev Dec 9, 2024

dalexeev commented Dec 11, 2024

	// The Viewport will retrigger make_custom_tooltip every few seconds
	// The viewport will retrigger `make_custom_tooltip()` every few seconds

Fix connecting a signal with a double click is too difficult #95044

Are you sure you want to change the base?

Fix connecting a signal with a double click is too difficult #95044

Conversation

Hilderin commented Aug 2, 2024 • edited by akien-mga Loading

Hilderin commented Aug 2, 2024

Giganzo commented Aug 2, 2024

Giganzo commented Aug 2, 2024 • edited Loading

Hilderin commented Aug 2, 2024

Hilderin commented Aug 2, 2024 • edited Loading

akien-mga commented Aug 7, 2024

matheusmdx commented Aug 23, 2024

Giganzo commented Aug 25, 2024

Hilderin commented Aug 25, 2024

Parsnip commented Sep 12, 2024

kitbdev commented Nov 21, 2024

akien-mga commented Dec 3, 2024

Rindbee commented Dec 3, 2024 • edited Loading

Hilderin commented Dec 4, 2024

dalexeev left a comment

Choose a reason for hiding this comment

dalexeev Dec 9, 2024

Choose a reason for hiding this comment

dalexeev Dec 9, 2024

Choose a reason for hiding this comment

dalexeev Dec 9, 2024

Choose a reason for hiding this comment

dalexeev Dec 9, 2024

Choose a reason for hiding this comment

dalexeev Dec 9, 2024

Choose a reason for hiding this comment

dalexeev Dec 9, 2024

Choose a reason for hiding this comment

dalexeev Dec 9, 2024

Choose a reason for hiding this comment

dalexeev commented Dec 11, 2024

Hilderin commented Aug 2, 2024 •

edited by akien-mga

Loading

Giganzo commented Aug 2, 2024 •

edited

Loading

Hilderin commented Aug 2, 2024 •

edited

Loading

Rindbee commented Dec 3, 2024 •

edited

Loading