Review Request: Implement lha/lzh support in ark - Bug 231896

Sun Jul 17 19:26:30 CEST 2011

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://svn.reviewboard.kde.org/r/5761/#review10307
-----------------------------------------------------------

Thanks for the patch, I hope you are still there :)

The patch looks fine overall, but the coding style is very inconsistent. In Ark we follow kdelibs' coding style, it might be good for you to take a look at it [1]. And please make sure there is no trailing whitespace in the code.

[1] http://techbase.kde.org/Policies/Kdelibs_Coding_Style

/trunk/KDE/kdeutils/ark/plugins/clilhaplugin/cliplugin.h
<http://svn.reviewboard.kde.org/r/5761/#comment11990>

    You can have yourself as the sole copyright owner of this file.

/trunk/KDE/kdeutils/ark/plugins/clilhaplugin/cliplugin.h
<http://svn.reviewboard.kde.org/r/5761/#comment11988>

    Extra empty line.

/trunk/KDE/kdeutils/ark/plugins/clilhaplugin/cliplugin.h
<http://svn.reviewboard.kde.org/r/5761/#comment11989>

    Ditto.

/trunk/KDE/kdeutils/ark/plugins/clilhaplugin/cliplugin.h
<http://svn.reviewboard.kde.org/r/5761/#comment11992>

    This could be private.

    Moreover, doesn't ParserState make more sense?

/trunk/KDE/kdeutils/ark/plugins/clilhaplugin/cliplugin.h
<http://svn.reviewboard.kde.org/r/5761/#comment11991>

    This class is not going to be inherited, so this can be changed to `private'.

/trunk/KDE/kdeutils/ark/plugins/clilhaplugin/cliplugin.h
<http://svn.reviewboard.kde.org/r/5761/#comment11993>

    This represents the current entry being parsed, so isn't m_currentEntry better?

/trunk/KDE/kdeutils/ark/plugins/clilhaplugin/cliplugin.cpp
<http://svn.reviewboard.kde.org/r/5761/#comment12000>

    Wrong copyright year.

/trunk/KDE/kdeutils/ark/plugins/clilhaplugin/cliplugin.cpp
<http://svn.reviewboard.kde.org/r/5761/#comment12001>

    Extra empty line.

/trunk/KDE/kdeutils/ark/plugins/clilhaplugin/cliplugin.cpp
<http://svn.reviewboard.kde.org/r/5761/#comment12002>

    Missing empty line above.

/trunk/KDE/kdeutils/ark/plugins/clilhaplugin/cliplugin.cpp
<http://svn.reviewboard.kde.org/r/5761/#comment11994>

    I didn't understand this. Can you elaborate?

/trunk/KDE/kdeutils/ark/plugins/clilhaplugin/cliplugin.cpp
<http://svn.reviewboard.kde.org/r/5761/#comment11995>

    I didn't understand what this has to do with the rest of the comment.

/trunk/KDE/kdeutils/ark/plugins/clilhaplugin/cliplugin.cpp
<http://svn.reviewboard.kde.org/r/5761/#comment11996>

    Some time ago I made some commits to CliInteface that made it always use KPtyProcess instead of KProcess in systems that support it (ie. non-Windows systems). Can you check if the comment still applies? 

/trunk/KDE/kdeutils/ark/plugins/clilhaplugin/cliplugin.cpp
<http://svn.reviewboard.kde.org/r/5761/#comment11997>

    These two variables could be static.

/trunk/KDE/kdeutils/ark/plugins/clilhaplugin/cliplugin.cpp
<http://svn.reviewboard.kde.org/r/5761/#comment11998>

    What if you use QString::split instead? QRegExp's are slow, and the pattern looks simple enough.

/trunk/KDE/kdeutils/ark/plugins/clilhaplugin/cliplugin.cpp
<http://svn.reviewboard.kde.org/r/5761/#comment11999>

    If you return in the if clause above you eliminate the need for the else.

/trunk/KDE/kdeutils/ark/plugins/clilhaplugin/cliplugin.cpp
<http://svn.reviewboard.kde.org/r/5761/#comment12005>

    You should check for the return value.

/trunk/KDE/kdeutils/ark/plugins/clilhaplugin/cliplugin.cpp
<http://svn.reviewboard.kde.org/r/5761/#comment12003>

    isEmpty()

/trunk/KDE/kdeutils/ark/plugins/clilhaplugin/cliplugin.cpp
<http://svn.reviewboard.kde.org/r/5761/#comment12004>

    Same thing about returning in the if.

/trunk/KDE/kdeutils/ark/plugins/clilhaplugin/cliplugin.cpp
<http://svn.reviewboard.kde.org/r/5761/#comment12006>

    You could declare the variable here and make it const.

/trunk/KDE/kdeutils/ark/plugins/clilhaplugin/cliplugin.cpp
<http://svn.reviewboard.kde.org/r/5761/#comment12007>

    Nitpick: extra space character.

/trunk/KDE/kdeutils/ark/plugins/clilhaplugin/cliplugin.cpp
<http://svn.reviewboard.kde.org/r/5761/#comment12008>

    m_lastEntry.clear()

/trunk/KDE/kdeutils/ark/plugins/clilhaplugin/cliplugin.cpp
<http://svn.reviewboard.kde.org/r/5761/#comment12009>

    Extra empty line.

/trunk/KDE/kdeutils/ark/plugins/clilhaplugin/kerfuffle_clilha.desktop
<http://svn.reviewboard.kde.org/r/5761/#comment12011>

    The license mentioned here does not match the one in the copyright headers.

/trunk/KDE/kdeutils/ark/plugins/clilhaplugin/kerfuffle_clilha.desktop
<http://svn.reviewboard.kde.org/r/5761/#comment12010>

    Please do not include the translations here, each translation team should be responsible for adding their translation to the file.

- Raphael

On Nov. 2, 2010, 2:22 p.m., George  Metaxas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://svn.reviewboard.kde.org/r/5761/
> -----------------------------------------------------------
> 
> (Updated Nov. 2, 2010, 2:22 p.m.)
> 
> 
> Review request for kdeutils.
> 
> 
> Summary
> -------
> 
> Implemented lzh/lha support in ark. Most of the required actions work file. However, the lha utility (http://lha.sourceforge.jp) does not like being used by another process. For example, if it has to overwrite a file, it simply skips it, when it is not being run at the terminal.  
> 
> 
> This addresses bug 231896.
>     https://bugs.kde.org/show_bug.cgi?id=231896
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdeutils/ark/plugins/CMakeLists.txt 1191233 
>   /trunk/KDE/kdeutils/ark/plugins/clilhaplugin/CMakeLists.txt PRE-CREATION 
>   /trunk/KDE/kdeutils/ark/plugins/clilhaplugin/cliplugin.h PRE-CREATION 
>   /trunk/KDE/kdeutils/ark/plugins/clilhaplugin/cliplugin.cpp PRE-CREATION 
>   /trunk/KDE/kdeutils/ark/plugins/clilhaplugin/kerfuffle_clilha.desktop PRE-CREATION 
> 
> Diff: http://svn.reviewboard.kde.org/r/5761/diff
> 
> 
> Testing
> -------
> 
> Tested the following cases:
> - Create a new archive
> - Add files and/or directories to an archive 
> - Delete files and/or directories from an archive
> - Extract files and/or directories from an archive
> - Preview files from an archive.
> 
> 
> Thanks,
> 
> George
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/kde-utils-devel/attachments/20110717/9b1673e7/attachment-0001.htm 