Review Request: Added support for LZO archives

[Raphael Kubo da Costa]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/103100/#review8294
-----------------------------------------------------------


The patch looks mostly right, but there are things below which you need to fix -- and please use QLatin1Char and QLatin1String with strings and chars.

As for lzop itself, from your comments and from the patch it does not look very flexible indeed. Have you considered using the libraries themselves?


plugins/clilzoplugin/cliplugin.h
<http://git.reviewboard.kde.org/r/103100/#comment7073>

    The copyright is yours.



plugins/clilzoplugin/cliplugin.h
<http://git.reviewboard.kde.org/r/103100/#comment7076>

    Minor nitpick: we usually add a space before the ":" here.



plugins/clilzoplugin/cliplugin.h
<http://git.reviewboard.kde.org/r/103100/#comment7074>

    Redundant blank line.



plugins/clilzoplugin/cliplugin.cpp
<http://git.reviewboard.kde.org/r/103100/#comment7075>

    Ditto about the copyright.



plugins/clilzoplugin/cliplugin.cpp
<http://git.reviewboard.kde.org/r/103100/#comment7077>

    Redundant empty line.



plugins/clilzoplugin/cliplugin.cpp
<http://git.reviewboard.kde.org/r/103100/#comment7079>

    Wrong indentation. Plus, I'm not sure CliInterface can handle the case of only DeleteProgram not being defined. Have you tried to delete a file from an lzo archive in Ark?



plugins/clilzoplugin/cliplugin.cpp
<http://git.reviewboard.kde.org/r/103100/#comment7080>

    lzop's man page lists some examples of how to extract single files. Do they not work?



plugins/clilzoplugin/cliplugin.cpp
<http://git.reviewboard.kde.org/r/103100/#comment7081>

    What will happen if one tries to do that in Ark then?



plugins/clilzoplugin/cliplugin.cpp
<http://git.reviewboard.kde.org/r/103100/#comment7082>

    I guess you mean p[FileExistsInput] and p[FileExistsExpression]?



plugins/clilzoplugin/cliplugin.cpp
<http://git.reviewboard.kde.org/r/103100/#comment7083>

    Does it mean lzop will always overwrite files when extracting or will always skip them?



plugins/clilzoplugin/cliplugin.cpp
<http://git.reviewboard.kde.org/r/103100/#comment7078>

    Redundant empty line.



plugins/clilzoplugin/cliplugin.cpp
<http://git.reviewboard.kde.org/r/103100/#comment7088>

    Will there be problems when the footer is not printed (it wasn't when I listed an archive with a single file)?



plugins/clilzoplugin/cliplugin.cpp
<http://git.reviewboard.kde.org/r/103100/#comment7085>

    Wrong indentation.



plugins/clilzoplugin/cliplugin.cpp
<http://git.reviewboard.kde.org/r/103100/#comment7087>

    Splitting and then grabbing the last element looks easier to read.



plugins/clilzoplugin/cliplugin.cpp
<http://git.reviewboard.kde.org/r/103100/#comment7086>

    Not needed.



plugins/clilzoplugin/cliplugin.cpp
<http://git.reviewboard.kde.org/r/103100/#comment7084>

    Don't think this is needed.



plugins/clilzoplugin/kerfuffle_clilzo.desktop
<http://git.reviewboard.kde.org/r/103100/#comment7089>

    You shouldn't add the translations yourself, the translators do that.



plugins/clilzoplugin/kerfuffle_clilzo.desktop
<http://git.reviewboard.kde.org/r/103100/#comment7090>

    Can you rebase on top of master and set the mimetypes the way the other plugins are currently doing?


- Raphael Kubo da Costa


On Nov. 9, 2011, 10:38 p.m., Othmane Moustaouda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/103100/
> -----------------------------------------------------------
> 
> (Updated Nov. 9, 2011, 10:38 p.m.)
> 
> 
> Review request for KDE Utils and Raphael Kubo da Costa.
> 
> 
> Description
> -------
> 
> Added support for LZO archives using lzop utility
> 
> 
> This addresses bug 211669.
>     http://bugs.kde.org/show_bug.cgi?id=211669
> 
> 
> Diffs
> -----
> 
>   plugins/CMakeLists.txt b1c3c03f66341e20e16132070248bb715e5b1d30 
>   plugins/clilzoplugin/CMakeLists.txt PRE-CREATION 
>   plugins/clilzoplugin/cliplugin.h PRE-CREATION 
>   plugins/clilzoplugin/cliplugin.cpp PRE-CREATION 
>   plugins/clilzoplugin/kerfuffle_clilzo.desktop PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/103100/diff/diff
> 
> 
> Testing
> -------
> 
> Listing and adding files to a new archive are working. There are some problems with deleting (lzop does not support it) and
> with adding files to an existing archive.
> 
> lzop does not fit perfectly with CLI interface, now I'm looking at how to change something to make it more elastic.
> 
> 
> Thanks,
> 
> Othmane Moustaouda
> 
>

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