D21760: Add KListOpenFiles::listProcessesWithOpenFiles

hallas added inline comments. INLINE COMMENTS > dfaure wrote in klistopenfilestest_unix.cpp:83 > I usually just use "/does/not/exist" as a path ;-) > > This might actually be better because Windows has weird race conditions with > the filesystem stuff. This test is unix only anyway so there

D21760: Add KListOpenFiles::listProcessesWithOpenFiles

hallas updated this revision to Diff 65284. hallas marked 3 inline comments as done. hallas added a comment. Review comments REPOSITORY R244 KCoreAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D21760?vs=65237=65284 BRANCH add_list_processes_with_open_files (branched from

D21760: Add KListOpenFiles::listProcessesWithOpenFiles

dfaure requested changes to this revision. dfaure added a comment. This revision now requires changes to proceed. Nice job. Final comments. INLINE COMMENTS > klistopenfilestest_unix.cpp:51 > +QCOMPARE(processInfoAvailableSpy.at(0).at(0).value(), > path.path()); > +

D21760: Add KListOpenFiles::listProcessesWithOpenFiles

hallas added a comment. I have added a minimal Windows implementation which always emits an error, along with a unit test. Please review it thoroughly and then I think it is ready to land :) REPOSITORY R244 KCoreAddons REVISION DETAIL https://phabricator.kde.org/D21760 To: hallas,

D21760: Add KListOpenFiles::listProcessesWithOpenFiles

hallas updated this revision to Diff 65237. hallas marked 2 inline comments as done. hallas added a comment. Review comments. Added minimal Windows implementation which basically always reports failure with the error code Unsupported. REPOSITORY R244 KCoreAddons CHANGES SINCE LAST UPDATE

D21760: Add KListOpenFiles::listProcessesWithOpenFiles

dfaure requested changes to this revision. dfaure added a comment. This revision now requires changes to proceed. The KJob usage turned out exactly like I thought it would, I still think it's the right thing to do :-) INLINE COMMENTS > hallas wrote in klistopenfiles.cpp:29 > The primary

D21760: Add KListOpenFiles::listProcessesWithOpenFiles

hallas added a comment. One thing, when this is ready to land I will address the Windows support so that we do not get broken builds :) REPOSITORY R244 KCoreAddons REVISION DETAIL https://phabricator.kde.org/D21760 To: hallas, davidedmundson, broulik, #frameworks, dfaure, bruns,

D21760: Add KListOpenFiles::listProcessesWithOpenFiles

hallas updated this revision to Diff 64973. hallas marked 2 inline comments as done. hallas added a comment. Review comments REPOSITORY R244 KCoreAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D21760?vs=64911=64973 BRANCH add_list_processes_with_open_files (branched from

D21760: Add KListOpenFiles::listProcessesWithOpenFiles

hallas added a comment. @dfaure - Overall, what do you think about the approach of subclassing KJob? Did it turn out like you had thought? And is this the solution we should go with, or was one of the other solutions better? INLINE COMMENTS > dfaure wrote in klistopenfiles.cpp:29 > I

D21760: Add KListOpenFiles::listProcessesWithOpenFiles

meven edited the summary of this revision. 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

D21760: Add KListOpenFiles::listProcessesWithOpenFiles

dfaure added inline comments. INLINE COMMENTS > klistopenfiles.cpp:29 > + > +class ListOpenFilesJobPrivate : public QObject > +{ I wonder if this actually needs to be a QObject, given that you use connect-to-pointer-to-member-function? > hallas wrote in klistopenfiles.cpp:44 > Should I also

D21760: Add KListOpenFiles::listProcessesWithOpenFiles

hallas added inline comments. INLINE COMMENTS > klistopenfiles.cpp:44 > +if (!path.exists()) { > +job->setError(KJob::UserDefinedError); > +job->emitResult(); Should I also set an error string here? And what about defining custom error codes, is that how we

D21760: Add KListOpenFiles::listProcessesWithOpenFiles

hallas added a comment. @meven , so I finally managed to rewrite this patch to use KJob instead. Please take a look at it again and see if this is better approach :) REPOSITORY R244 KCoreAddons REVISION DETAIL https://phabricator.kde.org/D21760 To: hallas, davidedmundson, broulik,

D21760: Add KListOpenFiles::listProcessesWithOpenFiles

hallas updated this revision to Diff 64911. hallas added a comment. Rewrote the code to use KJob REPOSITORY R244 KCoreAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D21760?vs=59636=64911 BRANCH add_list_processes_with_open_files (branched from master) REVISION DETAIL

D21760: Add KListOpenFiles::listProcessesWithOpenFiles

hallas added inline comments. INLINE COMMENTS > meven wrote in klistopenfiles.cpp:39 > The qAsConst wrapping is missing it seems. Yeah, I have only done this change locally ;) REPOSITORY R244 KCoreAddons REVISION DETAIL https://phabricator.kde.org/D21760 To: hallas, davidedmundson,

D21760: Add KListOpenFiles::listProcessesWithOpenFiles

meven added inline comments. INLINE COMMENTS > dfaure wrote in klistopenfiles.cpp:39 > Not possible, due to the removeDuplicates() line. The qAsConst wrapping is missing it seems. REPOSITORY R244 KCoreAddons REVISION DETAIL https://phabricator.kde.org/D21760 To: hallas, davidedmundson,

D21760: Add KListOpenFiles::listProcessesWithOpenFiles

broulik added inline comments. INLINE COMMENTS > klistopenfiles.cpp:26 > + > +void listProcessesWithOpenFiles(QDir path, Callback callback) > +{ Can you allow passing an optional context object, so when the caller gets deleted, you don't crash when the reply comes in? REPOSITORY R244

D21760: Add KListOpenFiles::listProcessesWithOpenFiles

hallas marked 3 inline comments as done. hallas added inline comments. INLINE COMMENTS > dfaure wrote in klistopenfiles.cpp:49 > 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

D21760: Add KListOpenFiles::listProcessesWithOpenFiles

hallas added a subscriber: cfeck. hallas added a comment. In D21760#493817 , @cfeck wrote: > You wrote "ported from Device Notifier". I haven't check in detail yet, but do other copyright holders need to be mentioned? I have added the

D21760: Add KListOpenFiles::listProcessesWithOpenFiles

dfaure added inline comments. INLINE COMMENTS > broulik wrote in klistopenfiles.cpp:39 > Or just make the `pidList` `const` which it should be anyway Not possible, due to the removeDuplicates() line. REPOSITORY R244 KCoreAddons REVISION DETAIL https://phabricator.kde.org/D21760 To:

D21760: Add KListOpenFiles::listProcessesWithOpenFiles

broulik added inline comments. INLINE COMMENTS > dfaure wrote in klistopenfiles.cpp:39 > qAsConst(pidList) to avoid a detach Or just make the `pidList` `const` which it should be anyway REPOSITORY R244 KCoreAddons REVISION DETAIL https://phabricator.kde.org/D21760 To: hallas,

D21760: Add KListOpenFiles::listProcessesWithOpenFiles

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 =

D21760: Add KListOpenFiles::listProcessesWithOpenFiles

ngraham added reviewers: bruns, Plasma. REPOSITORY R244 KCoreAddons REVISION DETAIL https://phabricator.kde.org/D21760 To: hallas, davidedmundson, broulik, #frameworks, dfaure, bruns, #plasma Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D21760: Add KListOpenFiles::listProcessesWithOpenFiles

cfeck resigned from this revision. cfeck added a comment. You wrote "ported from Device Notifier". I haven't check in detail yet, but do other copyright holders need to be mentioned? REPOSITORY R244 KCoreAddons REVISION DETAIL https://phabricator.kde.org/D21760 To: hallas,

D21760: Add KListOpenFiles::listProcessesWithOpenFiles

ngraham added reviewers: Frameworks, cfeck, dfaure. REPOSITORY R244 KCoreAddons REVISION DETAIL https://phabricator.kde.org/D21760 To: hallas, davidedmundson, broulik, #frameworks, cfeck, dfaure Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D21760: Add KListOpenFiles::listProcessesWithOpenFiles

hallas added a comment. Ping :D REPOSITORY R244 KCoreAddons REVISION DETAIL https://phabricator.kde.org/D21760 To: hallas, davidedmundson, broulik Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D21760: Add KListOpenFiles::listProcessesWithOpenFiles

hallas added a comment. First of, this is WIP, I just wanted to share this early to get some feedback. The reasoning for this is to generalize functionality for running `lsof`, this is currently in use by the Device Notifier applet and I would like to use it in Dolphin to fix bug #189302.

D21760: Add KListOpenFiles::listProcessesWithOpenFiles

hallas created this revision. hallas added reviewers: davidedmundson, broulik. hallas added a project: Frameworks. hallas requested review of this revision. REVISION SUMMARY Add KListOpenFiles::listProcessesWithOpenFiles for retrieving the list of processes that has files open in the given