Review Request: Added support for LZO archives

Sat Nov 26 14:21:46 UTC 2011

> On Nov. 18, 2011, 1:57 p.m., Raphael Kubo da Costa wrote:
> > 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?

This being my first contribution to the Ark: I thought that to better understand the code to skip for now the libraries.

I didn't used QLatin1String and QLatin1Char, because I based my code on that found in cliplugin-example, which doesn't use them. I'll change also the example :)

> On Nov. 18, 2011, 1:57 p.m., Raphael Kubo da Costa wrote:
> > plugins/clilzoplugin/cliplugin.cpp, line 47
> > <http://git.reviewboard.kde.org/r/103100/diff/1/?file=40749#file40749line47>
> >
> >     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?

When I try to delete a file after the confirmation message:
"Deleting these files is not undoable. Are you sure you want to do this?"

Then appears this error:
"Failed to locate program in PATH."

Looking here http://api.kde.org/4.7-api/kdeutils-apidocs/ark/html/cliinterface_8cpp_source.html#l00324 CliInterface can manage omission of programs.

Maybe I can add in CliInterface::deleteFiles a check, which control if DeleteProgram isn't defined, and displays a warning message saying that deletion is not supported?

> On Nov. 18, 2011, 1:57 p.m., Raphael Kubo da Costa wrote:
> > plugins/clilzoplugin/cliplugin.cpp, line 50
> > <http://git.reviewboard.kde.org/r/103100/diff/1/?file=40749#file40749line50>
> >
> >     lzop's man page lists some examples of how to extract single files. Do they not work?

I could not find how to extract a single file, even on man page. Can you show me please if you found something?

If is not possible to extract single files, maybe we can add a flag, something like SingleFilesExtractionSupported, then if false, CliInterface will extract the entire archive in /tmp, copying only specified files, it's also useful storing the path where the entire archive has been extracted in case the user want to extract/view another file from the same archive.
I thought it might be a solution, what do you think?

> On Nov. 18, 2011, 1:57 p.m., Raphael Kubo da Costa wrote:
> > plugins/clilzoplugin/kerfuffle_clilzo.desktop, line 64
> > <http://git.reviewboard.kde.org/r/103100/diff/1/?file=40750#file40750line64>
> >
> >     Can you rebase on top of master and set the mimetypes the way the other plugins are currently doing?

LZO archives has two mimetypes, I was undecided about how to set them. After looking in clizipplugin/kerfuffle_clilzo.desktop I saw also there two mimetypes and I followed that way.
But if is not correct I can remove 'application/x-lzo-compressed;' because it's rarely used.

> On Nov. 18, 2011, 1:57 p.m., Raphael Kubo da Costa wrote:
> > plugins/clilzoplugin/cliplugin.cpp, lines 95-96
> > <http://git.reviewboard.kde.org/r/103100/diff/1/?file=40749#file40749line95>
> >
> >     Splitting and then grabbing the last element looks easier to read.

Yes, it's better :), I had not thought...

> On Nov. 18, 2011, 1:57 p.m., Raphael Kubo da Costa wrote:
> > plugins/clilzoplugin/cliplugin.cpp, lines 54-56
> > <http://git.reviewboard.kde.org/r/103100/diff/1/?file=40749#file40749line54>
> >
> >     What will happen if one tries to do that in Ark then?

The archive will not be modified, without displaying error/warning messages.

Perhaps it's better handling this situation like with deletion? With AddProgram non specified and then display an error message or disabling the add button.

> On Nov. 18, 2011, 1:57 p.m., Raphael Kubo da Costa wrote:
> > plugins/clilzoplugin/cliplugin.cpp, lines 62-63
> > <http://git.reviewboard.kde.org/r/103100/diff/1/?file=40749#file40749line62>
> >
> >     Does it mean lzop will always overwrite files when extracting or will always skip them?

It will always skip them: lzop overwrites only if "-f" parameter is specified.

> On Nov. 18, 2011, 1:57 p.m., Raphael Kubo da Costa wrote:
> > plugins/clilzoplugin/cliplugin.cpp, line 60
> > <http://git.reviewboard.kde.org/r/103100/diff/1/?file=40749#file40749line60>
> >
> >     I guess you mean p[FileExistsInput] and p[FileExistsExpression]?

Yes, I typed the same thing :)

> On Nov. 18, 2011, 1:57 p.m., Raphael Kubo da Costa wrote:
> > plugins/clilzoplugin/cliplugin.cpp, lines 84-88
> > <http://git.reviewboard.kde.org/r/103100/diff/1/?file=40749#file40749line84>
> >
> >     Will there be problems when the footer is not printed (it wasn't when I listed an archive with a single file)?

I tried also with an archive with a single file without problems.

- Othmane

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

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/20111126/bcc3cc16/attachment-0001.html>