Fix new file creation leading to dupe items on a fresh view

Authored by hein on Jan 11 2019, 12:45 PM.



This was a regression caused by the code attempting to insert new items
at drop position, if available. setSortMode was being called in a slot
connected to the dir model's rowsInserted, but the Positioner has to be
initialized earlier as a proxy needs to handle
sourceRowsAboutToBeInserted as well.

Thanks to an investigation and patch by Oleg Solovyov in D17689 for
helping to get to the bottom of this.

This is aimed at 5.12+.


Diff Detail

R119 Plasma Desktop
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.
hein created this revision.Jan 11 2019, 12:45 PM
Restricted Application added a project: Plasma. · View Herald TranscriptJan 11 2019, 12:45 PM
hein requested review of this revision.Jan 11 2019, 12:45 PM
hein updated this revision to Diff 49233.Jan 11 2019, 12:46 PM
hein edited the summary of this revision. (Show Details)

Add BUG to message.

hein updated this revision to Diff 49234.Jan 11 2019, 12:48 PM

Forgot a hunk when moving to 5.12 branch.

McPain accepted this revision.Jan 14 2019, 9:09 AM

It works, thanks.
Removed D17689 and D17707 patches so I close then

This revision is now accepted and ready to land.Jan 14 2019, 9:09 AM
fvogt added a subscriber: fvogt.Jan 15 2019, 8:50 AM
fvogt added inline comments.

The comment refers to the first connect now, but is mostly aimed at the second. Maybe move it and add another one for the added connect call?


It doesn't use Qt::QueuedConnection explicitly - is the comment wrong or is it done implicitly?

hein updated this revision to Diff 49500.Jan 15 2019, 8:58 AM

Fix comments and make code reflect comment intent

hein requested review of this revision.Jan 15 2019, 8:59 AM

Requesting re-review due to significant behavior changes following @fvogt's comment

McPain added inline comments.Jan 15 2019, 1:37 PM

According to e94888701 and cc9c3d32a, the comment is wrong.

McPain requested changes to this revision.Jan 23 2019, 2:38 PM

That's what I got with this patch.

Please remove Qt::QueuedConnection from both connections.
Additionally, remove the references from comments

This revision now requires changes to proceed.Jan 23 2019, 2:38 PM
hein added a comment.Jan 24 2019, 8:31 AM

The QueuedConnection is fine, though. Why remove it?

davidedmundson added inline comments.

I'm a bit worried about queuing something with indexes. Indexes are only valid at that exact moment.

If the source model does the following:
then exits back to the event loop

When you process the delayed connection for the first insertion the old first/last could be pointing anywhere.

We could maybe get round this by converting to QPeristentModelIndexes immediately, then queue up the operation that uses them.

hein updated this revision to Diff 50227.Jan 25 2019, 10:20 AM

Don't queue the connections.

Somebody review this for me, please, I'm too far away from actually testing it

You should resign as reviewer then (it's under the Add Action... menu button).

davidedmundson accepted this revision.Feb 1 2019, 10:11 AM
davidedmundson added inline comments.

Also I don't think I understand this:

  • Delay this via queued connection, such that the row is available and can be mapped

at the point of rowsInserted() the row should be available, If not we have a source model bug.

This revision was not accepted when it landed; it landed in state Needs Review.Feb 1 2019, 11:14 AM
This revision was automatically updated to reflect the committed changes.
McPain added inline comments.Feb 1 2019, 11:17 AM

like I said, in the Qt::QueuedConnection was removed from connection, but the comment was unchanged