Adoption of KUserFeedback for Discover
ClosedPublic

Authored by apol on May 24 2017, 3:18 PM.

Details

Summary

Creates components so it can be integrated in any Kirigami application though.
Haven't tested custom information providers yet, need to get hold of a server
first to see how it will play out.

Depends on D24010
Depends on D24011

Diff Detail

Repository
R134 Discover Software Store
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
apol created this revision.May 24 2017, 3:18 PM
Restricted Application added a project: Plasma. · View Herald TranscriptMay 24 2017, 3:18 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
apol updated this revision to Diff 15217.Jun 6 2017, 12:58 PM

Adapt to changes in kuserfeedback, make sure more sources are sent

apol updated this revision to Diff 15366.Jun 11 2017, 11:56 PM

Adapt to changes kuserfeedback

apol updated this revision to Diff 15498.Jun 16 2017, 3:34 PM
  • Expose the applicationSourceName to KUserFeedback
apol updated this revision to Diff 15500.Jun 16 2017, 3:38 PM
  • Include a KUserFeedback schema file
vkrause added inline comments.Jun 19 2017, 12:22 PM
CMakeLists.txt
29

Hm, looks like I forgot to rename this to KUserFeedback... Should probably done to be consistent with naming elsewhere. I'll look into it, and sorry for yet another SC change.

discover/qml/DiscoverWindow.qml
118

You probably want to set submissionInterval to a fixed positive value for it to actually send data automatically. For best results, pick one that matches the time aggregation modes in UserFeedbackConsole (1, 7 or 30).

123

Note that this means you have basic telemetry enabled by default. If you want to let the user control this entirely, just drop the line. Same for surveyInterval above.

137

For this to trigger you will need to set some of the encouragement related properties on Provider, by default it's off.

apol updated this revision to Diff 15686.Jun 21 2017, 12:09 PM
apol marked 4 inline comments as done.
  • Expose the applicationSourceName to KUserFeedback
  • Include a KUserFeedback schema file
  • Address Volker's comments
apol updated this revision to Diff 32887.Apr 23 2018, 2:20 PM

Rebase to master

apol updated this revision to Diff 65967.Sep 13 2019, 9:56 AM

Rebase

apol updated this revision to Diff 65968.Sep 13 2019, 9:58 AM

Rename property

apol updated this revision to Diff 66273.Sep 16 2019, 10:41 PM

Remove the dialog and read the settings globally for Plasma.
Moved the feedback settings to the About page.

+1

discover/qml/DiscoverWindow.qml
71
This comment was removed by ngraham.

This doesn't build for me:

[ 82%] Building CXX object discover/CMakeFiles/plasma-discover.dir/plasmauserfeedback.cpp.o
In file included from /home/nate/kde/build/discover/discover/plasmauserfeedback.cpp:4:
/home/nate/kde/build/discover/discover/plasmauserfeedback.h:6:10: fatal error: KUserFeedback/Provider: No such file or directory
    6 | #include <KUserFeedback/Provider>
      |          ^~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.

I have the latest KUserFeedback and the KCM listed as a dependency installed.

Also...

Creates components so it can be integrated in any Kirigami application though.

Shouldn't these be in Kirigami, then?

apol added a comment.Sep 17 2019, 2:37 PM

Shouldn't these be in Kirigami, then?

Let's see what it ends up looking like, we've already moved half this patch to plasma-workspace.

apol updated this revision to Diff 66322.Sep 17 2019, 6:12 PM

Use KDE's server

apol retitled this revision from PoC: Generic adoption of KUserFeedback for Discover to Adoption of KUserFeedback for Discover.Wed, Oct 2, 1:55 PM
apol updated this revision to Diff 67200.Wed, Oct 2, 3:27 PM

Fix build

Now it fails to build in a different way. :)

In file included from /home/nate/kde/build/discover/discover/plasmauserfeedback.cpp:30:
/home/nate/kde/build/discover/discover/plasmauserfeedback.moc: In static member function ‘static void PlasmaUserFeedback::qt_static_metacall(QObject*, QMetaObject::Call, int, void**)’:
/home/nate/kde/build/discover/discover/plasmauserfeedback.moc:99:52: error: ‘class PlasmaUserFeedback’ has no member named ‘FeedbackLevel’; did you mean ‘feedbackLevel’?
   99 |         case 0: *reinterpret_cast< int*>(_v) = _t->FeedbackLevel(); break;
      |                                                    ^~~~~~~~~~~~~
      |                                                    feedbackLevel

Actually that's fixed by D24010.

ngraham requested changes to this revision.Wed, Oct 2, 4:19 PM

So this patch adds a semi-hidden menu item on the About page that opens the User Feedback KCM in a KCMShell window. I think this needs a better UX:

  1. We finally need a real Settings page/window for Discover if we're going to be adding more user-configurable settings like this one. The About page isn't an appropriate place for this. Maybe we should Change Sources to Settings and add it there.
  2. Shouldn't the UI here be Discover-specific? The global KCM doesn't seem relevant since it talks about turning on or off telemetry for all KDE apps and allows configuring stuff for Plasma specifically. Strictly speaking, Discover isn't a part of Plasma. And from a user perspective, Discover is just an app among other apps.
This revision now requires changes to proceed.Wed, Oct 2, 4:19 PM
apol added a comment.Wed, Oct 2, 4:35 PM

So this patch adds a semi-hidden menu item on the About page that opens the User Feedback KCM in a KCMShell window. I think this needs a better UX:

  1. We finally need a real Settings page/window for Discover if we're going to be adding more user-configurable settings like this one. The About page isn't an appropriate place for this. Maybe we should Change Sources to Settings and add it there.

True, maybe we should re-rename Sources to settings again.

  1. Shouldn't the UI here be Discover-specific? The global KCM doesn't seem relevant since it talks about turning on or off telemetry for all KDE apps and allows configuring stuff for Plasma specifically. Strictly speaking, Discover isn't a part of Plasma. And from a user perspective, Discover is just an app among other apps.

Discover is part of plasma and the information shared should be centrally configured.

In D5961#540961, @apol wrote:

Discover is part of plasma and the information shared should be centrally configured.

Then we need for the feedback KCM to mention Discover as a source of information that will be sent. Maybe this needs to be changed in KUserFeedback? Right now the explanatory text that comes from the framework only mentions System Settings). It's not obvious to users that Discover is a part of Plasma. So we need to make the connection obvious.

apol updated this revision to Diff 67423.Mon, Oct 7, 11:11 AM

Move options to Sources, rename Sources back to Settings

So close! The Settings page still says "Sources" in its header rather than "Settings."

Also it might be nice to have the actions visible on the toolbar when there's room rather than always having them all in an overflow menu. Other than that, looks good.

apol added a comment.Mon, Oct 7, 4:47 PM

So close! The Settings page still says "Sources" in its header rather than "Settings."

Also it might be nice to have the actions visible on the toolbar when there's room rather than always having them all in an overflow menu. Other than that, looks good.

That's kirigami's decision to hide, and given they don't have an icon, I'd say it makes sense as is.
Note the user will have reminders showing the config dialog.

apol updated this revision to Diff 67441.Mon, Oct 7, 4:47 PM

oops sources page title

Clicking "Submit Feedback" produces the following:

org.kde.UserFeedback: failed to submit user feedback: "Error transferring https://telemetry.kde.org/receiver/submit/org.kde.discover - server replied: Not Found" "\nUnknown product." . Calling scheduleNextSubmission with minTime 4 minutes

Is this a bug in the patch or in the server?

discover/qml/DiscoverWindow.qml
53
  1. What information? I'd change it to say "Submit usage statistics" (or whatever).
  2. When you click on this, nothing visibly happens in the UI. I would recommend showing a passiveNotification saying some kind of message acknowledging that something just happened.
60

Actions need to start with a verb:

  • Change feedback settings...
  • Configure feedback settings...

etc.

apol marked 2 inline comments as done.Mon, Oct 7, 5:39 PM
apol updated this revision to Diff 67448.Mon, Oct 7, 5:39 PM

++verbosity

WRT the server error, I'd like to have the patch in before configuring the server to acknowledge Discover.

ngraham accepted this revision.Mon, Oct 7, 5:46 PM

Shipit!

discover/qml/DiscoverWindow.qml
54

to better understand its users -> so we can better understand our users

This revision is now accepted and ready to land.Mon, Oct 7, 5:46 PM
This revision was automatically updated to reflect the committed changes.