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