Differential D4545

Centralize interaction with notifications
ClosedPublic
Actions

Authored by apol on Feb 10 2017, 12:26 PM.

Details

Reviewers
davidedmundson
mart
albertvaka
Group Reviewers
Plasma
Commits
R120:eb0991bc8387: Centralize interaction with notifications
Summary

Make sure clicking on them will work the same on the popup and the delegate,
there were some issues on one or the other, now all the code is centralized
in NotificationItem.

Test Plan

Been testing locally, repeatedly

Diff Detail

Repository
R120 Plasma Workspace
Branch
arcpatch-D4545
Lint
No Linters Available
Unit
No Unit Test Coverage
apol updated this revision to Diff 11166.Feb 10 2017, 12:26 PM
apol retitled this revision from to Centralize interaction with notifications.
apol updated this object.
apol edited the test plan for this revision. (Show Details)
apol added reviewers: Plasma, mart, davidedmundson.
Restricted Application added a project: Plasma. · View Herald TranscriptFeb 10 2017, 12:26 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
albertvaka added inline comments.Feb 10 2017, 1:44 PM
applets/notifications/package/contents/ui/NotificationDelegate.qml
36–37

This was there so they highlight on hover. Is it not needed anymore?

applets/notifications/package/contents/ui/NotificationItem.qml
70 ↗(On Diff #11166)

This is not the behaviour that was agreed here [1], we shouldn't close the notification if there is no action to be performed.

[1] https://phabricator.kde.org/D4215

mck182 added a subscriber: mck182.Feb 13 2017, 4:57 PM

Can we please fix and ship this quickly? https://phabricator.kde.org/D4142 shouldn't really have been shipped in that state.

apol marked an inline comment as done.Feb 13 2017, 5:19 PM
apol added inline comments.
applets/notifications/package/contents/ui/NotificationDelegate.qml
36–37

Earlier Plasma decided this isn't how we display mouse hover.
https://phabricator.kde.org/D4214

apol updated this revision to Diff 11302.Feb 13 2017, 5:20 PM

Address comment from Albert

Adopt behavior discussed in D4215

mart accepted this revision.Feb 13 2017, 8:47 PM
mart edited edge metadata.
This revision is now accepted and ready to land.Feb 13 2017, 8:47 PM
albertvaka requested changes to this revision.Feb 13 2017, 10:36 PM
albertvaka edited edge metadata.
albertvaka added inline comments.
applets/notifications/package/contents/ui/NotificationDelegate.qml
32 ↗(On Diff #11302)

toolIconSize property is used here: plasma-workspace/applets/notifications/package/contents/ui/Notifications.qml

This revision now requires changes to proceed.Feb 13 2017, 10:36 PM
apol updated this revision to Diff 11310.Feb 13 2017, 11:11 PM
apol edited edge metadata.

Stop setting unused property

apol marked an inline comment as done.Feb 13 2017, 11:11 PM
apol updated this revision to Diff 11311.Feb 13 2017, 11:21 PM
apol edited edge metadata.

Address Albert's comments

apol updated this revision to Diff 11312.Feb 13 2017, 11:26 PM

Add hover feedback

albertvaka accepted this revision.Feb 13 2017, 11:30 PM
albertvaka edited edge metadata.
This revision is now accepted and ready to land.Feb 13 2017, 11:30 PM
Closed by commit R120:eb0991bc8387: Centralize interaction with notifications (authored by apol). · Explain WhyFeb 13 2017, 11:38 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 11302

View Options

applets/notifications/package/contents/ui/NotificationDelegate.qml

Loading...
View Options

applets/notifications/package/contents/ui/NotificationItem.qml

Loading...
View Options

applets/notifications/package/contents/ui/NotificationPopup.qml

Loading...