As discussed in D3977, document when unregisterJob()
is actually supposed to be manually called.
For example, it is not necessary with the KIO jobtracker.
Details
Details
- Reviewers
kossebau dfaure - Commits
- R244:c688a15d871a: Improve apidox of KJobTrackerInterface
Diff Detail
Diff Detail
- Repository
- R244 KCoreAddons
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
| src/lib/jobs/kjobtrackerinterface.h | ||
|---|---|---|
| 82 | Then it should be protected? | |
| src/lib/jobs/kjobtrackerinterface.h | ||
|---|---|---|
| 82 | Probably yes, should we add a TODO comment for KF6? | |
Comment Actions
(Sorry, forgot about this one, subscribed too to many diffs in the phabricator view)
Looks good to me in general. Added some comments you might want to consider, but are free to ignore :)
| src/lib/jobs/kjobtrackerinterface.h | ||
|---|---|---|
| 54 | IMHO this should have an explicit listing of the signals which are connected, so the API consumer exactly knows what to rely on. A few of the "all the KJob signals" are not wired up. | |
| 82 | I would propose to make this a "@note ", given this is no description of the normal behaviour, but some additional info. It might also catch more attention. | |
| src/lib/jobs/kjobtrackerinterface.h | ||
|---|---|---|
| 82 | And for the protected, not sure, it feels unbalanced to me. But something to think about at time of KF6, so yes, add a (non-doxygen) comment somewhere (e.g. at method line end). | |
| src/lib/jobs/kjobtrackerinterface.h | ||
|---|---|---|
| 88 | We should decide now, don't leave a question mark in a TODO for KF6. Typically when the time comes to actually make the change, we won't remember in details why this is there and it'll be even harder to decide about it. I saw the same with unclear KDE4 TODOs, and then unclear KF5 TODOs.... and probably the same before that too ;) https://lxr.kde.org/source/extragear/sysadmin/apper/apperd/TransactionWatcher.cpp looks like a piece of code that would be broken by this being made protected, if I'm not mistaken. | |
Comment Actions
- Dropped TODO, there is not enough evidence that this method should become protected.