Review Request 114228: Fix Bug 328262 - rename bug if you cancel when folder already exists

[Emmanuel Pescosta]

> On Dec. 3, 2013, 3:13 p.m., Frank Reininghaus wrote:
> > 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?
> >

> and updates the URL that is stored in the KFileItem.
Oh I completely missed that :(

> Maybe there is a better solution?
Only connect the "renamingFailed" signal to "slotRenamingFailed" slot if the new name doesn't exist. Better?
if (op && !newNameExistsAlready) {


- Emmanuel


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/114228/#review45036
-----------------------------------------------------------


On Dec. 3, 2013, 1:33 p.m., Emmanuel Pescosta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/114228/
> -----------------------------------------------------------
> 
> (Updated Dec. 3, 2013, 1:33 p.m.)
> 
> 
> Review request for Dolphin and David Faure.
> 
> 
> Bugs: 328262
>     http://bugs.kde.org/show_bug.cgi?id=328262
> 
> 
> Repository: kde-baseapps
> 
> 
> Description
> -------
> 
> 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.
> 
> 
> Diffs
> -----
> 
>   dolphin/src/views/dolphinview.h 86bc5c1 
>   dolphin/src/views/dolphinview.cpp 20bc9f5 
>   lib/konq/konq_operations.h e9255c4 
>   lib/konq/konq_operations.cpp bd96f39 
> 
> Diff: http://git.reviewboard.kde.org/r/114228/diff/
> 
> 
> Testing
> -------
> 
> 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,
> 
> Emmanuel Pescosta
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20131203/3edb5250/attachment.htm>