optionally show a history of notifications
ClosedPublic

Authored by progwolff on Aug 12 2017, 10:14 AM.

Details

Summary

Bug 378032.
With these changes, notifications can be configured to always persist in the notifications applet.

Todo: filter by application, see https://bugs.kde.org/show_bug.cgi?id=378032#c27

Test Plan

Open the notification plasmoid's settings. Check 'Show a history of notifications'. Trigger some notifications. They should show up in the applet.

Diff Detail

Repository
R120 Plasma Workspace
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
progwolff created this revision.Aug 12 2017, 10:14 AM
Restricted Application added a project: Plasma. · View Herald TranscriptAug 12 2017, 10:14 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript

Thanks for looking into this.. first round of comments.

applets/notifications/package/contents/ui/Notifications.qml
259

?

287

How does this differ to the persistent delegate?

Can we share most of this?

applets/notifications/package/contents/ui/main.qml
56–57

This is now a somewhat confusing name as it's not the total.

Then you have the real total under a new name property below.

progwolff marked 2 inline comments as done.Aug 12 2017, 11:57 AM
progwolff added inline comments.
applets/notifications/package/contents/ui/Notifications.qml
259

sorry, missed to revert this...

287

It's almost the same. Only the models differ (NotificationsModel is used in NotificationDelegate, NotificationsHistoryModel is used in NotificationHistoryDelegate).

I also thought it might be a good idea to use a seperate delegates here, so that in the future history items might have a different visual representation than persistent notifications.

If this is not wanted, we could probably make the model a property of NotificationDelegate.

progwolff marked 2 inline comments as done.Aug 12 2017, 11:57 AM
progwolff updated this revision to Diff 18050.Aug 12 2017, 11:57 AM
  • uncomment unintentionally commented line
  • rename counters
progwolff updated this revision to Diff 18051.Aug 12 2017, 11:58 AM
  • fix property name

There's a visual quirk I'd like fixing.

Each delegate has a line on the bottom.

However, there's no line between the persistent notifications and the history, so it looks weird.
Maybe a heading "history" would also work.

Also in your last change to my suggestion you've made it so that non persistent items are included in the number in the compact representation.
I don't know if we want that or not.

progwolff updated this revision to Diff 18075.Aug 13 2017, 8:21 AM
  • show number of active items in compact representation, excluding history
  • show heading above history
  • don't show actions in history

Could you please attach some screenshots and maybe also add the VDG to the review?

progwolff added a reviewer: VDG.EditedAug 13 2017, 9:01 AM

Could you please attach some screenshots and maybe also add the VDG to the review?

mart added a subscriber: mart.Aug 14 2017, 9:47 AM
mart added inline comments.
applets/notifications/package/contents/ui/NotificationHistoryDelegate.qml
27 ↗(On Diff #18075)

Why a different delegate for this? all the delegates, persistent or history should be the same

@notmart, click the "show older changes" and see reply to my first comment asking the same thing.

mart added inline comments.Aug 14 2017, 9:57 AM
applets/notifications/package/contents/ui/Notifications.qml
287

I don't think we would ever want a different visual representation for them, i would consider that a bug, as the thing looks quite busy already.

they can be the same file with just a different model property assigned

progwolff updated this revision to Diff 18134.Aug 14 2017, 12:57 PM
  • use the same delegate for persistent notifications and history items
progwolff updated this revision to Diff 18135.Aug 14 2017, 1:08 PM
  • simplify object copy

Cool, do you have commit access?

applets/notifications/package/contents/ui/Notifications.qml
92

*is

progwolff updated this revision to Diff 18139.Aug 14 2017, 1:46 PM
  • fix typo

Cool, do you have commit access?

I have commit access, yes

progwolff marked 2 inline comments as done.Aug 15 2017, 10:41 AM

so this can be considered accepted?

This revision is now accepted and ready to land.Aug 15 2017, 12:00 PM
This revision was automatically updated to reflect the committed changes.