Bug 167788

Created attachment 26512 [details]
workaround/fix

The corruptions seems to occur whenever a row is removed from the table
(QStandardItemModel) while one of the items in the table is set as current
(setCurrentIndex). Looks like a bug in Qt to me. This patch deselects any item
prior to removing a device to prevent corruption from occuring, and also
deselects it after inserting a new device, which removes a minor painting
inconsistency in the same situation.

looks about right for row removal; but perhaps it makes sense to check if m_hoveredIndex is still valid before reseting it?

for rowInsertion, it would be good to reset it to whatever the mouse is over?

Created attachment 26515 [details]
fix corruption, recheck cursor

Well yes, concerning removal, it doesn't need to be updated if there is nothing
selected. I added a check.

And yes, also it would be good to recheck the mouse position and update the
highlight. I did that by calling the mouse event handler when something
changes, but first deselecting the current selection (otherwise it wouldn't get
updated as the row/column is the same but everything is shifted).
I've used the rowsInserted method to do that when adding devices, but I had to
use a signal from the data source to handle the "rowsRemoved" event (as there
is no method to overload).

So now it's all working properly - both the corruption is gone, and also the
highlight is properly updated even if the cursor doesn't move.

Created attachment 26516 [details]
fix corruption, recheck cursor

forgot to shrink some lines into one...

SVN commit 839907 by aseigo:

fix corruption on device plug/unplug while mouse hovered; based on patch by ambro
BUG:167788


 M  +35 -1     notifierview.cpp  
 M  +6 -0      notifierview.h  


WebSVN link: http://websvn.kde.org/?view=rev&revision=839907

Created attachment 26528 [details]
fix

I think what you did there was not correct.

Why did you remove the rowsAboutToBeRemoved? It is what fixes the corruption
this bug is all about. This method is called before the actual removal, so this
is the place where items have to be delesected (I tested it, bug not fixed).

The modelRowsRemoved and rowsInserted methods I added are used only after the
change, and all they do is refresh the highlight.
Also, the disconnect statement should be removed from modelRowsRemoved,
otherwise it would refresh properly on the first removal.

This patch (diff of current svn):
- adds rowsAboutToBeRemoved
- changes rowsInserted to only invalidate the selection if there is one
- changes modelRowsRemoved to never invalidate the selection (it is always
already invalidated in rowsAboutToBeRemoved), and removes the disconnect.
And I tested it, works in all aspects.

not fixed in svn see comment

It's still pretty much broken if used standalone on the desktop instead of the panel... I suggest you wait a little for me to rework things before changing anything.

Created attachment 26535 [details]
fix

This patch fixes all hover-related problems, both in panel and desktop mode.

Correct ordering of operations is crucial; the principle is not to interfere
with QTreeView's internal processing, in practice:
- invalidate the selection even before QTreeView handles rowsAboutToBeRemoved,
and before rows are inserted (it doesn't already have a slot for
rowsAboutToBeInserted)
- post (not invoke handler) a mouse move message after QTreeView handles
rowsInserted/rowsRemoved

I achieved the correct ordering of operations by overloading as many event
handlers as possible; only rowsAboutToBeInserted did not exist, so I created a
normal slot.

Also, to correctly refresh the selection I had to cache the cursor position.
That's becouse if the applet is on the desktop, the "global cursor position" is
actually relative to the black box arround the applet (why??) so I can't just
use QCursor::pos() which gives a true global position.

Please, do not commit something you haven't tested thoroughly; it only causes
confusion. If you think it can be done better, post your suggestions, so I can
at least test it.

i did test it (obviously), including on the desktop, and there was no corruption visible while testing when plugging and unplugging devices.

the reason i changed your patch was quite simple: connect/disconnect of the signal was done in the wrong place, testing seemed to indicate that the rowsAboutToBeRemoved implementation wasn't necessary (rowsAboutToBeRemoved was called, processing happened, then the rowsRemoved slot was called all before getting back to painting).

the new patch is really ugly, but that's the fault of the view, not you. i'm actually surprised that there is painting happening here between rowsAboutToBeRemoved and actual row removal =/


> the "global cursor position" is 
> actually relative to the black box arround the applet

yes, so that all operations in a widget can be done relative to the widget. this makes the common cases much easier; as for using QCursor::pos(), that will fail anyways unless you go through the view which you really don't want to do if at all possible since "the view" is not well defined enough for these kinds of cases due to multiple views being possible.

p.s. plasma-devel is really a better place for these discussions

I cannot reproduce the weird corruption I was seeing anymore; you probably just noticed the highlight-not-refreshed problem. Anyway works perfectly with current svn.