dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> klistopenfilestest.cpp:45
> + eventLoop.exec();
> + QVERIFY(processInfoList.empty() == false);
> + auto testProcessIterator = std::find_if(processInfoList.begin(),
> processInfoList.end(), [](const KProcessList::KProcessInfo& info)
QVERIFY(!processInfoList.empty())
> klistopenfilestest.cpp:52
> + const auto& processInfo = *testProcessIterator;
> + QVERIFY(processInfo.isValid() == true);
> + QCOMPARE(processInfo.pid(), QCoreApplication::applicationPid());
The `== true` can be removed
(repeats)
> klistopenfiles.cpp:39
> + KProcessList::KProcessInfoList processInfoList;
> + for (const auto& pidStr : pidList) {
> + qint64 pid = pidStr.toLongLong();
qAsConst(pidList) to avoid a detach
> hallas wrote in klistopenfiles.cpp:49
> What should we do for Windows? Do we support any other platforms that doesn't
> have lsof?
This reminds me of KCoreAddons' new KProcessInfo class, although that one
doesn't actually do this.
I see that FreeBSD and OSX have lsof (and the man page doesn't say those
options are missing there) so it should be fine, everywhere except Windows.
In general I don't really like starting executables like this, but indeed
writing a clone of lsof sounds like much work.
> hallas wrote in klistopenfiles.h:40
> I am very much in doubt about what a nice interface for this should be? I was
> thinking of an alternative approach where you would instantiate a class,
> connect to a signal on the class and then invoke a method to start the
> listing process, you would then receive the result on a slot instead. What do
> you guys think?
Reading the unittest, I definitely agree that a signal would be better. Then
you could use QSignalSpy::wait() without the need to use QEventLoop by hand :-)
This is really a job. I would inherit KJob for this, actually. It's the way we
do such things everywhere else.
REPOSITORY
R244 KCoreAddons
REVISION DETAIL
https://phabricator.kde.org/D21760
To: hallas, davidedmundson, broulik, #frameworks, dfaure, bruns, #plasma
Cc: kde-frameworks-devel, LeGast00n, sbergeron, michaelh, ngraham, bruns
