Differential D6089

Show no notification icon if there is no
ClosedPublic
Actions

Authored by nicolasfella on Jun 4 2017, 3:48 PM.

Details

Reviewers
albertvaka
Group Reviewers
KDE Connect
Commits
R224:732462135f56: Show no notification icon if there is no
Summary

I think it looks nicer than the kind of angry default icon and is the expected behaviour

Diff Detail

Repository
R224 KDE Connect
Lint
Lint Skipped
Unit
Unit Tests Skipped
nicolasfella created this revision.Jun 4 2017, 3:48 PM
apol added a subscriber: apol.Jun 5 2017, 9:28 AM

Also note there's 2 unrelated white-space changes.

plugins/notifications/notification.cpp
105 ↗(On Diff #15134)

QString()?

nicolasfella updated this revision to Diff 15214.Jun 6 2017, 12:15 PM

Fixed whitespace

nicolasfella added inline comments.Jun 8 2017, 11:34 AM
plugins/notifications/notification.cpp
105 ↗(On Diff #15134)

setIconName("none") is different to setIconName("").
Also the use of QStringLiteral is right here https://woboq.com/blog/qstringliteral.html

albertvaka added a subscriber: albertvaka.Jun 22 2017, 9:06 PM
albertvaka added inline comments.
plugins/notifications/notification.cpp
105 ↗(On Diff #15134)

setIconName("none") will try to find an icon named "none". Will display nothing because there is no icon with that name. The proper way of not displaying an icon is to set it to empty string, that is QString().

nicolasfella added inline comments.Jul 10 2017, 4:51 PM
plugins/notifications/notification.cpp
105 ↗(On Diff #15134)

setIconName(QString()) gives me some default icon, but setIconName("foobar") gives me my desired behaviour (no icon at all)

apol added inline comments.Jul 10 2017, 10:09 PM
plugins/notifications/notification.cpp
105 ↗(On Diff #15134)

Meh. Not your fault then.

Can you at least add a comment saying so?

albertvaka accepted this revision.Jul 10 2017, 10:24 PM

Maybe just change "none" to something like "nonexistent_icon_name_so_we_display_nothing", so it's more obvious this is not a hidden feature of KNotification or something...

plugins/notifications/notification.cpp
105 ↗(On Diff #15134)

https://media.giphy.com/media/a9xhxAxaqOfQs/giphy.gif

https://media.giphy.com/media/BEob5qwFkSJ7G/giphy.gif

This revision is now accepted and ready to land.Jul 10 2017, 10:24 PM
Closed by commit R224:732462135f56: Show no notification icon if there is no (authored by nicolasfella). · Explain WhyJul 11 2017, 9:27 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 15214

View Options

plugins/notifications/notification.cpp

Loading...