Fixes Bug 304878 - Dolphin shows "ghost" folders in places after autofs umount nfs shares
Review Request #106456 - Created Sept. 16, 2012 and submitted
Fixes Bug 304878 - Dolphin shows "ghost" folders in places after autofs umount nfs shares Patch tested with a usb storage device, thanks to Weng Xuetian. His way to reproduce it: 1. open dolphin 2. plug a usb disk 3. mount a partition 4. unplug without unmount it. 5. ghost entry appears.
Thanks for analysing this annyoing issue and finding out where the mysterious 'ghost' entry finds its way into the PlacesItemModel! I can confirm that your patch fixes the problem, and usually, I like small patches a lot :-) However, your fix will introduce a memory leak - the PlacesItem with the empty text() will not be added to the model, but will not be deleted either. One could delete it in an 'else' branch in PlacesItemModel::appendItemToGroup(PlacesItem* item), of course, but I think I would prefer to not even create the 'ghost' PlacesItem in the first place. I've just had a thorough look at the relevant code (which I'm not really familiar with) to check how a PlacesItem can get an empty text(). In PlacesItemModel::updateBookmarks(), after "if (!found)", the new PlacesItem is constructed, which causes a call of PlacesItem::setBookmark(const KBookmark&). If the bookmark's UDI is not empty, that will call PlacesItem::initializeDevice(const QString& udi), which will then just return if the UDI does not belong to a valid device (which is the case here because the device has been removed). At the moment, I think it would be best to fix it in PlacesItemModel::updateBookmarks(). At the beginning of the "if (!found)" branch, one could add a check to verify if the item would get an non-empty text (i.e., check if the bookmark's UDI is empty || the UDI belongs to a valid device) and create the PlacesItem only if that is the case. A small comment with a reference to the bug report would not hurt. If you have other ideas, just let me know!
Thanks for the quick update! This is good to go to the 4.9 branch (after fixing the two little issues below).
Please add a 'const' here - we always do that for variables which don't change such that you can see at first sight that this is a constant.
Looks good to me, but at the moment (i.e., in KDE 4.9.1), the item could be added with a non-empty text if the UDI is empty [see PlacesItem::setBookmark(const KBookmark& bookmark)]. I'm not sure if this can actually happen, but I would prefer to not risk any regressions and add the item if the UDI is empty or if the non-empty UDI belongs to a valid device. -> if (udi.isEmpty() || !Solid::Device(udi).isValid())