Differential D7256

emit previously shown notifications after resume from idle
Changes PlannedPublic
Actions

Authored by progwolff on Aug 11 2017, 6:44 PM.

Details

Reviewers
mart
Group Reviewers
Plasma
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.

Diff Detail

Repository
R120 Plasma Workspace
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
progwolff created this revision.Aug 11 2017, 6:44 PM
Restricted Application added a project: Plasma. · View Herald TranscriptAug 11 2017, 6:44 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
mart accepted this revision.Aug 14 2017, 9:58 AM
mart added a subscriber: mart.

i like it!

This revision is now accepted and ready to land.Aug 14 2017, 9:58 AM
davidedmundson added a subscriber: davidedmundson.Aug 14 2017, 10:05 AM

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.
davidedmundson added a comment.Aug 14 2017, 10:20 AM

edit. 3 things.

dataengines/notifications/notificationsengine.cpp
419

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.

sebas added a subscriber: sebas.Aug 16 2017, 9:52 AM

Some coding style suggestions

dataengines/notifications/notificationsengine.cpp
87

braces around the if block, please, space after if

401

for (
(missing whitespace)

progwolff planned changes to this revision.Aug 16 2017, 10:16 AM

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 D7271 has been accepted, this patch is secondary and I will take some time to overthink this.

Revision Contents
Changeset List

Diff 18017

View Options

dataengines/notifications/CMakeLists.txt

Loading...
View Options

dataengines/notifications/notificationsengine.h

Loading...
View Options

dataengines/notifications/notificationsengine.cpp

Loading...