Only follow mouse when moved (Fixes Bug #372635)
ClosedPublic

Authored by leszeklesner on Apr 18 2017, 10:43 AM.

Details

Summary

Use a new variable moved to detect if mouse moved and only change index if the mouse moved. This helps preventing index changes when only using keyboard to search something in milou and to not accidently start/open something you did not want (see bug report https://bugs.kde.org/show_bug.cgi?id=372635 )

BUG: 372635

In general the onEntered signal seems to be broken in Qt somehow as I could not make it work reliably with Qt 5.7.1. Sometimes it worked but mostly the code using onEntered failed to work with onPositionChanged (I guess this also does not always set it to true).
Hence I tried containsMouse which seems to work reliably at least on Qt 5.7.1.
Tests are appreciated especially on other Qt versions.

Diff Detail

Repository
R112 Milou
Lint
Lint Skipped
Unit
Unit Tests Skipped
leszeklesner created this revision.Apr 18 2017, 10:43 AM
Restricted Application added a project: Plasma. · View Herald TranscriptApr 18 2017, 10:43 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript

Copying my reply from RB, to keep this in one place:

Sorry for being dim, but I don't understand how this is meant to work.

In order for __moved to be true in containsMouseChanged:
You're either expecting to get a hoverMoveEvent followed by a hoverEnterEventOr you're expecting to get a hoverEnterEvent twice

Those will only happen if you enter a delegate, leave and enter it again? What else would cause it?

leszeklesner added a comment.EditedApr 18 2017, 11:03 AM

Copying my reply from RB, to keep this in one place:

Sorry for being dim, but I don't understand how this is meant to work.

Basically it should not set the currentIndex to index right after opening up the search results in krunner so it does not jump to the mouse position.

__moved

will be then set to true so that when you move the mouse on an item onContainsMouseChanged is called again and because __moved is now true sets the index accordingly.

Yes, I can see what the code is doing, but I don't see how that means it works.

will be then set to true so that when you move the mouse on an item onContainsMouseChanged is called again and because __moved is now true sets the index accordingly.

When you move a mouse on an item (without leaving and entering again) why would onContainsMouseChanged be called again - unless you exited and left again.

Yes, I can see what the code is doing, but I don't see how that means it works.

will be then set to true so that when you move the mouse on an item onContainsMouseChanged is called again and because __moved is now true sets the index accordingly.

When you move a mouse on an item (without leaving and entering again) why would onContainsMouseChanged be called again - unless you exited and left again.

You need to hover over another item in the result list thats true. Otherwise it would not be called again and thus not highlight anything.

Yes, and you have this boolean separately for each delegate.

So I can see how this supresses that initial selection in your bug report; but surely it will also supress every other normal case after that.

When we move from one delegate to another the first time from the point of view of the delegate we're moving into:
you get an onContainsMouseChanged - _moved is falsed. We set it to true, but otherwise do nothing
we get a onPositionChanged - we set _moved to true

We're not going to get another onContainsMouseChanged unless there's a bug in Qt.

Yes, and you have this boolean separately for each delegate.

So I can see how this supresses that initial selection in your bug report; but surely it will also supress every other normal case after that.

When we move from one delegate to another the first time from the point of view of the delegate we're moving into:

you get an onContainsMouseChanged -  _moved is falsed. We set it to true, but otherwise do nothing
we get a onPositionChanged - we set _moved to true

We're not going to get another onContainsMouseChanged unless there's a bug in Qt.

Yeah you are right. Upon testing I noticed that I need to hover twice over an item to actually highlight it.
Hmm... so we need to place the _moved outside the delegate that should do it

You can do this:

property bool moved: false
onMovedChanged: if (moved) {
    listView.currentIndex = index
}
onPositionChanged: {
...
moved = true;
}
leszeklesner edited the summary of this revision. (Show Details)

You can do this:

property bool moved: false
onMovedChanged: if (moved) {
    listView.currentIndex = index
}
onPositionChanged: {
...
moved = true;
}

I tried this, but onPositionChanged doesn't fire for me at all in qt 4.8.0.. Is this a bug?

Is it possible to attach the property __moved to the parent ListView dynamically, so it can be checked by all delegates?

You can do this:

property bool moved: false
onMovedChanged: if (moved) {
    listView.currentIndex = index
}
onPositionChanged: {
...
moved = true;
}

I tried this, but onPositionChanged doesn't fire for me at all in qt 4.8.0.. Is this a bug?

Is it possible to attach the property __moved to the parent ListView dynamically, so it can be checked by all delegates?

See my updated patch. It moves the moved property to listView and works much better than the previous patch.

While you fix the issue with the code, please add a line

BUG: 372635

in the summary instead of the other reference to the bug; this should send a notification and close the bug when the review is accepted.

leszeklesner edited the summary of this revision. (Show Details)Apr 18 2017, 8:59 PM

This works for me, thanks!

If I have the mouse outside the window, and then deliberately move the mouse in to select something, by design, this won't update the index the first time.

It'll fix your bug, sure - but we're knowingly introducing a different bug.

What we need to do to fix the bug without regressions is separate the two events:

    • the window expanding and us now containing the mouse
  • the user actively moving the mouse into view.
leszeklesner added a comment.EditedApr 20 2017, 2:29 PM

If I have the mouse outside the window, and then deliberately move the mouse in to select something, by design, this won't update the index the first time.

It'll fix your bug, sure - but we're knowingly introducing a different bug.

Yeah I see the issue.

What we need to do to fix the bug without regressions is separate the two events:

    • the window expanding and us now containing the mouse
  • the user actively moving the mouse into view.

There is no way to distinguish between the two with the normal Qt offerings of containsMouse or Entered signals as both are recognized as mouse moving into the view.
So it needs some other way to distinguish those events (indirectly maybe with onHeight changes). I will try figuring out something to address the introduced issue.

what about set moved variable according to global mousre position instead? I know this is possible with QCursor, but not sure if it's possible at qml side, though. I haven't looked at the code yet, but if there's c++ component, we can add a qobject derived class calling QCursor::pos(), and expose it to qml.

what about set moved variable according to global mousre position instead? I know this is possible with QCursor, but not sure if it's possible at qml side, though. I haven't looked at the code yet, but if there's c++ component, we can add a qobject derived class calling QCursor::pos(), and expose it to qml.

Not sure what you mean by global mouse position and how it should solve the issue though.
The problem is to distinguish between if the cursor was deliberately moved into the ListView or if the entries of the ListView cause the height to grow in such way that the mouse cursor hovers over it.
The more I think about it and the workarounds (like I said trying to watch for onHeightChanges) the more I have the view that this is not possible as onHeight changes also more than one time and might not include the cursor at first but later on it does (which would then highlight the entry under the cursor again which is not wanted).
The best solution for all of this would be if Qt would be able just to hide the cursor during typing. But that seems not to work.

Sorry I should have been more specific.

The difference between these two situation is whether the mouse position in global coordinate changed.

Inside onContainsMouseChanged, if containsMouse

  • If moved, select current index
  • Else if global mouse pos changed, set moved = true and select current index
  • Else, do nothing

Inside onPositionChanged,

  • If !moved and global mouse pos changed, set moved = true and select current index

The initial global mouse pos could be recorded at the same time as initializing moved = false.

This covers the following situation:

  • Initially mouse is outside the window, it becomes inside the window because of the height change.
    • Triggers onContainsMouseChanged, as the global mouse pos didn't change, it does nothing
    • Not sure if this triggers onPositionChanged, but it also does nothing because the check of global mouse pos
  • Initially mouse is outside the window, it becomes inside the window because the user moved it.
    • This triggers both onPositionChanged and onContainsMouseChanged. The global mouse pos changed, current index selected.
  • After mouse becomes inside the window because of height change, and is on some list item A, the user moves mouse within list item A
    • Triggers onPositionChanged. The global mouse pos changed, current index selected.

For reason I don't understand, onPositionChanged doesn't fire for me even with hoverEnable set to true. So the last point won't work. But otherwise it works well.

The patch implements what I mentioned.

Updated to solution by qi437103 which implements a mousehelper to get the global mouse position so we can distinguish between mouse being inside the resultslistview or outside

Cool, seems like a solid approach.

Just nitpicks left.

lib/mousehelper.h
41–42 ↗(On Diff #13771)

You don't need these to be invokable if you're exposing them as a property.

Right now you're exporting two things with the same name, the value of a property and a function.

lib/qml/qmlplugins.cpp
41 ↗(On Diff #13771)

May as well register a singleton, it doesn't have a state.

Instead of having one object created per delegate, we just one have object.

qi437103 updated this revision to Diff 13852.Apr 27 2017, 12:36 AM

Address comments. Seems I can update the diff directly.

broulik edited edge metadata.EditedApr 27 2017, 11:56 AM

That's actually pretty cool :) Not sure how QCursor::pos() behaves on Wayland if the mouse is outside the window but should still be fine I guess

lib/qml/ResultsView.qml
59

Since it doesn't have a NOTIFY signal won't QML complain about "property depends on non-NOTIFYable property"?

qi437103 updated this revision to Diff 13900.Apr 28 2017, 3:59 AM

Get rid of "property depends on non-NOTIFYable property" warning, as well as unneeded code.

davidedmundson accepted this revision.Apr 28 2017, 11:02 AM
This revision is now accepted and ready to land.Apr 28 2017, 11:02 AM
This revision was automatically updated to reflect the committed changes.