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

[Emmanuel Pescosta]

> On Oct. 29, 2012, 8:05 p.m., Frank Reininghaus wrote:
> > Thanks for your analysis, Emmanuel! Good investigation concerning the multiple connections! This might also explain the problems I ran into when I looked into bug 303742.
> > 
> > In any case, I think the cleanest solution is to disconnect the signals in KItemListView::slotRoleEditingCanceled() and KItemListView::slotRoleEditingFinished(). This should fix the problem with the error dialog, right? I see that it can also be done as you suggested in your patch, but disconnecting the signals looks more natural than trying to prevent the emission or the reception of the signal.
> > 
> > Note that bug 294445 is actually about something else though, and it can still be reproduced when your patch is applied.

> This should fix the problem with the error dialog, right?
Yes it should. But I think we should also use the m_editingRole value (Because it is already here ;) to put the roleEditingCanceled() emit in if-clauses. (=> Gives us 100% reliability, that the signal is not emitted multiple times)

> Note that bug 294445 is actually about something else though, and it can still be reproduced when your patch is applied.
The problem comes from KDirLister -> The returned KFileItem is not writable. I will try to fix that in kdelibs ;)

KFileItem KFileItemModel::rootItem() const
{
    return m_dirLister->rootItem();
}


- Emmanuel


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


On Oct. 27, 2012, 12:54 p.m., Emmanuel Pescosta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107070/
> -----------------------------------------------------------
> 
> (Updated Oct. 27, 2012, 12:54 p.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 f2ae375 
>   dolphin/src/kitemviews/private/kitemlistroleeditor.cpp 1e4b5fd 
> 
> 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/20121029/4de1cbb9/attachment.htm>