D29201: KCMUtils: Add option to append service file to list of arguments
meven added inline comments. INLINE COMMENTS > alex wrote in kpluginselector.cpp:855 > You could add a field to the KCModule class (thats where you ultimately want > to access the fileName). > But then the downside is that you don't have them as a constructor argument. moduleInfo is part of the ctor here, so the fileName is already available indirectly. A good thing would be to avoid having an option for such a niche usage although legitimate. REPOSITORY R295 KCMUtils REVISION DETAIL https://phabricator.kde.org/D29201 To: alex, #plasma, ngraham, meven, broulik, mart Cc: mart, apol, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D29201: KCMUtils: Add option to append service file to list of arguments
alex added inline comments. INLINE COMMENTS > meven wrote in kpluginselector.cpp:855 > moduleInfo is part of the ctor here, so the fileName is already available > indirectly. > A good thing would be to avoid having an option for such a niche usage > although legitimate. > moduleInfo is part of the ctor here, so the fileName is already available > indirectly. Yes, but the KCModuleProxy is just a wrapper class for the KCModule. The actual KCModule gets created in kcmoduleloader.cpp line 93+. PS: The moduleInfo is also available there, but I don't see any elegant way to make it available to the KCModule other than a property or appending it to the list of arguments. > A good thing would be to avoid having an option for such a niche usage > although legitimate. Always open to suggestions 😃 REPOSITORY R295 KCMUtils REVISION DETAIL https://phabricator.kde.org/D29201 To: alex, #plasma, ngraham, meven, broulik, mart Cc: mart, apol, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D29201: KCMUtils: Add option to append service file to list of arguments
meven added inline comments. INLINE COMMENTS > alex wrote in kpluginselector.cpp:855 > > moduleInfo is part of the ctor here, so the fileName is already available > > indirectly. > > Yes, but the KCModuleProxy is just a wrapper class for the KCModule. > The actual KCModule gets created in kcmoduleloader.cpp line 93+. > > PS: The moduleInfo is also available there, but I don't see any elegant way > to make it available to the KCModule other than a property or appending it to > the list of arguments. > > > A good thing would be to avoid having an option for such a niche usage > > although legitimate. > > Always open to suggestions 😃 Seems like a property would make sense, after all it is about it, or a ref to the KCModuleInfo REPOSITORY R295 KCMUtils REVISION DETAIL https://phabricator.kde.org/D29201 To: alex, #plasma, ngraham, meven, broulik, mart Cc: mart, apol, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D29201: KCMUtils: Add option to append service file to list of arguments
alex added inline comments. INLINE COMMENTS > meven wrote in kpluginselector.cpp:855 > Seems like a property would make sense, after all it is about it, or a ref to > the KCModuleInfo I have created a PR on Gitlab for this REPOSITORY R295 KCMUtils REVISION DETAIL https://phabricator.kde.org/D29201 To: alex, #plasma, ngraham, meven, broulik, mart Cc: mart, apol, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D29201: KCMUtils: Add option to append service file to list of arguments
alex abandoned this revision. alex added a comment. Abandoning this in favor of https://invent.kde.org/frameworks/kconfigwidgets/-/merge_requests/1 and a follow up PR. REPOSITORY R295 KCMUtils REVISION DETAIL https://phabricator.kde.org/D29201 To: alex, #plasma, ngraham, meven, broulik, mart Cc: mart, apol, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D29201: KCMUtils: Add option to append service file to list of arguments
alex created this revision. alex added reviewers: Plasma, ngraham. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. alex requested review of this revision. REVISION SUMMARY By setting the new variable to true the service path gets appended to the list of arguments. Otherwise there is no way (at least not that I know of, please correct me if I am wrong) of getting the service file which was used to launch the module. TEST PLAN Add `m_pluginSelector->setAppendServicePath(true);` in the kcms/runners/kcm.cpp in the repo plasma-desktop. If you print out the arguments of a runner config module (plasma-workspace contains some) you should see the service file. REPOSITORY R295 KCMUtils BRANCH service_path_append (branched from master) REVISION DETAIL https://phabricator.kde.org/D29201 AFFECTED FILES src/kpluginselector.cpp src/kpluginselector.h src/kpluginselector_p.h To: alex, #plasma, ngraham Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D29201: KCMUtils: Add option to append service file to list of arguments
alex edited the summary of this revision. alex added reviewers: meven, broulik. REPOSITORY R295 KCMUtils REVISION DETAIL https://phabricator.kde.org/D29201 To: alex, #plasma, ngraham, meven, broulik Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D29201: KCMUtils: Add option to append service file to list of arguments
alex updated this revision to Diff 81798. alex added a comment. Rename method REPOSITORY R295 KCMUtils CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D29201?vs=81239&id=81798 BRANCH service_path_append (branched from master) REVISION DETAIL https://phabricator.kde.org/D29201 AFFECTED FILES src/kpluginselector.cpp src/kpluginselector.h src/kpluginselector_p.h To: alex, #plasma, ngraham, meven, broulik Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D29201: KCMUtils: Add option to append service file to list of arguments
alex updated this revision to Diff 81799. alex added a comment. Little typo :-) REPOSITORY R295 KCMUtils CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D29201?vs=81798&id=81799 BRANCH service_path_append (branched from master) REVISION DETAIL https://phabricator.kde.org/D29201 AFFECTED FILES src/kpluginselector.cpp src/kpluginselector.h src/kpluginselector_p.h To: alex, #plasma, ngraham, meven, broulik Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D29201: KCMUtils: Add option to append service file to list of arguments
alex added a dependent revision: D29384: KCM Runners: Use setAppendServiceFile method for plugin selector. REPOSITORY R295 KCMUtils REVISION DETAIL https://phabricator.kde.org/D29201 To: alex, #plasma, ngraham, meven, broulik Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D29201: KCMUtils: Add option to append service file to list of arguments
apol added a comment. I have the feeling there's a missing piece here. Would it make sense to always pass it? Would it regress in any way? At the very least, on the API documentation, explain why one would want that. REPOSITORY R295 KCMUtils REVISION DETAIL https://phabricator.kde.org/D29201 To: alex, #plasma, ngraham, meven, broulik Cc: apol, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D29201: KCMUtils: Add option to append service file to list of arguments
alex added a comment. > Would it make sense to always pass it? Would it regress in any way? It would make sense, but it would change the existing behavior. If you use the method setConfigurationArguments then you expect your argument list to be the list of arguments you specified + an entry for the metadata. So adding an entry by default could break things. REPOSITORY R295 KCMUtils REVISION DETAIL https://phabricator.kde.org/D29201 To: alex, #plasma, ngraham, meven, broulik Cc: apol, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D29201: KCMUtils: Add option to append service file to list of arguments
mart added a comment. indeed, a bit more documentation then go for it REPOSITORY R295 KCMUtils REVISION DETAIL https://phabricator.kde.org/D29201 To: alex, #plasma, ngraham, meven, broulik Cc: mart, apol, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D29201: KCMUtils: Add option to append service file to list of arguments
alex updated this revision to Diff 81855. alex added a comment. Improve documentation Is that what you had in mind :-) ? And should I maybe add a comment to make this the default in KF6? REPOSITORY R295 KCMUtils CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D29201?vs=81799&id=81855 BRANCH service_path_append (branched from master) REVISION DETAIL https://phabricator.kde.org/D29201 AFFECTED FILES src/kpluginselector.cpp src/kpluginselector.h src/kpluginselector_p.h To: alex, #plasma, ngraham, meven, broulik Cc: mart, apol, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D29201: KCMUtils: Add option to append service file to list of arguments
alex updated this revision to Diff 81856. alex added a comment. Linebreak REPOSITORY R295 KCMUtils CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D29201?vs=81855&id=81856 BRANCH service_path_append (branched from master) REVISION DETAIL https://phabricator.kde.org/D29201 AFFECTED FILES src/kpluginselector.cpp src/kpluginselector.h src/kpluginselector_p.h To: alex, #plasma, ngraham, meven, broulik Cc: mart, apol, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D29201: KCMUtils: Add option to append service file to list of arguments
alex added a reviewer: mart. REPOSITORY R295 KCMUtils REVISION DETAIL https://phabricator.kde.org/D29201 To: alex, #plasma, ngraham, meven, broulik, mart Cc: mart, apol, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D29201: KCMUtils: Add option to append service file to list of arguments
alex updated this revision to Diff 81873. alex added a comment. Increase @since to 5.71 REPOSITORY R295 KCMUtils CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D29201?vs=81856&id=81873 BRANCH service_path_append (branched from master) REVISION DETAIL https://phabricator.kde.org/D29201 AFFECTED FILES src/kpluginselector.cpp src/kpluginselector.h src/kpluginselector_p.h To: alex, #plasma, ngraham, meven, broulik, mart Cc: mart, apol, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D29201: KCMUtils: Add option to append service file to list of arguments
alex added a comment. Friendly Ping 😃 REPOSITORY R295 KCMUtils REVISION DETAIL https://phabricator.kde.org/D29201 To: alex, #plasma, ngraham, meven, broulik, mart Cc: mart, apol, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D29201: KCMUtils: Add option to append service file to list of arguments
meven added inline comments.
INLINE COMMENTS
> kpluginselector.cpp:855
> +}
> +KCModuleProxy *currentModuleProxy = new
> KCModuleProxy(moduleInfo, moduleProxyParentWidget, arguments);
> if (currentModuleProxy->realModule()) {
Adding a fileName field to KCModuleProxy would make more sense to me, and do it
by default.
Plus KCModuleProxy has already access to the fileName since it receives
moduleInfo.
But I am not a specialist.
REPOSITORY
R295 KCMUtils
REVISION DETAIL
https://phabricator.kde.org/D29201
To: alex, #plasma, ngraham, meven, broulik, mart
Cc: mart, apol, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham,
bruns
D29201: KCMUtils: Add option to append service file to list of arguments
alex added inline comments. INLINE COMMENTS > meven wrote in kpluginselector.cpp:855 > Adding a fileName field to KCModuleProxy would make more sense to me, and do > it by default. > Plus KCModuleProxy has already access to the fileName since it receives > moduleInfo. > But I am not a specialist. You could add a field to the KCModule class (thats where you ultimately want to access the fileName). But then the downside is that you don't have them as a constructor argument. REPOSITORY R295 KCMUtils REVISION DETAIL https://phabricator.kde.org/D29201 To: alex, #plasma, ngraham, meven, broulik, mart Cc: mart, apol, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
