D7256: emit previously shown notifications after resume from idle
progwolff planned changes to this revision. progwolff added a comment. I totally aggree with @davidedmundson's objections. I will think about your comments and hand in another revision when I find some time. Things I will consider: - Don't re-emit persistent notifications. They will not close anyway. - Make sure that notifications don't show up in history twice - Don't show actions for re-emitted notifications - Make sure we still comply with the protocol - Filter by application? -> Ignore e.g. media player's notifications - Coding style (thanks, @sebas) As https://phabricator.kde.org/D7271 has been accepted, this patch is secondary and I will take some time to overthink this. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D7256 To: progwolff, #plasma, mart Cc: sebas, davidedmundson, mart, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, lukas
D7256: emit previously shown notifications after resume from idle
sebas added a comment.
Some coding style suggestions
INLINE COMMENTS
> notificationsengine.cpp:87
> +connect(KIdleTime::instance(), static_cast (KIdleTime::*)(int)>(::timeoutReached), this, [this, idleId](int
> id){
> +if(id == idleId)
> +m_history.clear();
braces around the if block, please, space after if
> notificationsengine.cpp:401
> +{
> +for(const NotificationData& d : m_history) {
> +Notify(d.appName, d.id, d.appIcon, d.summary, d.body, d.actions,
> d.hints, d.timeout);
for (
(missing whitespace)
REPOSITORY
R120 Plasma Workspace
BRANCH
master
REVISION DETAIL
https://phabricator.kde.org/D7256
To: progwolff, #plasma, mart
Cc: sebas, davidedmundson, mart, plasma-devel, ZrenBot, progwolff, lesliezhai,
ali-mohamed, jensreuterberg, abetts, apol, lukas
D7256: emit previously shown notifications after resume from idle
davidedmundson added a comment. edit. 3 things. INLINE COMMENTS > notificationsengine.cpp:419 > removeSource(source); > emit NotificationClosed(id, closeReason); > } We need to be careful to not emit this twice. That would be a protocol error. I think we'll have emitted it when the first one timed out. REPOSITORY R120 Plasma Workspace BRANCH master REVISION DETAIL https://phabricator.kde.org/D7256 To: progwolff, #plasma, mart Cc: davidedmundson, mart, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, lukas
D7256: emit previously shown notifications after resume from idle
davidedmundson added a comment. I'm not particularly convinced. You watch a movie, you get some notifications, but ignore them. You move the mouse, you get told now completely out-of-date info. Also any notification created by KNotification with actions simply won't work. The client side stops listening for replies after 30 seconds. Two things definitely need fixing: - with this, and your other patch - you'll get history twice, worse as your history shows a timestamp. - persistent notifications should definitely not be re-emitted as the original will still be there. REPOSITORY R120 Plasma Workspace BRANCH master REVISION DETAIL https://phabricator.kde.org/D7256 To: progwolff, #plasma, mart Cc: davidedmundson, mart, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, lukas
D7256: emit previously shown notifications after resume from idle
mart accepted this revision. mart added a comment. This revision is now accepted and ready to land. i like it! REPOSITORY R120 Plasma Workspace BRANCH master REVISION DETAIL https://phabricator.kde.org/D7256 To: progwolff, #plasma, mart Cc: mart, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, lukas
D7256: emit previously shown notifications after resume from idle
progwolff created this revision. Restricted Application added a project: Plasma. Restricted Application added a subscriber: plasma-devel. REVISION SUMMARY This is an attempt to improve the behaviour of notifications that are triggered while the session is idle. This is related to (but not fixing) Bug 378032. Notifications that are triggered while the session is idle might be missed by the user. With these changes, notifications will be shown again when the session resumes from idle. This behaviour might be controversial. A history of notifications seems to be unwanted. I tried to find a way to make missing important notifications less likely while not annoying the user too much. TEST PLAN Stay away from the keyboard and the mouse for more than 30 seconds. Then remotely trigger a notification and wait until it disappears. It should show up again once you move the mouse or hit a key. REPOSITORY R120 Plasma Workspace BRANCH master REVISION DETAIL https://phabricator.kde.org/D7256 AFFECTED FILES dataengines/notifications/CMakeLists.txt dataengines/notifications/notificationsengine.cpp dataengines/notifications/notificationsengine.h To: progwolff, #plasma Cc: plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart, lukas
