Fix Notifications in Plasmoid
ClosedPublic

Authored by nicolasfella on Aug 8 2017, 7:39 PM.

Details

Summary

Fixed Issues mentioned in https://phabricator.kde.org/T6729
This patch also fixes a crash when the filetransfer of the icon fails

Test Plan

Receive a notification with a previously unknown icon -> icon in Plasmoid is displayed correctly
Receive a notification with an already existing id -> notification in Plasmoid is updated

Diff Detail

Repository
R224 KDE Connect
Lint
Lint Skipped
Unit
Unit Tests Skipped
nicolasfella created this revision.Aug 8 2017, 7:39 PM
apol requested changes to this revision.Aug 8 2017, 11:47 PM
apol added a subscriber: apol.
apol added inline comments.
plugins/notifications/notificationsdbusinterface.cpp
129

I'd say it's fine to keep these debug statements. They're showing an error after all and debug is very easy to filter out.

143

Can you elaborate on this? I'm not sure where it's from, maybe needs a comment?

This revision now requires changes to proceed.Aug 8 2017, 11:47 PM
nicolasfella added inline comments.Aug 9 2017, 12:09 AM
plugins/notifications/notificationsdbusinterface.cpp
129

These produce a lot of output which seems to be false alarms

143

I found this commented out in the code. The goal is to remove the notification from the plasmoid without deleting the noti object because we still want to use it

broulik added a subscriber: broulik.Aug 9 2017, 8:26 AM
broulik added inline comments.
plugins/notifications/notificationsdbusinterface.h
61

Please avoid a Boolean trap, use an enum of some sort instead. If you see a removeNotification("foo", false) you cannot tell at a glance what false means.

nicolasfella edited edge metadata.

Use enum instead of bool

apol accepted this revision.Aug 13 2017, 5:49 PM
apol added inline comments.
plugins/notifications/notificationsdbusinterface.h
43

Use full names, camel-cased:
KeepNotification, DestroyNotification

This revision is now accepted and ready to land.Aug 13 2017, 5:49 PM
This revision was automatically updated to reflect the committed changes.
nicolasfella reopened this revision.Oct 13 2017, 7:36 PM

This change was overridden by D7312. I discovered that when observing a bug I fixed months ago :D

This revision is now accepted and ready to land.Oct 13 2017, 7:36 PM

Why is it needed to remove and re-add the notification instead of just calling update?

Why is it needed to remove and re-add the notification instead of just calling update?

Because otherwise the plasmoid won't be updated

Isn't there something that we can emit so the model knows the data has been updated? @apol master of Qt models, do you know a solution to this problem?

Don't crash when the icon cannot be transferred.

apol added a comment.Oct 23 2017, 9:56 AM

Why is it needed to remove and re-add the notification instead of just calling update?

Yes of course, we should be calling dataChanged.

nicolasfella edited the summary of this revision. (Show Details)

emit dataChanged instead of readding the notification.

apol added a comment.Nov 1 2017, 11:35 PM

Otherwise looks good to me

plugins/notifications/notification.cpp
153

QStringLiteral

plugins/notifications/notificationsdbusinterface.cpp
148

Unrelated change, remove before committing.

153

Unrelated change, remove before committing.

plugins/notifications/notificationsdbusinterface.h
42

Unrelated change, remove before committing.

This revision was automatically updated to reflect the committed changes.