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
Details
- Reviewers
vkrause davidedmundson ngraham - Group Reviewers
Plasma - Commits
- R120:2dd40cfb21b4: feedback kcm: Make it about Plasma
Diff Detail
Diff Detail
- Repository
- R120 Plasma Workspace
- Branch
- nokillswitch
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 17825 Build 17843: arc lint + arc unit
Comment Actions
+++
| 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? | |
Comment Actions
Indeed, this is much nicer.
The explanatory text for "Disabled" appears to duplicate some of the text above it though:
Comment Actions
Ok, now I see this:
Is that caused by a change here, or did something truly change on my system in the last hour?
Comment Actions
It's changed in the last iteration of the patch. You probably enabled the kill switch on the present version of the kcm.
Comment Actions
Yeah, I did. So now how do I turn it off? 😂 Is there a CLI tool we can mention in the message, maybe?
Comment Actions
Yay, that update fixed the issue, phew!
However the strings are still a little garbled:
Comment Actions
The introductory "We make plasma for you..." text is still inappropriately shown in the explanatory text for the Disabled state rather than on top:
Comment Actions
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
Comment Actions
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. | |
Comment Actions
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.