Change the path for every item of the subdirectories in a directory rename
ClosedPublic

Authored by jtamate on Dec 16 2018, 8:48 AM.

Details

Summary

As only the items in subdirectories of the renamed one are modified, if all the items are changed the same part of the path with the same new value, the order is preserved, therefore modify the path directly in the list.

BUG: 401552

Test Plan

Can't reproduce the bug.
Passes kdirmodeltest and kdirlistertest (and many others).

Diff Detail

Repository
R241 KIO
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
jtamate created this revision.Dec 16 2018, 8:48 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptDec 16 2018, 8:48 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
jtamate requested review of this revision.Dec 16 2018, 8:48 AM
jtamate edited the summary of this revision. (Show Details)Dec 16 2018, 8:52 AM

Unfortunately it's still crashing for me, even with this patch.

autotests/kdirlistertest.cpp
1299–1308

I was not able to reproduce the crash with the steps you described in https://phabricator.kde.org/D10742#377375.

I can semi-reliably reproduce it by renaming a non-empty folder expanded in the dolphin detail view.

jtamate updated this revision to Diff 47666.Dec 16 2018, 12:36 PM
jtamate edited the summary of this revision. (Show Details)
jtamate edited the test plan for this revision. (Show Details)

I think this time I got the problem right.
One of the classics: I was modifying the list while it was being used.

dfaure requested changes to this revision.Dec 16 2018, 4:28 PM
dfaure added inline comments.
src/core/kcoredirlister.cpp
1580

Why is this modifying lstItems? That's a somewhat costly operation, if it's not needed. Why not a normal readonly iteration?

This revision now requires changes to proceed.Dec 16 2018, 4:28 PM

What happened to the unittest promised by the commit log? ;)

jtamate updated this revision to Diff 47731.Dec 17 2018, 6:52 PM
jtamate marked an inline comment as done.
jtamate retitled this revision from Unit test and fix for bug 401552 to fix for bug 401552.
jtamate edited the summary of this revision. (Show Details)
jtamate edited the test plan for this revision. (Show Details)

There is no need for a unit test, it is already in kdirmodeltest (but perhaps could be expanded in another patch).

I've realized that it only modifies all paths of the subdirectories, and therefore if all the items of a sorted list change the same values, the order is preserved.

jtamate edited the summary of this revision. (Show Details)Dec 17 2018, 8:16 PM
dfaure requested changes to this revision.Dec 17 2018, 8:57 PM

Please change the summary to something more descriptive. A bug number is pretty opaque in itself.

The patch itself looks good.

I also don't understand "kdirmodeltest tests this already". It passes for me, without this change.

This revision now requires changes to proceed.Dec 17 2018, 8:57 PM
jtamate retitled this revision from fix for bug 401552 to Change the path for every item of the subdirectories in a directory rename.Dec 17 2018, 9:07 PM

I'll work on the unit test this weekend. I don't currently have enough free time on weekdays.

jtamate updated this revision to Diff 47995.Dec 22 2018, 8:54 AM

Added the unit test. It crashes for me every time without the patch and none with the patch.

dfaure requested changes to this revision.Dec 22 2018, 11:41 AM
dfaure added inline comments.
autotests/kdirlistertest.cpp
1326

What's drive_c?

1334

This could be done before the previous line, and used there to remove some duplication?

1343

Waiting for 0 signals doesn't really make sense, does it? I bet it works the same without the TRY_

1346

You could do the more compact and more informative

QVERIFY2(job->exec(), qPrintable(job->errorString());

on line 1342, obviously, not here.

This revision now requires changes to proceed.Dec 22 2018, 11:41 AM
jtamate updated this revision to Diff 48004.Dec 22 2018, 1:13 PM
jtamate marked 4 inline comments as done.

Changes requested done and reduced code duplication.

autotests/kdirlistertest.cpp
1326

I started this patch following the structure of .wine/drive_c ...

jtamate updated this revision to Diff 48048.Dec 23 2018, 8:34 AM

With this version, the test with the patch applied is everlasting in:

while [ true ]; do bin/kdirlistertest testRenameDirectory; if [ $? -ne 0 ]; then break; fi; done

and is still crashing without the patch.

dfaure accepted this revision.Dec 23 2018, 1:47 PM
This revision is now accepted and ready to land.Dec 23 2018, 1:47 PM
This revision was automatically updated to reflect the committed changes.