Review Request: Fix Bug 294445 - Renaming currently viewed folder in Folders sidebar doesn't update main area

[Frank Reininghaus]

> On Oct. 30, 2012, 10:17 p.m., Frank Reininghaus wrote:
> > Thanks for the update! I still believe that disconnecting in the slots which are invoked by the signals is better because it's pretty clear that it only makes sense to receive one of this signals exactly once. The best way to ensure that is to disconnect as soon as one of the slots is invoked. This would also prevent the need for the clearFocus() call in keyPressEvent - to someone who does not know much about this discussion, finishing renaming by a forced call of clearFocus() probably looks like a mysterious hack ;-)
> > 
> > About the real issue - good catch! This was probably not easy to find. However, I'm wondering if it wouldn't be better to make m_model->rootItem() (and hence, KDirLister::rootItem()) return the updated KFileItem. I've just checked it by adding some debug output - it looks like the rootItem that we get is the one for the *old* URL, with the name that the directory had before renaming. I'm wondering if KIO's magic wouldn't be able to handle this properly.
> > 
> > We should ask David once you've pushed a fix for the issue about the annoying dialogs when trying to rename using the Folders Panel.
> 
> Emmanuel Pescosta wrote:
>     > This would also prevent the need for the clearFocus() call in keyPressEvent - to someone who does not know much about this discussion, finishing renaming by a 
>     > forced call of clearFocus() probably looks like a mysterious hack ;-)
>     Oh sorry, I have forgotten to revert this (~~) mysterious hack (~~). I will fix it
>     
>     > However, I'm wondering if it wouldn't be better to make m_model->rootItem() return the updated KFileItem.
>     Okay. Is the m_model->rootItem() the root item of all currently displayed items or the general root item (File system level)?
>     
>     I hope the bug 292934 is also fixed with this patch. I think it is also related to the m_model->rootItem() problem. Could you test it? (I don't have nfs shares)
>     https://bugs.kde.org/show_bug.cgi?id=292934
>

>> This would also prevent the need for the clearFocus() call in keyPressEvent - to someone who does not know much about this discussion, finishing renaming by a 
>> forced call of clearFocus() probably looks like a mysterious hack ;-)
> Oh sorry, I have forgotten to revert this (~~) mysterious hack (~~). I will fix it

All right, you can just push the "disconnects" in the two slots to 4.9 (I can't do it right now, I'm not at home). It would be good if a bug report about the error dialog could be created and closed with the commit to make sure that the issue will be listed in the 4.9.3 changelog:

https://bugs.kde.org/buglist.cgi?bug_status=RESOLVED&bug_status=VERIFIED&bug_status=CLOSED&bugidtype=include&chfield=cf_versionfixedin&chfieldfrom=2011-06-01&chfieldto=Now&chfieldvalue=4.9.3&list_id=266882&query_format=advanced&order=product%2Cbug_id&query_based_on=

>> However, I'm wondering if it wouldn't be better to make m_model->rootItem() return the updated KFileItem.
> Okay. Is the m_model->rootItem() the root item of all currently displayed items or the general root item (File system level)?

According to the KDirLister docs, it should be the URL of the folder which is being listed:

http://api.kde.org/4.x-api/kdelibs-apidocs/kio/html/classKDirLister.html#a4d3775cce77c63b536f5735b26c79b08

And after a rename operation, this seems not to be the case any more.

> I hope the bug 292934 is also fixed with this patch. I think it is also related to the m_model->rootItem() problem. Could you test it? (I don't have nfs shares)
> https://bugs.kde.org/show_bug.cgi?id=292934

Yes, looks like it might be related, but I don't have any NFS shares to test with either :-(


- Frank


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


On Oct. 30, 2012, 11:36 a.m., Emmanuel Pescosta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107070/
> -----------------------------------------------------------
> 
> (Updated Oct. 30, 2012, 11:36 a.m.)
> 
> 
> Review request for Dolphin and Frank Reininghaus.
> 
> 
> Description
> -------
> 
> Fix Bug 294445 - Renaming currently viewed folder in Folders sidebar doesn't update main area
> 
> Especially the error, while renaming a folder in the folderview panel. (Error-dialog appears when you rename the same folder several times)
> 
> 
> Question:
> 
> void KItemListView::editRole(int index, const QByteArray& role)
> {
>     KItemListWidget* widget = m_visibleItems.value(index);
>     if (!widget || m_editingRole) {
>         return;
>     }
> 
>     m_editingRole = true;
>     widget->setEditedRole(role);
> 
>     connect(widget, SIGNAL(roleEditingCanceled(int,QByteArray,QVariant)),
>             this, SLOT(slotRoleEditingCanceled(int,QByteArray,QVariant)));
>     connect(widget, SIGNAL(roleEditingFinished(int,QByteArray,QVariant)),
>             this, SLOT(slotRoleEditingFinished(int,QByteArray,QVariant)));
> }
> 
> Should we disconnect the signals roleEditingCanceled and roleEditingFinished from KItemListWidget* widget, before we connect the signals? When you rename the same folder/file several times, the slots are called multiple times ...
> 
> 1. Rename -> 1 Slot call
> 2. Rename -> 2 Slot calls
> 3. Rename -> 3 Slot calls 
> ...
> 
> Solution:
> 
> widget->disconnect(SIGNAL(roleEditingCanceled(int,QByteArray,QVariant), this);
> widget->disconnect(SIGNAL(roleEditingFinished(int,QByteArray,QVariant), this);
> 
> 
> This addresses bug 294445.
>     http://bugs.kde.org/show_bug.cgi?id=294445
> 
> 
> Diffs
> -----
> 
>   dolphin/src/kitemviews/kitemlistview.cpp 3c60b8e 
>   dolphin/src/kitemviews/private/kitemlistroleeditor.cpp 1e4b5fd 
>   dolphin/src/views/dolphinview.cpp 5b3d074 
> 
> Diff: http://git.reviewboard.kde.org/r/107070/diff/
> 
> 
> Testing
> -------
> 
> Renaming a folder in the folderview-panel works without problems. 
> 
> 
> Thanks,
> 
> Emmanuel Pescosta
> 
>

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