Fix Bug 328262 - rename bug if you cancel when folder already exists
Review Request #114228 - Created Nov. 30, 2013 and submitted
Use the old instead of the new item url to update the item text in the model. Only the item text is temporarily renamed, the url is still the old one until the item was successfully renamed by KIO. So we must use the old url to access the right item.
1. Creat Folder 1 "Bar" 2. Create Folder 2 "Foo" 3. Rename Folder 2 into "Foo" and press Enter 4. Cancel the question 5. Folder 1 is still "Bar" and folder 2 is still "Foo" Works for me.
Thanks for the patch! I haven't been able to test it yet, but have you verified that this does not bring https://bugs.kde.org/show_bug.cgi?id=319119 back? I can't see how both the current and the patched version of the code can have the same effect in the use case pointed out in that bug. Or, in other words: if the current code finds the file for which the renaming failed in the model with the URL 'newUrl' successfully in the use case of bug 319119, then how can it still work if we look for 'oldUrl'?
If you have tested that https://bugs.kde.org/show_bug.cgi?id=319119 is still fixed with this change, feel free to commit - your solution does look correct (and I don't have a good way to test it myself at the moment). I'm still a bit confused though why the existing code in that function is supposed to successfully fix bug 319119, and the patch doesn't change that. If you have some insights into this, please let us know :-)
Review request changed
If the user has cancelled the renaming job, no real renaming was performed and so the item still has the old url in the model. So we use the old url in this special case and the new url in all other cases to update the item text. @David: * Added "int error" to KonqOperations::renamingFailed and hand over the renamingJob->error()
Revision 2 (+14 -7)
Thanks for testing it and for the new patch! The thing that totally confused me was that the item which could not be renamed successfully is stored in the model's hash m_items with the old URL in one case, and with the new URL in the other case. I think that I understand the reason now. The crucial difference is not that the renaming is cancelled by the user in the "new" bug, but something else: 1. If we try to rename "bar" -> "foo", and "foo" already exists, DolphinView::slotRoleEditingFinished(int index, const QByteArray& role, const QVariant& value) notices that the new URL exists already, and skips the code that updates the file name in the model. Therefore, the item is still stored with the old URL in the hash m_items when the "renamingFailed" signal is received. 2. If no item with the new URL exists yet, DolphinView changes the file name in the model by calling the model's setData() method. setData() itself only changes the "text", stored in the QHash 'values' for the item, and updates the URL that is stored in the KFileItem. However, it also starts the "resort all items" timer, and after the delayed resorting, the *new* URL will be stored in the hash m_items. Therefore, I think it might be better to fix this in a different way. Even though your patch works nicely for the two use cases that the "old" and the "new" bug refer to, it could be that a user finds a way to cancel a renaming in a different scenario, and then it will break. I'm not sure if there is a simple and fool-proof way to get it right. I currently see the following possible solution, which is a mix of the current code and v1 of your patch: In DolphinView::slotRenamingFailed(const KUrl& oldUrl, const KUrl& newUrl), first try to find the "oldUrl" in the model, like v1 of your patch does. If the "oldUrl" is not found, i.e., if the index is < 0, we know that the URL has been updated already, and then we try to look up the "newUrl" instead. I think that this approach should be safe in all cases. What do you think? Maybe there is a better solution?