D15892: [Devicenotifications Engine] Keep at most one notification per UDI
This revision was automatically updated to reflect the committed changes. bruns marked an inline comment as done. Closed by commit R120:17380886d9b8: [Devicenotifications Engine] Keep at most one notification per UDI (authored by bruns). CHANGED PRIOR TO COMMIT https://phabricator.kde.org/D15892?vs=42702=42767#toc REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D15892?vs=42702=42767 REVISION DETAIL https://phabricator.kde.org/D15892 AFFECTED FILES dataengines/devicenotifications/devicenotificationsengine.cpp dataengines/devicenotifications/devicenotificationsengine.h dataengines/devicenotifications/ksolidnotify.cpp dataengines/devicenotifications/ksolidnotify.h To: bruns, #frameworks, broulik Cc: broulik, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D15892: [Devicenotifications Engine] Keep at most one notification per UDI
broulik accepted this revision. This revision is now accepted and ready to land. REPOSITORY R120 Plasma Workspace BRANCH device_notifier_qml_fix2 REVISION DETAIL https://phabricator.kde.org/D15892 To: bruns, #frameworks, broulik Cc: broulik, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D15892: [Devicenotifications Engine] Keep at most one notification per UDI
bruns marked an inline comment as done. bruns added inline comments. INLINE COMMENTS > broulik wrote in devicenotificationsengine.cpp:39 > This potentially breaks applets relying on the structure of that name? Not > sure how big of an issue this is as far as "API promise" we give for > dataengines but perhaps at least keep the `notification` part first The only user I could find is the device notifier, which relies on the contents of the container only for matching, and does not require any specific format for the source name. Preferably we had some API documentation for the dataengines ... > broulik wrote in devicenotificationsengine.h:26 > Unused? yes, leftover from a different approach. > devicenotificationsengine.h:43 > void notify(Solid::ErrorType solidError, const QString& error, const > QString& errorDetails, const QString ); > +void clearNotification(const QString ); > whitespace. > broulik wrote in ksolidnotify.cpp:152 > Move this into the relevant `case` below, please I considered this and did not because of the early return. All the other `case`s just do a break and have a common return statement (more correctly, an `emit notify(...)`), this one differs. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D15892 To: bruns, #frameworks Cc: broulik, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D15892: [Devicenotifications Engine] Keep at most one notification per UDI
broulik added a comment.
I think this makes sense
INLINE COMMENTS
> devicenotificationsengine.cpp:39
> {
> -const QString source = QStringLiteral("notification %1").arg(m_id++);
> +const QString source = QStringLiteral("%1 notification").arg(udi);
>
This potentially breaks applets relying on the structure of that name? Not sure
how big of an issue this is as far as "API promise" we give for dataengines but
perhaps at least keep the `notification` part first
> devicenotificationsengine.h:26
> #include
> +#include
>
Unused?
> ksolidnotify.cpp:152
> {
> +if ((error == Solid::ErrorType::NoError) && (type ==
> SolidReplyType::Setup)) {
> +emit clearNotification(udi);
Move this into the relevant `case` below, please
REPOSITORY
R120 Plasma Workspace
REVISION DETAIL
https://phabricator.kde.org/D15892
To: bruns, #frameworks
Cc: broulik, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed,
jensreuterberg, abetts, sebas, apol, mart
D15892: [Devicenotifications Engine] Keep at most one notification per UDI
bruns created this revision. bruns added a reviewer: Frameworks. Herald added a project: Plasma. Herald added a subscriber: plasma-devel. bruns requested review of this revision. REVISION SUMMARY Each notification was created as new datasource, and was newer removed as long as the engine exists. This is especially bad for long living applets like the device notifier. As there can only be one notification per device (the last error state, or none), use the UDI as source name and update the contents. Also cancel (remove source) an old notifications in case of a successful setup, otherwise old error messages are shown in the device notifier. TEST PLAN in Dolphin, select "Release" (context menu) for a mounted CD -> Notification in device notifier appears "Device can be safely removed" remount without ejecting -> Notification message is removed, device shows "mounted" emblem. Without the change, the message stayed and the device kept the notification emblem "(!)". REPOSITORY R120 Plasma Workspace BRANCH device_notifier_qml_fix2 REVISION DETAIL https://phabricator.kde.org/D15892 AFFECTED FILES dataengines/devicenotifications/devicenotificationsengine.cpp dataengines/devicenotifications/devicenotificationsengine.h dataengines/devicenotifications/ksolidnotify.cpp dataengines/devicenotifications/ksolidnotify.h To: bruns, #frameworks Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
