-
-
Notifications
You must be signed in to change notification settings - Fork 19.9k
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
Tooltips: Improve code clarity and docs #43280
Tooltips: Improve code clarity and docs #43280
Conversation
[csharp] | ||
var styleBox = new StyleBoxFlat(); | ||
styleBox.SetBgColor(new Color(1, 1, 0)); | ||
styleBox.SetBorderWidthAll(2); | ||
// We assume here that the `Theme` property has been assigned a custom Theme beforehand. | ||
Theme.SetStyleBox("panel", "TooltipPanel", styleBox); | ||
Theme.SetColor("font_color", "TooltipLabel", new Color(0, 1, 1)); | ||
[/csharp] |
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.
I haven't tested my C# conversion yet. If anyone wants to give it a try and confirm that it works, that'd be welcome.
Note that you have to have a valid Theme
, which can presumably be created with:
Theme = new Theme();
Theme.CopyDefaultTheme();
d8ecc74
to
078a98b
Compare
tooltip = tooltip.strip_edges(); | ||
if (tooltip.length() == 0) { | ||
return; // bye | ||
String tooltip_text = _gui_get_tooltip( |
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.
Very nice clean up on this one, especially the confusing variable names. I wasn't sure when tooltip
refers to the control and when to the string when I was looking at that code. Since you are making changes to that function, I'd also rename the which
variable to something more descriptive. Perhaps tooltip_text_source
? Or tooltip_text_source_node
, since tooltip_text_source_control
would be a bit confusing, haha.
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.
I thought about renaming which
but since it's only used in local code (and a helper method) I didn't feel it was so problematic. But here goes, I renamed it to tooltip_owner
.
scene/main/viewport.cpp
Outdated
Control *base_tooltip = which->make_custom_tooltip(tooltip); | ||
// Controls can implement `make_custom_tooltip` to provide their own tooltip. | ||
// This should be a Control node which will be added as child to a TooltipPanel. | ||
Control *base_tooltip = which->make_custom_tooltip(tooltip_text); |
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.
Perhaps should be named base_tooltip_control
just to follow suit.
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.
I'm not sure this would be really useful as it's only used locally and it's clear from it's type declaration. This would also open the door to renaming make_custom_tooltip
and tons of tooltip-related methods which can either be about tooltip widgets or tooltip texts in the existing API.
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.
Yeah. you are right. Better not go down the rabbit hole.
Changes look good to me all in all. The docs are definitely clearer now and the extra section on theming tooltips is great, I didn't know that. Also, I'd like to bring up my argument from #39677 (comment) and say that if user returns a custom tooltip Control with I feel like there are three main ways we can communicate with the user, in order of accessibility:
If we make the first point of contact, behaviour, confusing by showing unexpected, half-cropped tooltip, we haven't communicated the cause of the problem to the user very well. If we instead show empty tooltip panel, the user might either get a hint that their tooltip is not set to |
078a98b
to
18380c3
Compare
In the |
The return type for `_make_custom_tooltip` is clarified as Control, and users should make sure to return a visible node for proper size calculations. Moreover in the current master branch, a PopupPanel will be added as parent to the provided tooltip to make it a sub-window. Clarifies documentation for `Control._make_custom_tooltip`, and shows how to use the (until now undocumented) "TooltipPanel" and "TooltipLabel" theme types to style tooltips. Fixes godotengine#39677.
18380c3
to
c5d8daf
Compare
Oh, sorry, didn't realize that. That's good then. Perhaps we should replicate that behaviour in 3.2 when backporting this change. |
Cherry-picked for 3.2.4.
I chose not to as this could be considered a breaking change. IMO the fact that I was the one to report this issue even though I don't work on any serious Godot project apart from the engine itself shows that there are probably not too many users affected by this, so the documentation change should be enough for those. |
gui.tooltip_control, | ||
gui.tooltip_control->get_global_transform().xform_inv(gui.last_mouse_pos), | ||
&tooltip_owner); | ||
tooltip_text.strip_edges(); |
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.
I introduced a bug here, this doesn't do anything as it's not assigned.
Fixes godotengine#43940, was a regression from godotengine#43280.
Fixes godotengine#43940, was a regression from godotengine#43280. (cherry picked from commit a4af940)
The return type for
_make_custom_tooltip
is clarified as Control, and usersshould make sure to return a visible node for proper size calculations.
Moreover in the current master branch, a PopupPanel will be added as parent
to the provided tooltip to make it a sub-window.
Clarifies documentation for
Control._make_custom_tooltip
, and shows how touse the (until now undocumented) "TooltipPanel" and "TooltipLabel" theme types
to style tooltips.
Fixes #39677.
Supersedes #43273.
My aim initially was to re-add flexibility to
Control._make_custom_tooltip
in what kind of tooltip popup will be created. In3.2
you can pass anyControl
to be used as is, while inmaster
popups deriveWindow
, and thus the custom tooltipControl
created by the user is wrapped in aTooltipPanel
of typePopupPanel
.I thought some users might want more flexibility here, so I tried to make
TooltipPanel
part of the public API, and make it so it's no longer used by default inViewport
but should be returned by users in_make_custom_tooltip
, if they want it (otherwise they could use anotherWindow
-derived control for a popup window, or even another type ofControl
for some in-game tooltip hints). But sincePopup
/PopupPanel
now inheritWindow
, they're no longerControl
so we have a type mismatch. In the end, I'm not sure the flexibility I was pursuing really corresponds to actual use cases that users have (it came from my own use case in https://github.com/akien-mga/godot-debug-watermark, but I think I can work with styling TooltipPanel as documented in this PR), so I prefer to wait for user feedback before exploring a potentially more flexible API.