Review Request 123967: Change argument of ReadOnlyArchiveInterface::finished() to fix cancellation of password dialogs

[Raphael Kubo da Costa]

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


Before commenting on the smaller details in the code, a few more pressing comments:

* You are checking for `ReadOnlyArchiveInterface::Cancelled` in several parts which only interact with the `Job` layer, which looks like an oversight.
* In certain cases in `cliinterface.cpp` (specifically, `handleLine()`), manually emitting `finished()` in addition to calling `failOperation()` causes crashes. See commit c7af2d6372ea2b222e1af223b3fd53cdeb1f3c2b.
* Have you seen the follow-up I sent to my own email in kde-commits? http://article.gmane.org/gmane.comp.kde.cvs/1420983 -- it contains a suggestion for a different approach that might avoid some of the pitfalls here.

- Raphael Kubo da Costa


On May 31, 2015, 7:51 p.m., Ragnar Thomsen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/123967/
> -----------------------------------------------------------
> 
> (Updated May 31, 2015, 7:51 p.m.)
> 
> 
> Review request for KDE Utils, Elvis Angelaccio and Raphael Kubo da Costa.
> 
> 
> Bugs: 267670
>     http://bugs.kde.org/show_bug.cgi?id=267670
> 
> 
> Repository: ark
> 
> 
> Description
> -------
> 
> Change the argument of the ReadOnlyArchiveInterface::finished() from bool to a new enum with the possible values: Success, Cancelled and ProgramNotFound. Similarly change the argument of the Job::onFinished() slot. This way CliInterface can send a finished() signal with the Cancelled argument. Job handles the finished signal in the OnFinished() slot and sets the KJob::error() to KJob::KilledJobError if argument is "Cancelled". The callers (Part and BatchExtract) are modified to do nothing if the error code is KJob::KilledJobError. This fixes an issue where the user cancels the "Enter password" dialog and the UI keeps being blocked and/or error messageboxes appear.
> 
> The commit ee006cb6a83e6cbd01a2dc459b8b0c8be664c05e in the frameworks branch should be reverted before applying this diff, since this is meant to be an improved replacement of that commit.
> 
> 
> Diffs
> -----
> 
>   app/batchextract.cpp 4adc519 
>   kerfuffle/archiveinterface.h 3126a4c 
>   kerfuffle/cliinterface.cpp 089f36c 
>   kerfuffle/jobs.h 5e177d3 
>   kerfuffle/jobs.cpp 061b75e 
>   part/part.cpp fa1688e 
> 
> Diff: https://git.reviewboard.kde.org/r/123967/diff/
> 
> 
> Testing
> -------
> 
> Opened a password-protected zip archive, clicked on a file and cancelled the dialog. Opened a password-protected zip archive, selected a file for extraction and cancelled the password dialog. Opened a password-protected rar archive, and cancelled the password dialog. In all cases, the UI is not blocked after cancelling the dialog and no message boxes appear.
> 
> 
> Thanks,
> 
> Ragnar Thomsen
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-utils-devel/attachments/20150531/801fe8b1/attachment-0001.html>