[KCoreDirLister] Remove assert in reinsert method
Needs ReviewPublic

Authored by chinmoyr on May 13 2019, 6:47 PM.

Details

Reviewers
dfaure
pino
elvisangelaccio
apol
Group Reviewers
Frameworks
Summary

Here I am trying to fix #405461. We reach this assertion in reinsert() after we create a new folder
in kfilewidget. The code path is triggered by the signal KDirWatch::created which is then followed
by itemsAddedInDirectory(), updateDirectory(), processPendingUpdates() and then reinsert().

Now there are two issues with reinsert(). First it asserts that DirItem::lstItems (list of KItems)
must contain a KItem whose url is lexicographically greater than the oldUrl we are trying to update.
But when creating folders from kfilewidget it is almost always not true. In fact after creating a
new folder it is empty.

Other issue is with the behaviour of reinsert(). In case the exact KItem for oldUrl doesn't exist,
it replaces the first greater KItem with the new one. From this method's description and from what I
have observed so far this behaviours is incorrect. So removing the assertion seems to be an appropriate
way to fix the bug.

PS: Following the code has been difficult so I might have missed a place where
placing an extra updateDirectory() call would have fixed the issue or I might be talking nonsense here.
Please correct me if this is the case.

BUG: 405461

Diff Detail

Repository
R241 KIO
Branch
kcoredirlister
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 11804
Build 11822: arc lint + arc unit
chinmoyr created this revision.May 13 2019, 6:47 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptMay 13 2019, 6:47 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
chinmoyr requested review of this revision.May 13 2019, 6:47 PM

Can you look into adding a unittest for the case that hits the assert? As you say, this code isn't easy... but the unittest is quite comprehensive (although never enough, as proven by this).

@chinmoyr D23875 fixes the crash, I suggest to abandon this change request. We can discuss it again if someone manages to hit the assert again, in a different scenario :-)