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
Details
Details
- Reviewers
dfaure - Group Reviewers
Frameworks - Commits
- R241:211c3a680ea7: Change the path for every item of the subdirectories in a directory rename
Can't reproduce the bug.
Passes kdirmodeltest and kdirlistertest (and many others).
Diff Detail
Diff Detail
- Repository
- R241 KIO
- Lint
Lint Skipped - Unit
Unit Tests Skipped
| autotests/kdirlistertest.cpp | ||
|---|---|---|
| 1299–1308 ↗ | (On Diff #47655) | 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. |
Comment Actions
I think this time I got the problem right.
One of the classics: I was modifying the list while it was being used.
| src/core/kcoredirlister.cpp | ||
|---|---|---|
| 1580 ↗ | (On Diff #47666) | Why is this modifying lstItems? That's a somewhat costly operation, if it's not needed. Why not a normal readonly iteration? |
Comment Actions
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.
Comment Actions
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.
Comment Actions
I'll work on the unit test this weekend. I don't currently have enough free time on weekdays.
Comment Actions
Added the unit test. It crashes for me every time without the patch and none with the patch.
| autotests/kdirlistertest.cpp | ||
|---|---|---|
| 1326 ↗ | (On Diff #47995) | What's drive_c? |
| 1334 ↗ | (On Diff #47995) | This could be done before the previous line, and used there to remove some duplication? |
| 1343 ↗ | (On Diff #47995) | Waiting for 0 signals doesn't really make sense, does it? I bet it works the same without the TRY_ |
| 1346 ↗ | (On Diff #47995) | You could do the more compact and more informative QVERIFY2(job->exec(), qPrintable(job->errorString()); on line 1342, obviously, not here. |
Comment Actions
Changes requested done and reduced code duplication.
| autotests/kdirlistertest.cpp | ||
|---|---|---|
| 1326 ↗ | (On Diff #47995) | I started this patch following the structure of .wine/drive_c ... |
Comment Actions
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.