DevHeads.net

Review Request 115695: Rework KNotification to work without KNotify daemon

Review request for kde-workspace, KDE Frameworks, Plasma, and Sune Vuorela.

Repository: knotifications

Description
This patch merges KNotify daemon into KNotificationManager to have less daemons running and less dbus traffic. The patch is not yet finished (and for now it's full of QDebugs, that will all be removed and FIXMEs to indicate what needs doing), but as the Alpha2 is quite soon, I'd like to start the general review asap so some more changes can be done if needed.

Now it's KNotificationManager that handles the KNotifyPlugin-s and hands them the notification directly. KNotifyConfig was repurposed a bit, now it serves mostly just as the .notifyrc wrapper, all the rest is reused from the KNotification object. There are some changes in the KNotification API - id() and appName() are now exposed to public and "slotReceivedId(int)" is now also public so that KNotificationManager can directly give it an id. I'd like to rename this and make it a non-slot. It's not the DBus/Galago server ID anymore, that is handled in NotifyByPopup which is responsible for communicating with the galago server (all the methods there were renamed to actually have *galago* in the name so it's clear), therefore the mapping of DBus/Galago Server ids is managed only there as it is actually only needed here. KNotitification::id() is assigned by the KNotificationManager and it's a simple increasing counter.

The UI/Plasmoid changes will come next - basically the plan is to put only the "Persistent" notifications in the notifications history.

Diffs
src/notifybypopupgrowl.cpp PRE-CREATION
src/notifybypopupgrowl.h PRE-CREATION
src/notifybypopup.cpp PRE-CREATION
src/notifybypopup.h PRE-CREATION
src/knotifyplugin.h PRE-CREATION
src/knotifyplugin.cpp PRE-CREATION
src/knotifyconfig.cpp PRE-CREATION
src/knotifyconfig.h PRE-CREATION
src/knotificationmanager.cpp a4d0dfa
src/knotificationmanager_p.h 81d962d
src/knotification.cpp 5d7405b
src/knotification.h 00554ba
CMakeLists.txt 63ebf71
src/CMakeLists.txt a81b913

Diff: <a href="https://git.reviewboard.kde.org/r/115695/diff/" title="https://git.reviewboard.kde.org/r/115695/diff/">https://git.reviewboard.kde.org/r/115695/diff/</a>

Testing
Works perfectly with both plasma notifications and kpassivepopup.

Thanks,

Martin Klapetek

Comments

Re: Review Request 115695: Rework KNotification to work without

By Martin Klapetek at 02/12/2014 - 11:54

(Updated Feb. 12, 2014, 4:54 p.m.)

Review request for kde-workspace, KDE Frameworks, Plasma, and Sune Vuorela.

Changes
Couple updates
* slots in NotifyByPopup renamed all to on*
* reenabled resolveUndeclaredEntity --> dep on KCodecs

Repository: knotifications

Description
This patch merges KNotify daemon into KNotificationManager to have less daemons running and less dbus traffic. The patch is not yet finished (and for now it's full of QDebugs, that will all be removed and FIXMEs to indicate what needs doing), but as the Alpha2 is quite soon, I'd like to start the general review asap so some more changes can be done if needed.

Now it's KNotificationManager that handles the KNotifyPlugin-s and hands them the notification directly. KNotifyConfig was repurposed a bit, now it serves mostly just as the .notifyrc wrapper, all the rest is reused from the KNotification object. There are some changes in the KNotification API - id() and appName() are now exposed to public and "slotReceivedId(int)" is now also public so that KNotificationManager can directly give it an id. I'd like to rename this and make it a non-slot. It's not the DBus/Galago server ID anymore, that is handled in NotifyByPopup which is responsible for communicating with the galago server (all the methods there were renamed to actually have *galago* in the name so it's clear), therefore the mapping of DBus/Galago Server ids is managed only there as it is actually only needed here. KNotitification::id() is assigned by the KNotificationManager and it's a simple increasing counter.

The UI/Plasmoid changes will come next - basically the plan is to put only the "Persistent" notifications in the notifications history.

Diffs (updated)
src/knotification.h 00554ba
CMakeLists.txt 63ebf71
src/CMakeLists.txt a81b913
src/knotification.cpp 5d7405b
src/knotificationmanager.cpp a4d0dfa
src/knotificationmanager_p.h 81d962d
src/knotifyconfig.h PRE-CREATION
src/knotifyconfig.cpp PRE-CREATION
src/knotifyplugin.h PRE-CREATION
src/knotifyplugin.cpp PRE-CREATION
src/notifybypopup.h PRE-CREATION
src/notifybypopup.cpp PRE-CREATION
src/notifybypopupgrowl.h PRE-CREATION
src/notifybypopupgrowl.cpp PRE-CREATION

Diff: <a href="https://git.reviewboard.kde.org/r/115695/diff/" title="https://git.reviewboard.kde.org/r/115695/diff/">https://git.reviewboard.kde.org/r/115695/diff/</a>

Testing
Works perfectly with both plasma notifications and kpassivepopup.

Thanks,

Martin Klapetek

Re: Review Request 115695: Rework KNotification to work without

By Martin Klapetek at 02/13/2014 - 06:14

(Updated Feb. 13, 2014, 11:14 a.m.)

Review request for kde-workspace, KDE Frameworks, Plasma, and Sune Vuorela.

Changes
Another bunch of updates
* dpointer for NotfiyByPopup
* simplified KNotificationManager::notify()'s arguments - passing just the KNotification object is enough
* no need to generate KNotify interface files

Repository: knotifications

Description
This patch merges KNotify daemon into KNotificationManager to have less daemons running and less dbus traffic. The patch is not yet finished (and for now it's full of QDebugs, that will all be removed and FIXMEs to indicate what needs doing), but as the Alpha2 is quite soon, I'd like to start the general review asap so some more changes can be done if needed.

Now it's KNotificationManager that handles the KNotifyPlugin-s and hands them the notification directly. KNotifyConfig was repurposed a bit, now it serves mostly just as the .notifyrc wrapper, all the rest is reused from the KNotification object. There are some changes in the KNotification API - id() and appName() are now exposed to public and "slotReceivedId(int)" is now also public so that KNotificationManager can directly give it an id. I'd like to rename this and make it a non-slot. It's not the DBus/Galago server ID anymore, that is handled in NotifyByPopup which is responsible for communicating with the galago server (all the methods there were renamed to actually have *galago* in the name so it's clear), therefore the mapping of DBus/Galago Server ids is managed only there as it is actually only needed here. KNotitification::id() is assigned by the KNotificationManager and it's a simple increasing counter.

The UI/Plasmoid changes will come next - basically the plan is to put only the "Persistent" notifications in the notifications history.

Diffs (updated)
src/knotifyconfig.h PRE-CREATION
src/knotifyconfig.cpp PRE-CREATION
src/knotifyplugin.h PRE-CREATION
src/knotifyplugin.cpp PRE-CREATION
src/notifybypopup.h PRE-CREATION
src/notifybypopup.cpp PRE-CREATION
src/notifybypopupgrowl.h PRE-CREATION
src/notifybypopupgrowl.cpp PRE-CREATION
CMakeLists.txt 63ebf71
src/CMakeLists.txt a81b913
src/knotification.h 00554ba
src/knotification.cpp 5d7405b
src/knotificationmanager.cpp a4d0dfa
src/knotificationmanager_p.h 81d962d

Diff: <a href="https://git.reviewboard.kde.org/r/115695/diff/" title="https://git.reviewboard.kde.org/r/115695/diff/">https://git.reviewboard.kde.org/r/115695/diff/</a>

Testing
Works perfectly with both plasma notifications and kpassivepopup.

Thanks,

Martin Klapetek

Re: Review Request 115695: Rework KNotification to work without

By Martin Klapetek at 02/26/2014 - 11:53

(Updated Feb. 26, 2014, 4:53 p.m.)

Status
This change has been discarded.

Review request for kde-workspace, KDE Frameworks, Plasma, and Sune Vuorela.

Repository: knotifications

Description
This patch merges KNotify daemon into KNotificationManager to have less daemons running and less dbus traffic. The patch is not yet finished (and for now it's full of QDebugs, that will all be removed and FIXMEs to indicate what needs doing), but as the Alpha2 is quite soon, I'd like to start the general review asap so some more changes can be done if needed.

Now it's KNotificationManager that handles the KNotifyPlugin-s and hands them the notification directly. KNotifyConfig was repurposed a bit, now it serves mostly just as the .notifyrc wrapper, all the rest is reused from the KNotification object. There are some changes in the KNotification API - id() and appName() are now exposed to public and "slotReceivedId(int)" is now also public so that KNotificationManager can directly give it an id. I'd like to rename this and make it a non-slot. It's not the DBus/Galago server ID anymore, that is handled in NotifyByPopup which is responsible for communicating with the galago server (all the methods there were renamed to actually have *galago* in the name so it's clear), therefore the mapping of DBus/Galago Server ids is managed only there as it is actually only needed here. KNotitification::id() is assigned by the KNotificationManager and it's a simple increasing counter.

The UI/Plasmoid changes will come next - basically the plan is to put only the "Persistent" notifications in the notifications history.

Diffs
src/knotifyconfig.h PRE-CREATION
src/knotifyconfig.cpp PRE-CREATION
src/knotifyplugin.h PRE-CREATION
src/knotifyplugin.cpp PRE-CREATION
src/notifybypopup.h PRE-CREATION
src/notifybypopup.cpp PRE-CREATION
src/notifybypopupgrowl.h PRE-CREATION
src/notifybypopupgrowl.cpp PRE-CREATION
CMakeLists.txt 63ebf71
src/CMakeLists.txt a81b913
src/knotification.h 00554ba
src/knotification.cpp 5d7405b
src/knotificationmanager.cpp a4d0dfa
src/knotificationmanager_p.h 81d962d

Diff: <a href="https://git.reviewboard.kde.org/r/115695/diff/" title="https://git.reviewboard.kde.org/r/115695/diff/">https://git.reviewboard.kde.org/r/115695/diff/</a>

Testing
Works perfectly with both plasma notifications and kpassivepopup.

Thanks,

Martin Klapetek

Re: Review Request 115695: Rework KNotification to work without

By Kevin Ottens at 02/21/2014 - 03:06

Way to big to properly review to be honest. That said, I agree with Aleix I'd like it to be closer to the target with less regressions before letting it in. You still have a few days. ;-)

Also don't forget to update the yaml and the list of frameworks in the wiki, it's moving in tier 3 territory AFAICT.

- Kevin Ottens

On Feb. 13, 2014, 10:14 a.m., Martin Klapetek wrote:

Re: Review Request 115695: Rework KNotification to work without

By Aleix Pol at 02/19/2014 - 06:05

src/knotification.cpp
<https://git.reviewboard.kde.org/r/115695/#comment35297>

remove qDebugs?

src/knotification.cpp
<https://git.reviewboard.kde.org/r/115695/#comment35298>

Improve message and make qWarning?

src/knotification.cpp
<https://git.reviewboard.kde.org/r/115695/#comment35299>

Needed?

src/knotificationmanager.cpp
<https://git.reviewboard.kde.org/r/115695/#comment35300>

This will be done in the future? Maybe it would be better to commit when there are no regressions?

src/knotificationmanager.cpp
<https://git.reviewboard.kde.org/r/115695/#comment35301>

qWarning?

src/knotificationmanager.cpp
<https://git.reviewboard.kde.org/r/115695/#comment35302>

?

src/notifybypopupgrowl.h
<https://git.reviewboard.kde.org/r/115695/#comment35303>

Is growl still the thing for MacOS X?

I'm sorry I cannot give better insight about the moving the daemon here. It makes sense to me, but I have no idea why it used to be this way.

- Aleix Pol Gonzalez

On Feb. 13, 2014, 10:14 a.m., Martin Klapetek wrote:

Re: Review Request 115695: Rework KNotification to work without

By =?UTF-8?Q?Nicol... at 02/19/2014 - 17:56

OS X 10.8 added a built-in notification system.

- Nicolás

On Feb. 13, 2014, 7:14 a.m., Martin Klapetek wrote:

Re: Review Request 115695: Rework KNotification to work without

By Martin Klapetek at 02/19/2014 - 08:52

I'm not expecting the ship is as much just yet, but rather comments on how the new architecture without knotify looks & works. I put it up early on as Kevin wanted to have this in Alpha2, which is in 10 days from now. So I wanted to get comments on the general approach asap so I could still manage to handle any wanted changes in the arch before Alpha2.

- Martin

On Feb. 13, 2014, 11:14 a.m., Martin Klapetek wrote:

Re: Review Request 115695: Rework KNotification to work without

By Aleix Pol at 02/19/2014 - 08:44

Still you shouldn't ship such comments, so it's probably why nobody checked the "ship it" thing.

- Aleix

On Feb. 13, 2014, 10:14 a.m., Martin Klapetek wrote:

Re: Review Request 115695: Rework KNotification to work without

By Martin Klapetek at 02/19/2014 - 06:17

Yes it will..again, as said in the description - "but as the Alpha2 is quite soon, I'd like to start the general review asap so some more changes can be done if needed."

...so this is more about the general change rather than the final code ;)

Yet once again the description xD - "it's full of ... FIXMEs to indicate what needs doing"

I'm not sure to be honest...I've heard they reworked it and I've no idea if they still support Growl...also Growl used to be on Windows afair, but have no idea about it there either...I'd personally be in favor of dropping this as I have no way to test

- Martin

On Feb. 13, 2014, 11:14 a.m., Martin Klapetek wrote:

Re: Review Request 115695: Rework KNotification to work without

By Martin Klapetek at 02/19/2014 - 06:12

As the description says - "and for now it's full of QDebugs, that will all be removed" - so I'll drop all the qdebug issues

- Martin

On Feb. 13, 2014, 11:14 a.m., Martin Klapetek wrote:

Re: Review Request 115695: Rework KNotification to work without

By Martin Klapetek at 02/18/2014 - 06:48

Ping, any comments?

- Martin Klapetek

On Feb. 13, 2014, 11:14 a.m., Martin Klapetek wrote:

Re: Review Request 115695: Rework KNotification to work without

By Martin Klapetek at 02/12/2014 - 08:57

src/notifybypopup.cpp
<https://git.reviewboard.kde.org/r/115695/#comment34956>

This is obviously reversed to test KPassivePopup

- Martin Klapetek

On Feb. 12, 2014, 1:56 p.m., Martin Klapetek wrote: