Details
- Reviewers
vkrause ngraham - Group Reviewers
Plasma - Commits
- R134:f1f1597f4f2e: Adoption of KUserFeedback for Discover
Diff Detail
- Repository
- R134 Discover Software Store
- Branch
- arcpatch-D5961_1
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 16695 Build 16713: arc lint + arc unit
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. |
- Expose the applicationSourceName to KUserFeedback
- Include a KUserFeedback schema file
- Address Volker's comments
Remove the dialog and read the settings globally for Plasma.
Moved the feedback settings to the About page.
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?
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.
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
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:
- 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.
- 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.
True, maybe we should re-rename Sources to settings again.
- 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.
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.
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.
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 ↗ | (On Diff #66273) |
|
60 ↗ | (On Diff #66273) | Actions need to start with a verb:
etc. |
++verbosity
WRT the server error, I'd like to have the patch in before configuring the server to acknowledge Discover.
Shipit!
discover/qml/DiscoverWindow.qml | ||
---|---|---|
54 ↗ | (On Diff #66273) | to better understand its users -> so we can better understand our users |