D5039: Add libzip plugin

[Elvis Angelaccio]

elvisangelaccio requested changes to this revision.
elvisangelaccio added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> libzipplugin.cpp:2
> +/*
> + * Copyright (c) 2016 Ragnar Thomsen <rthomsen6 at gmail.com>
> + *

2017

> libzipplugin.cpp:72
> +
> +    zip_t *archive;
> +    int errcode;

Declare variable as close as possible to where they are needed. In this case this could just be:

  zip_t *archive = zip_open(...);

> libzipplugin.cpp:118
> +
> +    zip_t *archive;
> +    int errcode;

same here.

> libzipplugin.cpp:175
> +    // Register the callback function to get progress feedback.
> +    Callback<void(double)>::func = std::bind(&LibzipPlugin::progressEmitted, this, std::placeholders::_1);
> +    void (*c_func)(double) = static_cast<decltype(c_func)>(Callback<void(double)>::callback);

We are not using the return value of `std::bind()`, are we? Is there a reason why we need the ugly ` Callback<void(double)>::func` here?

> libzipplugin.cpp:255
> +
> +    auto e = new Archive::Entry();
> +

The `Entry` objects need to be deleted. I think here we should do what libarchive does, i.e. delete them (later) in the destructor.

> libzipplugin.cpp:432
> +    qCDebug(ARK) << "Killing operation...";
> +    m_abortOperation = true;
> +    return true;

Remove `m_abortOperation`. The libzip plugin will run in another thread, we need to use `QThread::currentThread()->isInterruptionRequested()` instead of this boolean flag. Have a look at the libarchive plugin.

> libzipplugin.cpp:581
> +    // Handle password-protected files.
> +    zip_file *zf = NULL;
> +    bool firstTry = true;

`nullptr`?

> libzipplugin.h:2
> +/*
> + * Copyright (c) 2016 Ragnar Thomsen <rthomsen6 at gmail.com>
> + *

2017 :p

> libzipplugin.h:60-63
> +    bool m_abortOperation;
> +    bool m_overwriteAll;
> +    bool m_skipAll;
> +    bool m_listAfterAdd = false;

Either initialize all the member variables here on in the constructor initializer list.

REPOSITORY
  R36 Ark

REVISION DETAIL
  https://phabricator.kde.org/D5039

To: rthomsen, elvisangelaccio
Cc: kde-utils-devel, #ark, tctara
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-utils-devel/attachments/20170313/a7ff4b16/attachment.html>