At the moment we were merging two different concepts into a same KCM and it was
too confusing.
Simplify making the kcm only about Plasma by removing the kill switch and
making sure the text only affects Plasma components.
Details
- Reviewers
vkrause davidedmundson ngraham - Group Reviewers
Plasma - Commits
- R120:2dd40cfb21b4: feedback kcm: Make it about Plasma
Diff Detail
- Repository
- R120 Plasma Workspace
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
+++
kcms/feedback/feedback.h | ||
---|---|---|
31 | Should we keep this readonly and grey out the whole UI if unset? Or better yet, is it possible to disable the entire KCM through some .desktop file trick? |
Indeed, this is much nicer.
The explanatory text for "Disabled" appears to duplicate some of the text above it though:
Ok, now I see this:
Is that caused by a change here, or did something truly change on my system in the last hour?
It's changed in the last iteration of the patch. You probably enabled the kill switch on the present version of the kcm.
Yeah, I did. So now how do I turn it off? ๐ Is there a CLI tool we can mention in the message, maybe?
Yay, that update fixed the issue, phew!
However the strings are still a little garbled:
The introductory "We make plasma for you..." text is still inappropriately shown in the explanatory text for the Disabled state rather than on top:
Oh, I see what you mean, this text is entirely coming from KUserFeedback.
Maybe it's something we can tackle there:
https://phabricator.kde.org/source/kuserfeedback/browse/master/src/provider/core/feedbackconfiguicontroller.cpp$187
I agree with Nate's comment, but lets get this in and then tackle that there.
I just one QML line that I don't follow, then lets ship it
kcms/feedback/package/contents/ui/main.qml | ||
---|---|---|
41โ43 | warning? It's not really an error exactly if it's configured. | |
112 | Option to consider with regards to Nate's comment: enabled: slider.value > 0 so it greys out really showing it's disabled, but still has the useful text. | |
116 | I don't understand this top line |
kcms/feedback/package/contents/ui/main.qml | ||
---|---|---|
116 | Yeah... feedbackController.telemetryDescription() function depends on applicationName, but since it's a function we don't have a way to notify the text changes. Maybe we could look into improving the API upstream, but it's the only reliable way I found to do this as is. |
I integrated David's idea to move this patch forward, but I think here we should look into move this text elsewhere:
https://cgit.kde.org/kuserfeedback.git/tree/src/provider/core/feedbackconfiguicontroller.cpp#n188
In this text we are talking about why to enable user feedback while on the other levels we are talking about which information is being sent.
This will need discussion with @vkrause and KUserFeedback.