Add KListOpenFilesJob for retrieving the list of processes that have opened files in the given path or a subdirectory of path. This code is ported from Device Notifier but rewritten to use KJob.
Details
- Reviewers
davidedmundson broulik dfaure bruns cfeck - Group Reviewers
Frameworks Plasma - Commits
- R244:e119ce7b5afe: Add KListOpenFilesJob
Unit Tests
Diff Detail
- Repository
- R244 KCoreAddons
- Branch
- add_list_processes_with_open_files (branched from master)
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 16386 Build 16404: arc lint + arc unit
@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 :)
src/lib/util/klistopenfiles.cpp | ||
---|---|---|
44 ↗ | (On Diff #64911) | Should I also set an error string here? And what about defining custom error codes, is that how we usually do? |
src/lib/util/klistopenfiles.h | ||
37 ↗ | (On Diff #64911) | I am a little unsure if there is any other KJob functions that should be overwritten in this class? The current implementation is quite minimal, but maybe that is fine? |
src/lib/util/klistopenfiles.cpp | ||
---|---|---|
29 ↗ | (On Diff #64911) | I wonder if this actually needs to be a QObject, given that you use connect-to-pointer-to-member-function? |
44 ↗ | (On Diff #64911) | KIO::ERR_DOES_NOT_EXIST would fit well here, but that's in kio... I suggest adding enum { ERR_DOES_NOT_EXIST = KJob::UserDefinedError + 11 } And yes, you should do something like setErrorText(i18n("Directory does not exist: %1", path)); |
53 ↗ | (On Diff #64911) | +setErrorText(<something dependent on ProcessError>) |
src/lib/util/klistopenfiles.h | ||
41 ↗ | (On Diff #64911) | const QDir & --- or actually, we use const QString & everywhere else for paths. |
51 ↗ | (On Diff #64911) | The @since should go into the (missing) class documentation, given that the whole class is new. |
@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?
src/lib/util/klistopenfiles.cpp | ||
---|---|---|
29 ↗ | (On Diff #64911) | The primary motivation was memory-management, but I could also just make the d-pointer in ListOpenFilesJob a QScopedPointer, would that be better? |
44 ↗ | (On Diff #64911) | I have added the error enum entries, but I am a little hesitant to add to error texts since KCoreAddons doesn't today depend on KDE::I18n and this would introduce that dependency, do we really want that? When I look at the KIO classes then they usually set som non-translated string as the error text, would that be an option? |
One thing, when this is ready to land I will address the Windows support so that we do not get broken builds :)
The KJob usage turned out exactly like I thought it would, I still think it's the right thing to do :-)
src/lib/util/klistopenfiles.cpp | ||
---|---|---|
29 ↗ | (On Diff #64911) | Yes, that would be more lightweight. |
44 ↗ | (On Diff #64911) | My bad. In KCoreAddons should should use tr()+arg() rather than i18n. KIO doesn't have non-translated strings. It reimplements errorString() to combine the errorText (usually a path or URL) with a translated string based on the error code. |
Review comments. Added minimal Windows implementation which basically always reports failure with the error code Unsupported.
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 :)
Nice job. Final comments.
autotests/klistopenfilestest_unix.cpp | ||
---|---|---|
51 ↗ | (On Diff #65237) | prepend const so begin/end below don't detach |
83 ↗ | (On Diff #65237) | 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. |
src/lib/util/klistopenfiles.h | ||
94 ↗ | (On Diff #65237) | KIO is designed like this for some reason, but I would just document the job constructor and let people create the job with new, outside KIO. That's e.g. what akonadi does. |
autotests/klistopenfilestest_unix.cpp | ||
---|---|---|
83 ↗ | (On Diff #65237) | This test is unix only anyway so there shouldn't be any race conditions. But it might be a little more expressive to use "/does/not/exist" so I have changed it anyway ;) |
src/lib/util/klistopenfiles.h | ||
94 ↗ | (On Diff #65237) | Would it make sense to rename the ListOpenFilesJob class to simply Job? It is already inside a KListOpenFiles namespace so the current name seems a little long and duplicated? Do we have any general naming convention for these things? |
Given that the namespace doesn't contain anything else anymore, I would just get rid of it, and provide a single class, KListOpenFilesJob.
src/lib/util/klistopenfiles.h | ||
---|---|---|
23 ↗ | (On Diff #65330) | Should these files be renamed to klistopenfilesjob (along with tests etc.) now that is what the class is called? |
Yes, the filenames should match the classname, obviously :)
I did a deeper review of the KJob usage and I have two more comments, sorry for not taking the time to do this earlier...
src/lib/util/klistopenfiles_unix.cpp | ||
---|---|---|
57 ↗ | (On Diff #65330) | Emitting result twice would be a very bad idea, a KJob can only emit result once. But QProcess can emit errorOccurred followed by finished (e.g. if the subprocess crashes, from what I can see in qprocess.cpp). |
62 ↗ | (On Diff #65330) | const |
73 ↗ | (On Diff #65330) | The class would be slightly easier to use if this was a getter instead of a signal. Because then, in a unittest, you can just do job->exec() and then job->processInfoList(), no spy needed. See KIO::StatJob::statResult() for an example. Signals emitted by jobs are useful if they happen during the job lifetime (e.g. progress signals). If they happen at the same time as result, then it's easier to use result as the notification signal. The downside is one more member variable, but it'll be very short-lived so I wouldn't worry about memory usage. |
No problem ;) I would rather have this change go in slow and correct than quick and dirty :)
src/lib/util/klistopenfiles_unix.cpp | ||
---|---|---|
57 ↗ | (On Diff #65330) | Good catch! Actually it turns out to be a tad more complicated ;) QProcess will only emit finished if the process actually started, so if you have the case where lsof is not installed you will only get the errorOccurred signal and not finished, but if it crashes you get both. I actually added a unit test to test the first case (where lsof is not found), and I ended up making a solution where we simply remember if we have emitted the result and only emit it if we haven't done so before. |
73 ↗ | (On Diff #65330) | Agree, I have refactored the code to do this and it is much simpler :) |
Yep, much simpler indeed.
autotests/klistopenfilesjobtest_unix.cpp | ||
---|---|---|
35 | (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. | |
81 | 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). | |
src/lib/util/klistopenfilesjob.h | ||
38 | s/has/have/ | |
40 | [is it useful to repeat that sentence? I admit I'm no expert on Doxygen's @brief command] | |
42 | has -> have (plural) | |
58 | 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 | |
src/lib/util/klistopenfilesjob_unix.cpp | ||
94 | 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). | |
src/lib/util/klistopenfilesjob_win.cpp | ||
27 | Won't be needed anymore once the getter returns by value :-) |
autotests/klistopenfilesjobtest_unix.cpp | ||
---|---|---|
35 | Makes sense, this was a leftover from when the job was returned by a function instead of new-ing it directly :) Another reason for not checking is that new doesn't return nullptr on failure, instead it throws std::bad_alloc, to get nullptr on failure you need to use the nothrow version of new :) | |
src/lib/util/klistopenfilesjob.h | ||
40 | I don't think it is useful, so I have remove it | |
src/lib/util/klistopenfilesjob_win.cpp | ||
27 | Remove :) |
This is good to go in :-)
If you push it today it'll indeed be in 5.62, to be tagged tomorrow, otherwise we'll need to adjust the @since tag ;-)
I think i prefer waiting to after the release so it has a chance to get some mileage before release :) I will make sure to update the @since tag.
Now we can use it :
dolphin D19989
plasma-vault
libksysguard (KLsofWidget)
dataengines/devicenotifications/ksolidnotify
The test fails on FreeBSD, in CI.
Any plans for fixing it, or should I mark it as expected failure?