Review Request 118269: Fix crash when showing the confirmation dialog for trash/delete operations

[David Faure]

> On June 1, 2014, 3:01 p.m., David Faure wrote:
> > That makes the setWindow call (as called by konq_operations.cpp) useless, though.
> > 
> > This is related to the line in jobuidelegate.cpp which says
> >    QWidget *widget = job() ? window() : NULL; // ### job is NULL here, most of the time, right?
> > 
> > Clearly we need a better way to pass the QWidget* parent to the askDeleteConfirmation() method, it's not working on either end (because no job).
> > 
> > But the API (JobUiDelegateExtension) is in KIOCore, so no QWindow nor QWidget there.
> > 
> > I think we need to use a member variable in KDialogJobUiDelegate in addition to setting it in KJobWidgets.

http://www.davidfaure.fr/2014/kjobwidgets.diff
http://www.davidfaure.fr/2014/kio_jobuidelegate.diff

Seems to work, I get a widget pointer when confirmation dialog pops up.


- David


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


On May 22, 2014, 9:39 p.m., Frank Reininghaus wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118269/
> -----------------------------------------------------------
> 
> (Updated May 22, 2014, 9:39 p.m.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Bugs: 334648
>     https://bugs.kde.org/show_bug.cgi?id=334648
> 
> 
> Repository: kjobwidgets
> 
> 
> Description
> -------
> 
> Currently Dolphin (and probably anything else that can delete files using KIO) crashes when it's supposed to show the confirmation dialog.
> 
> The problem is that KonqOperations::askDeleteConfirmation() sets up a KIO::JobUiDelegate object which has no associated job, and calls its setWindow(QWidget*) method before calling the actual askDeleteConfirmation() function.
> 
> KIO::JobUiDelegate::setWindow(QWidget*) then calls the setWindow function of the base class, KDialogJobUiDelegate::setWindow(QWidget*), which then crashes because it contains the line
> 
> Q_ASSERT(job())
> 
> and there is no job.
> 
> The problem does not exist in KDE SC 4.x - the code went through a big refactoring in
> 
> https://git.reviewboard.kde.org/r/111081/
> 
> which added the assert.
> 
> Just removing the assert won't help because the function then calls
> KJobWidgets::setWindow(KJob *job, QWidget *widget)
> which dereferences the job, i.e., we get a segfault instead.
> 
> This patch removes the assert and wraps the function in an "if (job())" block instead. I'm not entirely sure if that is the correct solution though - any feedback is welcome. Alternatively, one could move the if-check to the child class, i.e., to KIO::JobUiDelegate::setWindow(QWidget*), if this is the only valid use of a KDialogJobUiDelegate without a job.
> 
> Or maybe it does not make much sense at all to have the askDeleteConfirmation function, which is probably always called before any job is set up, in a KDialogJobUiDelegate subclass? Changing that would probably require more intrusive changes though.
> 
> 
> Diffs
> -----
> 
>   src/kdialogjobuidelegate.cpp fb4c99a 
> 
> Diff: https://git.reviewboard.kde.org/r/118269/diff/
> 
> 
> Testing
> -------
> 
> Fixes the crash for me, and the confirmation dialog works as expected (i.e., the user can choose if the file should really be deleted/trashed or not).
> 
> 
> Thanks,
> 
> Frank Reininghaus
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20140601/857d1e70/attachment.html>