-
-
Notifications
You must be signed in to change notification settings - Fork 20k
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
Editor: Display deprecated/experimental messages in tooltips #89058
Editor: Display deprecated/experimental messages in tooltips #89058
Conversation
0a8c424
to
3aa5161
Compare
I see there's significant refactoring work beyond just adding deprecated/experimental messages. Could you write a few words to outline the rationale for the changes and help with the code review? |
Some refactoring was necessary to avoid code duplication for different types of class members, so the Additionally, I implemented hover and scroll functionality since sometimes the descriptions are too tall to fit on the screen. |
3aa5161
to
6afd006
Compare
The experimental/deprecated message looks like part of the description. It needs to be more separated IMO. |
This matches the current appearance in the main help. Deprecated/experimental messages are simply inserted as paragraphs before the description. The only indicator is an icon and red/yellow bold text. In the future we may add some decorative elements if we deem it necessary. But in my opinion, it’s already quite noticeable. |
This comment was marked as resolved.
This comment was marked as resolved.
editor/connections_dialog.cpp
Outdated
} | ||
|
||
EditorHelpBit *help_bit = memnew(EditorHelpBit(p_text)); | ||
EditorHoverableTooltip *tooltip = memnew(EditorHoverableTooltip(const_cast<ConnectionsDockTree *>(this))); |
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.
You can avoid const_casts
if you store the Control in a variable and then connect the signals in _notification()
.
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 don't think this is possible. In any case you will need to remove the qualifier, since you cannot assign const T *
to T *
.
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.
Then maybe move the const cast to EditorHoverableTooltip()
show_tooltip()
and make the argument const? It would at least make creating tooltips cleaner.
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'd rather keep the const_cast
in calls rather than move it into the definition. Because EditorHelpBitTooltip
requires a non-const pointer for connect()
. In my opinion, it is better not to hide the problem.
// Standard tooltips do not allow you to hover over them. | ||
// This class is intended as a temporary workaround. |
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'd say it's a proper solution, not a workaround. If you want to do more than a standard tooltip, make a more specialized one.
Though it sucks that it basically bypasses the tooltip system (returning empty Controls as custom tooltips is an interesting solution, but you solved a problem that I myself faced with tooltips).
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 think a better solution would be to allow hovering over tooltips (we can make this configurable). In this case, when you hover over a tooltip control, another tooltip may appear. That is, we need a tooltip stack.
This comment was marked as outdated.
This comment was marked as outdated.
editor/editor_help.cpp
Outdated
title = memnew(RichTextLabel); | ||
title->set_fit_content(true); | ||
title->set_selection_enabled(true); | ||
//title->set_context_menu_enabled(true); |
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.
Commented-out code should be removed.
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.
Added TODO comment.
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.
Why would you want the context menu though?
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.
Some people like me are used to copy text with context menu in certain scenarios. I have no idea why I'm not using Ctrl + C here.
6afd006
to
8ed8032
Compare
No, this PR doesn't fix the bug. See #90446 instead.
I'm not sure how to fix this.
This is probably platform specific. On Linux (KDE), the parent window only loses focus if I click on the tooltip.
I fixed this by changing the title background and making the minimum content height about 2 lines:
Made the height of embedded |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
It looks like I managed to solve both problems. |
2bccb90
to
5397ef4
Compare
5397ef4
to
527a1fd
Compare
I actually found another bug .-. |
I tried different flag combinations, but I could not get the keyboard input to be passed to the parent window, and the mouse input to be processed in the popup. If I fix the keyboard issue, then hyperlinks in the popup become unclickable and vice versa. |
Unless you manage to fix it, it's better to have links unclickable than to introduce regression. Scrolling alone is a great improvement already, the link issue could be fixed later. |
I'm not sure if this is a regression. It's somewhat expected that while an interactive popup is on the screen, it will prevent input. Previously, the tooltip was non-interactive, you simply couldn't hover over it.
The problem is that the hyperlinks look interactive, an underline appears on hover, but nothing happens on click. To me this looks like a bigger bug than the problem above.
We could try forwarding keyboard input (except |
But it's not expected that when you have an input field active and focused, you are not able to input text. I discovered this issue by accident, because I was doing something using a build with this PR included; it's confusing and makes editing values troublesome, because you have to be aware of your cursor and avoid hovering the inspector.
That could work. I think |
Edit: maybe its not the cause, that does break keyboard inputs for popups, but not for the Inspector. |
@KoBeWi If you agree with this, let's do it this way. This PR is already too big and to be honest I don't want to mess around with input forwarding, at least right now. |
What way? Like it is now? Or with disabled clicks? |
Options:
|
I'd go with 2. The links were already unclickable before. Now it would be a bit more confusing (since you can hover them), but it's a bug with the new feature, while the other option is a regression. It's better to add new things and iron them out than to break old things. |
527a1fd
to
ae15121
Compare
Fixed by forwarding input to the parent viewport. |
ae15121
to
a714cb9
Compare
Added |
Thanks! |
EditorHelpBit
).