D21760: Add KListOpenFilesJob

[David Faure]

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


  Yep, much simpler indeed.

INLINE COMMENTS

> klistopenfilesjobtest_unix.cpp:34
> +    auto job = new KListOpenFilesJob(path.path());
> +    QVERIFY(job);
> +    job->exec();

(minor) we never check that `new` succeeded, in Qt code.

The reasoning is that on desktop systems, with swap enabled, before the swap is exhausted, the user will have given up and rebooted the machine anyway. I know I do, many times a month :). So in practice, `new` can be considered to always succeed.

> klistopenfilesjobtest_unix.cpp:80
> +    {
> +        setenv(name_.latin1(), newValue.data(), 1);
> +    }

You can use `qputenv`, more portable (in case we ever enable this code on Windows), safer in terms of life cycle of the strings (I mean in general, I know it isn't a problem here).

> klistopenfilesjob.h:37
> +/**
> + * @brief Provides information about processes that has open files in a given path or subdirectory of path.
> + *

s/has/have/

> klistopenfilesjob.h:39
> + *
> + * Provides information about processes that has open files in a given path or subdirectory of path.
> + *

[is it useful to repeat that sentence? I admit I'm no expert on Doxygen's @brief command]

> klistopenfilesjob.h:41
> + *
> + * When start() is invoked it starts to collect information about processes that has any files open in path or a
> + * subdirectory of path. When it is done the KJob::result signal is emitted and the result can be retrieved with the

has -> have  (plural)

> klistopenfilesjob.h:57
> +    void start() override;
> +    const KProcessList::KProcessInfoList& processInfoList() const;
> +

Return by value rather than by const ref. Otherwise it makes it hard to one day write something like

  if (someCondition)
      return KProcessInfoList();
  return m_processInfoList;

The cost of increasing/decreasing a refcount is negligible, especially in code that starts a secondary process :-)

+ don't forget to document the method

> klistopenfilesjob_unix.cpp:93
> +    const QDir path;
> +    bool hasEmittedResult_;
> +    QProcess lsofProcess;

Inconsistent: two member variables have a trailing '_', the others don't.

In KF5 code it's usual to either have no prefix, or have `m_` as prefix (but never a suffix).

> klistopenfilesjob_win.cpp:26
> +public:
> +    KProcessList::KProcessInfoList processInfoList;
> +};

Won't be needed anymore once the getter returns by value :-)

REPOSITORY
  R244 KCoreAddons

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

To: hallas, davidedmundson, broulik, #frameworks, dfaure, bruns, #plasma
Cc: meven, cfeck, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20190906/1100257e/attachment-0001.html>