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

GUI: Do not show tooltip on hovered focused EditText widget #3055

Closed

Conversation

@antoniou79
Copy link
Contributor

@antoniou79 antoniou79 commented Jun 9, 2021

Prevents showing a tooltip for a widget is it is an editable field (editText) and has both mouse hovering above it and the current focus.

This is so that typing text is not interrupted / slowed down by a periodical display of the tooltip, if the mouse is hovering over the same text field that the user is editing.

The bug was mentioned for the Android port on the forums here:
https://forums.scummvm.org/viewtopic.php?p=95531#p95531
However, it is not Android specific, even though the slowdown is a lot more noticeable on an Android device.

antoniou79 added 2 commits Jun 9, 2021
The bool was to quickly update the _lastMousePosition.time always for a TextEdit hovered focused widget

However, this was not the code's effect. It would update the time everytime the period kTooltipDelay for displaying tooltip expired. Which can be done without the extra bool variable.
I order to always update _lastMousePosition.time, we'd need to also always check the widget under the mouse cursor and if it's of textedit type, not just at kTooltipDelay expiration. This would probably introduce additional delay there for little extra benefit.
@criezy
criezy approved these changes Jun 9, 2021
Copy link
Member

@criezy criezy left a comment

This seems to be a good idea to me and the implementations seems OK.
Maybe the two commits should be squashed, but this can be done when the PR gets merged.

@@ -153,6 +153,8 @@ class Widget : public GuiObject {
void lostFocus() { _hasFocus = false; lostFocusWidget(); }
virtual bool wantsFocus() { return false; }

uint32 getType() const {return _type;}

This comment has been minimized.

@bluegr

bluegr Jun 9, 2021
Member

Suggested change
uint32 getType() const {return _type;}
uint32 getType() const { return _type; }
@bluegr
Copy link
Member

@bluegr bluegr commented Jun 9, 2021

Nice work overall! Good idea and straightforward implementation

In order to conform with the rest of the code
@antoniou79
Copy link
Contributor Author

@antoniou79 antoniou79 commented Jun 10, 2021

I realise now that this is an issue even if the hovered widget is different than the focused EditText widget, since the manager will keep displaying periodically the tooltip for that other widget while the user is typing in the EditText.

If it's ok, I'll close this PR and open a new one.

The if clause will be changed to
Widget *focusedWdg = activeDialog->getFocusWidget(); if (focusedWdg && focusedWdg->getType() == kEditTextWidget) {

Thus we would catch the case where there is a focused EditTextWidget at the same time when mouse cursor is hovering over the same or another widget which has a tooltip.

And also the comments will be updated to reflect this new behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants