D8336: Improve apidox of KJobTrackerInterface
This revision was automatically updated to reflect the committed changes. Closed by commit R244:c688a15d871a: Improve apidox of KJobTrackerInterface (authored by elvisangelaccio). REPOSITORY R244 KCoreAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8336?vs=23725=23831 REVISION DETAIL https://phabricator.kde.org/D8336 AFFECTED FILES src/lib/jobs/kjobtrackerinterface.h To: elvisangelaccio, kossebau, dfaure Cc: apol, #frameworks
D8336: Improve apidox of KJobTrackerInterface
dfaure accepted this revision. This revision is now accepted and ready to land. REPOSITORY R244 KCoreAddons BRANCH master REVISION DETAIL https://phabricator.kde.org/D8336 To: elvisangelaccio, kossebau, dfaure Cc: apol, #frameworks
D8336: Improve apidox of KJobTrackerInterface
elvisangelaccio updated this revision to Diff 23725. elvisangelaccio added a comment. - Dropped TODO, there is not enough evidence that this method should become protected. REPOSITORY R244 KCoreAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8336?vs=23105=23725 BRANCH master REVISION DETAIL https://phabricator.kde.org/D8336 AFFECTED FILES src/lib/jobs/kjobtrackerinterface.h To: elvisangelaccio, kossebau, dfaure Cc: apol, #frameworks
D8336: Improve apidox of KJobTrackerInterface
elvisangelaccio marked an inline comment as done. REPOSITORY R244 KCoreAddons REVISION DETAIL https://phabricator.kde.org/D8336 To: elvisangelaccio, kossebau, dfaure Cc: apol, #frameworks
D8336: Improve apidox of KJobTrackerInterface
dfaure requested changes to this revision. dfaure added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > kjobtrackerinterface.h:88 > */ > -virtual void unregisterJob(KJob *job); > +virtual void unregisterJob(KJob *job); // TODO KF6: should it become > protected? > 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. But then again, it might be broken code in the first place, in which case we can still make the change... please investigate. REPOSITORY R244 KCoreAddons REVISION DETAIL https://phabricator.kde.org/D8336 To: elvisangelaccio, kossebau, dfaure Cc: apol, #frameworks
D8336: Improve apidox of KJobTrackerInterface
elvisangelaccio updated this revision to Diff 23105. elvisangelaccio marked 3 inline comments as done. elvisangelaccio added a comment. - Addressed comments REPOSITORY R244 KCoreAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8336?vs=20870=23105 BRANCH master REVISION DETAIL https://phabricator.kde.org/D8336 AFFECTED FILES src/lib/jobs/kjobtrackerinterface.h To: elvisangelaccio, kossebau, dfaure Cc: apol, #frameworks
D8336: Improve apidox of KJobTrackerInterface
kossebau added inline comments. INLINE COMMENTS > elvisangelaccio wrote in kjobtrackerinterface.h:73 > Probably yes, should we add a TODO comment for KF6? 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). REPOSITORY R244 KCoreAddons BRANCH master REVISION DETAIL https://phabricator.kde.org/D8336 To: elvisangelaccio, kossebau, dfaure Cc: apol, #frameworks
D8336: Improve apidox of KJobTrackerInterface
kossebau accepted this revision. kossebau added a comment. This revision is now accepted and ready to land. (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 :) INLINE COMMENTS > kjobtrackerinterface.h:54 > * Register a new job in this tracker. > + * The default implementation connects all the KJob signals > + * to the protected slots of this class. 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. > kjobtrackerinterface.h:73 > * Unregister a job from this tracker. > + * You need to manually call this method only if you re-implemented > + * registerJob() without connecting KJob::finished to this slot. 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. REPOSITORY R244 KCoreAddons BRANCH master REVISION DETAIL https://phabricator.kde.org/D8336 To: elvisangelaccio, kossebau, dfaure Cc: apol, #frameworks
D8336: Improve apidox of KJobTrackerInterface
elvisangelaccio added a comment. @kossebau Ping? REPOSITORY R244 KCoreAddons REVISION DETAIL https://phabricator.kde.org/D8336 To: elvisangelaccio, kossebau, dfaure Cc: apol, #frameworks
D8336: Improve apidox of KJobTrackerInterface
elvisangelaccio added inline comments. INLINE COMMENTS > apol wrote in kjobtrackerinterface.h:73 > Then it should be protected? Probably yes, should we add a TODO comment for KF6? REPOSITORY R244 KCoreAddons REVISION DETAIL https://phabricator.kde.org/D8336 To: elvisangelaccio, kossebau, dfaure Cc: apol, #frameworks
D8336: Improve apidox of KJobTrackerInterface
apol added inline comments. INLINE COMMENTS > kjobtrackerinterface.h:73 > * Unregister a job from this tracker. > + * You need to manually call this method only if you re-implemented > + * registerJob() without connecting KJob::finished to this slot. Then it should be protected? REPOSITORY R244 KCoreAddons REVISION DETAIL https://phabricator.kde.org/D8336 To: elvisangelaccio, kossebau, dfaure Cc: apol, #frameworks
D8336: Improve apidox of KJobTrackerInterface
elvisangelaccio created this revision. elvisangelaccio added reviewers: kossebau, dfaure. Restricted Application added a project: Frameworks. Restricted Application added a subscriber: Frameworks. REVISION SUMMARY As discussed in https://phabricator.kde.org/D3977, document when unregisterJob() is actually supposed to be manually called. For example, it is not necessary with the KIO jobtracker. REPOSITORY R244 KCoreAddons BRANCH master REVISION DETAIL https://phabricator.kde.org/D8336 AFFECTED FILES src/lib/jobs/kjobtrackerinterface.h To: elvisangelaccio, kossebau, dfaure Cc: #frameworks
