Review Request 120216: Add "open-with" action and right-click context menu to Ark

[Raphael Kubo da Costa]

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


This looks very promising, thank you!

I've added several inline comments, but most of them are just small style nitpicks. In addition to those:
* Are those changes to `archiveview.*` all required or can they be done separately?
* Can you make sure your patch is on top of KDE/4.14 or master? I tried `git apply`ing it here but got a failure in one of the chunks in `part.cpp`.


part/part.h
<https://git.reviewboard.kde.org/r/120216/#comment48111>

    This empty line should be removed.



part/part.h
<https://git.reviewboard.kde.org/r/120216/#comment48101>

    You're missing a space before the '{'.



part/part.h
<https://git.reviewboard.kde.org/r/120216/#comment48102>

    How about `InternalViewer` and `ExternalProgram`?



part/part.h
<https://git.reviewboard.kde.org/r/120216/#comment48103>

    Same thing about the naming.



part/part.h
<https://git.reviewboard.kde.org/r/120216/#comment48104>

    Small style nitpick: there should be no space after the '&' in the first parameter.
    
    As for the second parameter, since all the callers of the method always specify both arguments I don't see the need for setting a default value here.



part/part.cpp
<https://git.reviewboard.kde.org/r/120216/#comment48112>

    This is unrelated, right?



part/part.cpp
<https://git.reviewboard.kde.org/r/120216/#comment48105>

    Please use `QLatin1String("foo")` instead of `QLatin1String( "foo" )` -- those spaces were added when Laurent added QLatin1String calls everywhere with his script years ago.



part/part.cpp
<https://git.reviewboard.kde.org/r/120216/#comment48106>

    Same thing about spaces in the `KIcon()` and `QL1S()` calls.



part/part.cpp
<https://git.reviewboard.kde.org/r/120216/#comment48109>

    Instead of using `m_previewDirList.last()` here (which probably works just fine these days, but could be prone to a race condition if we ever stop deactivating the whole UI while extracting the file to preview it), you could `qobject_cast` `job` to an `ExtractJob` and call `ExtractJob::destinationDirectory()` like we already do in `Part::slotExtractionDone()`.



part/part.cpp
<https://git.reviewboard.kde.org/r/120216/#comment48115>

    You should probably pass `true` to `tempFiles` here so that the previewed file is removed automatically once the program is closed, and to avoid having them added to the recent files list (and it would match the behavior of `ArkViewer).



part/part.cpp
<https://git.reviewboard.kde.org/r/120216/#comment48110>

    This empty line should be removed.



part/part.cpp
<https://git.reviewboard.kde.org/r/120216/#comment48108>

    `QL1S("foo")` instead of `QL1S( "foo" )`.


- Raphael Kubo da Costa


On Sept. 15, 2014, 1:45 p.m., Elvis Angelaccio wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/120216/
> -----------------------------------------------------------
> 
> (Updated Sept. 15, 2014, 1:45 p.m.)
> 
> 
> Review request for KDE Utils and Raphael Kubo da Costa.
> 
> 
> Bugs: 179066
>     http://bugs.kde.org/show_bug.cgi?id=179066
> 
> 
> Repository: ark
> 
> 
> Description
> -------
> 
> This review aims to bring life to this 2 years-old review: https://git.reviewboard.kde.org/r/103690/
> The patch is mostly the same, but the issues pointed by Raphael should now be fixed.
> In particular, the default Ark behavior is unchanged: when an entry is double clicked, the preview is showed (instead of launching the default application of that entry).
> 
> 
> Diffs
> -----
> 
>   part/archiveview.h daac59b1322047f089c6d51024ba36f92e9d2439 
>   part/archiveview.cpp 6b9918df732758eb4146a97c3b7250e1423400e8 
>   part/ark_part.rc 044c11a562d03589314f05e86eb1d68e633ee35e 
>   part/part.h 5379b9fc1aaa4ce451c8b1745ca46ee78630b005 
>   part/part.cpp b4ebcd27c462d2b8037b5ea40c56969eda71bdcb 
> 
> Diff: https://git.reviewboard.kde.org/r/120216/diff/
> 
> 
> Testing
> -------
> 
> Open an existing archive, right-click on an entry, the context menu is showed and and the "Open With..." action is available.
> 
> 
> Thanks,
> 
> Elvis Angelaccio
> 
>

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