Provide a telemetrics kcm module for Plasma
ClosedPublic

Authored by apol on Sep 16 2019, 10:40 PM.

Details

Summary

It offers 2 main features:

  • Configure the system-wide KUserFeedback kill-switch
  • Provide a setting for how much information we want our Plasma to be sending
Test Plan

Tested it together with Discover, works fine.

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.
There are a very large number of changes, so older changes are hidden. Show Older Changes

Should we make it already a "Privacy" KCM just in case?

ognarb added a subscriber: ognarb.Sep 17 2019, 7:59 AM
ognarb added inline comments.
kcms/feedback/package/contents/ui/main.qml
47

strange </p> tag in the text

Should we make it already a "Privacy" KCM just in case?

I'm not convinced about that. "Privacy" is not something you need to enable (it's on by default), and it's not something we want to suggest you have to disable. "Telemetry" is quite technical and has some negative connotations, phrasing this positive along the lines of "Feedback", "Contribute Feedback", etc seems better to me.

kcms/CMakeLists.txt
2

Needs to be conditional on KUserFeedback presence I guess?

apol added a comment.Sep 17 2019, 10:20 AM

Should we make it already a "Privacy" KCM just in case?

No, I don't think this will undermine our users' privacy. If it did, I (we?) wouldn't be adding telemetry at all.
This is so they can keep in control of what their system sends.

Probably could do with a screenshot as I expect this will need quite thorough UI iterations to get the wording spot on.

kcms/feedback/feedback.cpp
17

Is including kdeglobals here deliberate?

KUserFeedback won't read it..but maybe it makes sense?

23

Did I write that line? I genuinely can't remember :/

In any case, this should work for having a syadmin force disable telemetrics by kiosk which I thought was the most likely case.

It doesn't cover the sysadmin force enabling it - but that probably isn't too useful if we don't have a configurable server anyway

apol marked an inline comment as done.Sep 17 2019, 12:16 PM
apol added inline comments.
kcms/feedback/feedback.cpp
17

This setting needs to be read by every Plasma component, these can check through kconfig.
See D5961.

23

Yes, you did.
I also wondered about that, I agree we should respect KUserfeedback's default. Maybe it's something we can delegate onto KUserfeedback somehow.

apol marked 4 inline comments as done.Sep 17 2019, 12:27 PM
apol marked an inline comment as done.

Screenshot, I thought I'd uploaded it already.

Please ensure that the KCM links to https://kde.org/privacypolicy-apps.php.
It would also be appreciated if those implementing or modifying Telemetry in their applications read this first as well.

Where you need to state what the KCM is for, it would be appreciated if the text from that page could be reused where possible for the sake of consistency.

apol updated this revision to Diff 66291.Sep 17 2019, 12:36 PM

Include the policy, KUserfeedback decides the setting default, polishing.

apol updated this revision to Diff 66292.Sep 17 2019, 12:38 PM

Copyrights

I think the hard part of the design is conveying that the top option is a global killswitch that will affect all kuserfeedback apps, whilst the bottom combo is the setting for all plasma things.

Maybe some flat groupboxes, so it's

Global settings

Telemetry enabled

  • Offer telemetry feedback controls in apps

Plasma

We make Plasma for you. You can help us improve it by contributing information on how you use it....blah blah

Plasma Settings: [ comboBoxhere ]

kcms/feedback/package/contents/ui/main.qml
34

Generally we only want a label if it's deliberately spanning and not associated with a control.

Otherwise, we want:

Kirigami.FormData.label: i18n("Blah blah:")

After opening the KCM for the first time, the checkbox is checked. I thought we were going to make this opt-in? If so, it needs to be unchecked by default.

For the message, I would phrase it like this:

We make Plasma for you. You can help us improve it by contributing information on how you use it. This allows us to focus on things that matter to you.

Contributing usage information is optional and entirely anonymous. It will not associate the data with any kind of unique identifier, and will never track the documents you open, the websites you visit, or any other kind of personal information.

When we say, "anonymous", we mean it! For more information, see https://kde.org/privacypolicy-apps.php

In terms of the UI, I would lay it out like this:

<introductory block of text>

[] Send usage information

Apps that collect information
      Plasma: [ Combo box ]
    Discover: [ Combo box ]
     Dolphin: [ Combo box ]
    Etc (this list should be automatically populated with each app that uses KUserFeedback)

Also, the default item for each combobox shouldn't be "No statistics". If you've opted in, you clearly want to send some information.

Finally, the text in the combobox needs to be much clearer, or we need to add clarifying labels somewhere. I don't know what the difference is between "Basic usage information" and "detailed usage information", for example. The fact that it doesn't say specifically what will be collected is disconcerting from a privacy point of view. I think we need to be really clear here about what's getting sent to boost confidence that we mean what we say and entice people to turn it on.

kcms/feedback/kcm_feedback.desktop
12

This isn't a regional setting. I'd put it at the top-level in the Personalization section, or else maybe make a new Privacy section and put it in there like Kai alluded to.

Also it would be nice if the screenshot depicting the current UI lived in the Test Plan section (and got updatd that with each iteration) so it doesn't get buried in the comment thread.

apol updated this revision to Diff 66298.Sep 17 2019, 2:36 PM

Improve how we display plasma settings

apol edited the test plan for this revision. (Show Details)Sep 17 2019, 2:37 PM
apol added a comment.Sep 17 2019, 2:48 PM

Hey @ngraham, I introduced many of your points to the proposal. Here's some thoughts on the rest:

Apps that collect information

I don't think we can do this, at least not right now, easily. Non-Plasma products (dolphin, krita) will need to have their own KCM for now, unless we abstract it at some point, but that's something we can do later down the line, they will need to have their own KCM within the application after all, otherwise it wouldn't be possible to control their telemetry outside of Plasma.

The fact that it doesn't say specifically what will be collected is disconcerting from a privacy point of view.

We don't really have this information right now. I agree we could try to do better in this front.

If this KCM is purely for Plasma, then we kind of have a problem, because we're putting the global switch in it, but it also has to show Plasma-specific stuff.

If you change the text that says "We make Plasma for you" into "We make software for you", then the text can apply to all KDE software and we can move it out of the Plasma section and above everything else as first proposed.

kcms/feedback/feedback.cpp
42

I would avoid using the word "Telemetry". It has negative connotations. Maybe "Feedback" instead.

kcms/feedback/package/contents/ui/main.qml
46

This is aligned in a weird way. Let's come up with a better way to communicate this information. Maybe we could add it to the text block.

58

Ew gross, frames

aspotashev added inline comments.
kcms/feedback/feedback.cpp
42

"telemetry" is also ambiguous. These are also telemetries:

  1. Ksysguard daemon,
  2. GPS and altitude measurements being received from a UAV.
apol marked 2 inline comments as done.Sep 17 2019, 9:55 PM
apol added inline comments.
kcms/feedback/feedback.cpp
42

He was suggesting moving away from "telemetry". Addressed, unsure where this line appears on the UI.

apol updated this revision to Diff 66342.Sep 17 2019, 9:55 PM
apol marked an inline comment as done.

Don't use "telemetry"

apol updated this revision to Diff 66351.Sep 17 2019, 11:50 PM

Include a provider for plasmashell

ngraham requested changes to this revision.Sep 18 2019, 3:43 PM

So this needs UI changes to be shippable:

  1. Don't ever use frames in QML, they look terrible.
  1. Make the layout more like this:
<introductory text block; use the text I suggested in https://phabricator.kde.org/D24011#533243>

Feedback: [] Allow KDE software to collect anomymous usage information

Plasma:   [ combobox of options ]
          <label that explains what the currently-selected option in the combobox actually means>
This revision now requires changes to proceed.Sep 18 2019, 3:43 PM

Additionally:

  • Make the KCM visible in System Settings, I would say as a new top-level category under Personalization
  • Make the name/title match between the .desktop file and the KAboutData
  • Make the comment/description match between the .desktop file and the KAboutData
  • Needs its own icon. Requested one here: https://bugs.kde.org/show_bug.cgi?id=412029
apol updated this revision to Diff 66467.Sep 19 2019, 2:11 PM

Iterated with Nate's feedback

Thanks, looking better.

I don't see explanatory text below the combobox yet; looks like that part isn't quite working.

Also, suppose I want to contribute both usage and system information, how do I do that? The wording in the combobox implies that I can only contribute one or the other. If there are multiple pieces of information that it would make sense to send, then a combobox isn't the right control and we'll need to use radio buttons or checkboxes--or else we'll need to re-word the options so that they reveal the full set of what will be included.

kcms/feedback/feedback.cpp
42

Title should be the same as the name i.e. "User Feedback"

43

Description should be "Configure user feedback settings"

kcms/feedback/kcm_feedback.desktop
15

Add Comment=Configure user feedback settings

kcms/feedback/package/contents/ui/main.qml
24

"as QQC2"

42
  1. Since this is no longer just about Plasma, maybe the first sentence should say, "We make software for you".
  1. Use kuit formatting for newlines: xi18nc("@info", "Line 1<nl/>Line 2<nl/>Line 3<nl/>etc.");
apol marked 4 inline comments as done.Sep 19 2019, 3:32 PM
apol updated this revision to Diff 66475.Sep 19 2019, 3:32 PM

Polish

Code for plasmashell part is fine. Ship it on that part.
Might be nicer as an extra file outside ShellCorona which is already way too big. Especially when we add in more metrics.

shell/shellcorona.cpp
95

That's a nice idea, I like that.

apol updated this revision to Diff 67130.Tue, Oct 1, 1:13 PM

move panelsourcecount into a separate file

apol updated this revision to Diff 67184.Wed, Oct 2, 12:51 PM

Better looking margins

apol edited the test plan for this revision. (Show Details)Wed, Oct 2, 12:56 PM
ngraham requested changes to this revision.Wed, Oct 2, 1:27 PM

I would remove the big text block from the form layout, since this makes it look off-center. Also it needs a bigger margin below it. See also the following inline comments:

kcms/feedback/package/contents/ui/main.qml
42

#1 isn't done yet (it still talks about Plasma, not KDE software more generally)

69

These strings still need to be re-worded, and possibly collapsed, and possible the whole UI needs to be re-thought. What if I want to submit detailed system information but only basic usage information? Maybe we need two combo boxes, one for system info and one for usage info.

90

This label never shows up for me, no matter which entry is selected in the combo box.

This revision now requires changes to proceed.Wed, Oct 2, 1:27 PM
apol added inline comments.Wed, Oct 2, 1:42 PM
kcms/feedback/package/contents/ui/main.qml
42

But it's not true, only Plasma components will follow the setting. Only the checkbox will be used elsewhere. (hence the old ugly frame).

69

That's not how it's implemented in KUserfeedback.

enum TelemetryMode {
        NoTelemetry, ///< Transmit no data at all.
        BasicSystemInformation = 0x10, ///< Transmit basic information about the system.
        BasicUsageStatistics = 0x20, ///< Transmit basic usage statistics.
        DetailedSystemInformation = 0x30, ///< Transmit detailed system information.
        DetailedUsageStatistics = 0x40, ///< Transmit detailed usage statistics.
    };

DetailedSystemInformation = BasicUsageStatistics+BasicSystemInformation

And I don't think it's wrong. It's about making it easy for users to choose to help us learn about their behaviour.

ngraham accepted this revision.Wed, Oct 2, 1:56 PM

Okay, let's land this now and I'll send a follow-up patch to tweak the UI.

This revision is now accepted and ready to land.Wed, Oct 2, 1:56 PM
This revision was automatically updated to reflect the committed changes.
GB_2 added a subscriber: GB_2.Sat, Oct 12, 2:55 PM

I have updated two PCs and on both telemetry was on by default...

Actually on or just the config saying it was on?

Please upload your ~/.config/KDE/UserFeedback.conf to a bug report.