Review Request 114561: Information Panel: fix race condition (caused by multiple running PreviewJobs)

[Frank Reininghaus]

> On Dec. 24, 2013, 3:13 p.m., Emmanuel Pescosta wrote:
> > dolphin/src/panels/information/informationpanelcontent.cpp, line 167
> > <https://git.reviewboard.kde.org/r/114561/diff/2/?file=226664#file226664line167>
> >
> >     I think that you can remove all "m_previewJob = 0;" lines. The pointer becomes 0 when the object will be destroyed.
> 
> Emmanuel Pescosta wrote:
>     Frank, Merry christmas!

Thanks for your comments! You are right about the "= 0" thing. In the first (unpublished) version of this patch, I did not use a QPointer, so the "= 0" was necessary, but then I thought that we could better use a QPointer for additional safety, just in case the PreviewJob gets deleted from somewhere else.

You are also right about the "m_pendingPreview" issue. I first thought that I'd better leave it as it is to keep the changes in the stable branch as small as possible, but if you also agree that it's not needed any more, I'll remove it.


- Frank


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/114561/#review46138
-----------------------------------------------------------


On Dec. 20, 2013, 7:52 a.m., Frank Reininghaus wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/114561/
> -----------------------------------------------------------
> 
> (Updated Dec. 20, 2013, 7:52 a.m.)
> 
> 
> Review request for Dolphin.
> 
> 
> Bugs: 250787
>     http://bugs.kde.org/show_bug.cgi?id=250787
> 
> 
> Repository: kde-baseapps
> 
> 
> Description
> -------
> 
> In
> 
> InformationPanelContent::showItem(const KFileItem& item) 
> 
> or
> 
> InformationPanelContent::showItems(const KFileItemList& items),
> 
> we either load an icon in the panel, or we start a PreviewJob to get a preview for the current item. However, if a long-running PreviewJob is active already, we load the preview that it creates later on, even though the preview might be for an item that is not hovered any more.
> 
> To fix this, we can keep a pointer to the last preview job, and kill it before a new item is being shown in the panel, or a new preview job is started.
> 
> 
> Diffs
> -----
> 
>   dolphin/src/panels/information/informationpanelcontent.h ed9200a 
>   dolphin/src/panels/information/informationpanelcontent.cpp d63f23b 
> 
> Diff: https://git.reviewboard.kde.org/r/114561/diff/
> 
> 
> Testing
> -------
> 
> I couldn't really test it yet because my development user does not have the PDF thumbnailer (yet), and previews for other files are generated so fast here that it's hard to see the race. But I verified that the panel still works as expected when hovering items.
> 
> 
> Thanks,
> 
> Frank Reininghaus
> 
>

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