[Differential] [Requested Changes To] D3235: CliInterface refactoring

[elvisangelaccio (Elvis Angelaccio)]

elvisangelaccio requested changes to this revision.
elvisangelaccio added a comment.
This revision now requires changes to proceed.


  I just had a quick look, but overall it looks fine :)

INLINE COMMENTS

> archiveinterface.cpp:54
>      connect(this, &ReadOnlyArchiveInterface::entry, this, &ReadOnlyArchiveInterface::onEntry);
> +    Q_ASSERT(args.size() >= 2);
> +    m_metaData = args.at(1).value<KPluginMetaData>();

This check should be the first thing in the ctor

> cliinterface.h:73
>      virtual void resetParsing() = 0;
> -    virtual ParameterList parameterList() const = 0;
> +    virtual void setupCliParameters(CliParameters *params) = 0;
>      virtual bool readListLine(const QString &line) = 0;

This method in not really necessary. Every plugin could just do the setup in their own constructor (since they have access to `m_cliParameters`).
This way we can also drop the need for `cacheParameterList()`.

> cliinterface.h:87
> +     */
> +    virtual QString escapeFileName(const QString &fileName) const;
> +

Why move this from `private` to `public`?

> cliinterface.h:116
> +
> +    CliParameters *m_cliParameters;
> +

Make it protected and add a public getter

> cliparameters.h:38-76
> +class KERFUFFLE_EXPORT CliParameters: public QObject
> +{
> +    Q_OBJECT
> +
> +    Q_PROPERTY(QString addProgram MEMBER m_addProgram)
> +    Q_PROPERTY(QString deleteProgram MEMBER m_deleteProgram)
> +    Q_PROPERTY(QString extractProgram MEMBER m_extractProgram)

Since this new class is not only about parameters/switches but also other CLI stuff (regexes, etc.) maybe we should find a more descriptive name, though I can't think any at the moment :p

> cliparameters.h:81
> +
> +    bool isInitialized() const;
> +

Once you drop `CliInterface::cacheParameterList()` you can probably drop this as well.

> cliparameters.h:106
> +
> +protected:
> +

unnecessary keyword

REPOSITORY
  rARK Ark

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

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