STORM-1713: Mouse pointer flickers when hovering over any active/clickable UI item


Submitter:	Ansariel Hiller	Reviewers
Branch:		Groups:	viewer
Bugs:	STORM-1713	People:	richard.linden
Change Number:	None	Repository:	viewer-development

Description:

The fix for the flickering mouse cursor consists of 2 parts:

The first part changes LLView::childrenHandleHover() so that the cursor is only set if the control itself is blocking the mouse event and not at every hierarchy level in the control hierarchy. If the cursor would be set at each level, it would cause a flicker in case the different controls set a different cursor. This change actually resembles the algorithm used prior the start of the flickering.

The second part changes LLToolTip::handleHover() and specifically handles flickering of the cursor while hovering over the "i"-button of a hovertip. The general call to LLPanel::handleHover() was changed to be only called if the tooltip itself does not set the cursor itself. Logging the calls to LLWindowWin32::setCursor() revealed that if the "i"-button on a hovertop is hovered with the cursor said method is called twice with different cursors alternatively. Checking the call stack further revealed that one call is coming from LLToolTip::handleHover() and the other one from LLButton::handleHover(). Latter gets invoked if LLPanel::handleHover() is called. Since nothing is really done here except setting the cursor to UI_CURSOR_ARROW only ti get then set to UI_CURSOR_HAND if LLPanel::handleHover() returns, it doesn't make sense to do invoke that method unless the cursor is not changed in the tooltip itself. So LLPanel::handleHover() is only invoked if the tooltip does not set the cursor itself, so that child controls should take care.

Testing Done:

Testing was done by myself on Firestorm rev. 24073 (http://hg.phoenixviewer.com/phoenix-firestorm-lgpl/rev/bcc95de39ca9) on Windows 7 and Lance Corrimal on Dolphin Viewer. Apparently the fix works without any side-effects

Expand All

tested the patch in dolphin viewer 3.2.0.22107. works as expected, no negative sideeffects that I can see.

Lance Corrimal Nov. 23, 2011, 11:34 a.m. (Nov. 23, 2011, 11:34 a.m.)
```
sidenote, testing happened exclusively on linux.
```

Reviewers (People)
- added richard.linden

Description:

Richard - this has passed QA (it works), but I'd like your review please

Please do not associate the ability to set the mouse cursor with the fact that a view is mouse_opaque.  Mouse_opaque, a.k.a. blockMouseEvent is used as a shortcut that could easily be replaced by some complicated logic that always returns true.  It does not indicate the desire to update the mouse cursor.  The rule we use for mutating side effects (i.e. setting tooltip, changing mouse cursor, etc.) is that by default the leafmost view wins.  In this case, the proper fix is to store the current mouse cursor in your LLWindow* implementation and then only set it once a frame in LLWindow*::gatherInput()

Ansariel Hiller Dec. 5, 2011, 11:35 a.m. (Dec. 5, 2011, 11:35 a.m.)

Are you actually referring to the first part, the second or both? I'm asking because the first part actually reverts LLView::childrenHandleHover() to what it was before flickering started.

indra/llui/lltooltip.cpp (Diff revision 1)

let's keep the old code and make the cursor something that is set many times, but only propagated to the OS once per frame, like in gatherInput() or LLViewerWindow::draw();

indra/llui/llview.cpp (Diff revision 1)

I know this is reverting to older behavior, but the older behavior is wrong in tying the ability to set cursors to the mouse_opaque attribute.

indra/llui/lltooltip.cpp (Diff revision 1)

I suppose gatherInput() is executed each frame or at least regularly (saw some Windows message handling in there)? Appears for me the better place to handle the cursor update - LLViewerWindow::draw() is IMHO too high-level to actually invoke updating of the cursor - remember the cursor is set in LLUI. So the viewer classes would have to take care a low-level tier is properly doing stuff set in a mid-level tier - this doesn't sound reasonable for me.

Ansariel Hiller Dec. 5, 2011, 2:08 p.m. (Dec. 5, 2011, 2:08 p.m.)

Hmmm... Somehow I have a feeling that comment wasn't linked properly.

Richard Nelson Dec. 5, 2011, 2:49 p.m. (Dec. 5, 2011, 2:49 p.m.)

gatherinput() should be called per frame on all platforms.  It's the equivalent of the idle loop for our platform abstraction.  I agree that putting the call in LLViewerWindow::draw() is a bad idea, that was just an example that came to mind of a per-frame update where it *could* live.

Diff
- added Diff r2

Description:

Updated patch based on Richard's review. LLWindow::setCursor() will now set a new member variable mNextCursor instead of directly changing the cursor. The former LLWindow::setCursor() method and its implementations have been renamed to updateCursor(). The method is invoked at the end of LLWindow::gatherInput().

The patch was tested on today's tip in viewer-development (http://hg.secondlife.com/viewer-development/changeset/a984f7ffeb4b) and during my tests on Windows 7 it seemed to work fine. However, I cannot test Linux and Mac.

I approve, thanks!  I'm guessing we will end up testing this on all 3 platforms during the merge process, so hopefully all goes well.

I'm finding that in a viewer with the r2 version of this patch the mouse pointer flickers through at least three different states in rapid succeession when you hover it over your opened inventory and your inventory is still loading.
I'm pretty sure it didn't do that with the previous version.

Lance Corrimal Dec. 8, 2011, 1:17 p.m. (Dec. 8, 2011, 1:17 p.m.)

on a viewer with the older patch the mouse pointer flickers too in this situation, but by far not as pronounced.

status changed from pending to submitted

You have a pending review.

Other reviews

Your comment

Review Board 1.6.11

This change has been marked as submitted.