Re: Review Request 129146: Fix emitting close when an event has no actions

[David Edmundson]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129146/
---

(Updated Oct. 15, 2016, midnight)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks.


Changes
---

Submitted with commit 07a6d3e2e51ee461ad37bb42edad3518f3b8a3e9 by David 
Edmundson to branch master.


Repository: knotifications


Description
---

KNotificatitionManager::self() has an early check if a notification has
no actions. If it has no actions it tries to close the notification
early. However it's dereferencing an object that it has not referenced,
sending the ref count to -1, a corrupt state that is not handled
gracefully in KNotification, and we don't ever close the notification.

By referencing and dereferencing we're still calling the cleanup in
KNotification (if applicable) but without sending the ref count negative.

This fixes ksmserver waiting for the logout sound to play; which
currently never emits close, as the default setup has no logout sound.


Diffs
-

  autotests/knotification_test.cpp 5d063f1a19d7f5e4d8c77d7e6301e81223401b08 
  src/knotification.cpp 352cf496faef7a120c3d5c805cf6ffb324292a0c 
  src/knotificationmanager.cpp c315db9a3f17771d4095cfdc7982475949213a1c 

Diff: https://git.reviewboard.kde.org/r/129146/diff/


Testing
---

Autotest that used to fail included


Thanks,

David Edmundson

Re: Review Request 129146: Fix emitting close when an event has no actions

[David Edmundson]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129146/
---

(Updated Oct. 13, 2016, 2:30 p.m.)


Review request for KDE Frameworks.


Repository: knotifications


Description
---

KNotificatitionManager::self() has an early check if a notification has
no actions. If it has no actions it tries to close the notification
early. However it's dereferencing an object that it has not referenced,
sending the ref count to -1, a corrupt state that is not handled
gracefully in KNotification, and we don't ever close the notification.

By referencing and dereferencing we're still calling the cleanup in
KNotification (if applicable) but without sending the ref count negative.

This fixes ksmserver waiting for the logout sound to play; which
currently never emits close, as the default setup has no logout sound.


Diffs (updated)
-

  autotests/knotification_test.cpp 5d063f1a19d7f5e4d8c77d7e6301e81223401b08 
  src/knotification.cpp 352cf496faef7a120c3d5c805cf6ffb324292a0c 
  src/knotificationmanager.cpp c315db9a3f17771d4095cfdc7982475949213a1c 

Diff: https://git.reviewboard.kde.org/r/129146/diff/


Testing
---

Autotest that used to fail included


Thanks,

David Edmundson

Re: Review Request 129146: Fix emitting close when an event has no actions

[Aleix Pol Gonzalez]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129146/#review99980
---


Ship it!




Makes sense to me, I'd add an assert as well, to make sure we're using the API 
properly.

- Aleix Pol Gonzalez


On Oct. 11, 2016, 1:19 p.m., David Edmundson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129146/
> ---
> 
> (Updated Oct. 11, 2016, 1:19 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: knotifications
> 
> 
> Description
> ---
> 
> KNotificatitionManager::self() has an early check if a notification has
> no actions. If it has no actions it tries to close the notification
> early. However it's dereferencing an object that it has not referenced,
> sending the ref count to -1, a corrupt state that is not handled
> gracefully in KNotification, and we don't ever close the notification.
> 
> By referencing and dereferencing we're still calling the cleanup in
> KNotification (if applicable) but without sending the ref count negative.
> 
> This fixes ksmserver waiting for the logout sound to play; which
> currently never emits close, as the default setup has no logout sound.
> 
> 
> Diffs
> -
> 
>   autotests/knotification_test.cpp 5d063f1a19d7f5e4d8c77d7e6301e81223401b08 
>   src/knotificationmanager.cpp c315db9a3f17771d4095cfdc7982475949213a1c 
> 
> Diff: https://git.reviewboard.kde.org/r/129146/diff/
> 
> 
> Testing
> ---
> 
> Autotest that used to fail included
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

Re: Review Request 129146: Fix emitting close when an event has no actions

[David Edmundson]

> On Oct. 11, 2016, 12:16 p.m., Anthony Fieroni wrote:
> > src/knotificationmanager.cpp, line 203
> > 
> >
> > n->close()
> 
> David Edmundson wrote:
> I did think about that, but what if the userspace code is:
> 
> KNotification *n = new KNoti..
> n->ref();
> n->sendEvent();
> 
> we wouldn't want to close immediately.
> 
> Anthony Fieroni wrote:
> User must *never* do that, ref/deref interface is only for manager.

It's documented as being available for user use:

https://api.kde.org/frameworks-api/frameworks-apidocs/frameworks/knotifications/html/classKNotification.html#ac37dbd2cccd26af667bea20a7df831c3

>void KNotification::ref(   )   

>The notification will automatically be closed if all presentations are 
>finished.

>if you want to show your own presentation in your application, you should use 
>this function, so it will not be automatically closed when there is nothing to 
>show.

>Don't forgot to deref, or the notification may be never closed if there is no 
>timeout.


- David


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129146/#review99930
---


On Oct. 11, 2016, 11:19 a.m., David Edmundson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129146/
> ---
> 
> (Updated Oct. 11, 2016, 11:19 a.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: knotifications
> 
> 
> Description
> ---
> 
> KNotificatitionManager::self() has an early check if a notification has
> no actions. If it has no actions it tries to close the notification
> early. However it's dereferencing an object that it has not referenced,
> sending the ref count to -1, a corrupt state that is not handled
> gracefully in KNotification, and we don't ever close the notification.
> 
> By referencing and dereferencing we're still calling the cleanup in
> KNotification (if applicable) but without sending the ref count negative.
> 
> This fixes ksmserver waiting for the logout sound to play; which
> currently never emits close, as the default setup has no logout sound.
> 
> 
> Diffs
> -
> 
>   autotests/knotification_test.cpp 5d063f1a19d7f5e4d8c77d7e6301e81223401b08 
>   src/knotificationmanager.cpp c315db9a3f17771d4095cfdc7982475949213a1c 
> 
> Diff: https://git.reviewboard.kde.org/r/129146/diff/
> 
> 
> Testing
> ---
> 
> Autotest that used to fail included
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

Re: Review Request 129146: Fix emitting close when an event has no actions

[Anthony Fieroni]

> On Oct. 11, 2016, 3:16 p.m., Anthony Fieroni wrote:
> > src/knotificationmanager.cpp, line 203
> > 
> >
> > n->close()
> 
> David Edmundson wrote:
> I did think about that, but what if the userspace code is:
> 
> KNotification *n = new KNoti..
> n->ref();
> n->sendEvent();
> 
> we wouldn't want to close immediately.

User must *never* do that, ref/deref interface is only for manager.


- Anthony


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129146/#review99930
---


On Oct. 11, 2016, 2:19 p.m., David Edmundson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129146/
> ---
> 
> (Updated Oct. 11, 2016, 2:19 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: knotifications
> 
> 
> Description
> ---
> 
> KNotificatitionManager::self() has an early check if a notification has
> no actions. If it has no actions it tries to close the notification
> early. However it's dereferencing an object that it has not referenced,
> sending the ref count to -1, a corrupt state that is not handled
> gracefully in KNotification, and we don't ever close the notification.
> 
> By referencing and dereferencing we're still calling the cleanup in
> KNotification (if applicable) but without sending the ref count negative.
> 
> This fixes ksmserver waiting for the logout sound to play; which
> currently never emits close, as the default setup has no logout sound.
> 
> 
> Diffs
> -
> 
>   autotests/knotification_test.cpp 5d063f1a19d7f5e4d8c77d7e6301e81223401b08 
>   src/knotificationmanager.cpp c315db9a3f17771d4095cfdc7982475949213a1c 
> 
> Diff: https://git.reviewboard.kde.org/r/129146/diff/
> 
> 
> Testing
> ---
> 
> Autotest that used to fail included
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

Re: Review Request 129146: Fix emitting close when an event has no actions

[David Edmundson]

> On Oct. 11, 2016, 12:16 p.m., Anthony Fieroni wrote:
> > src/knotificationmanager.cpp, line 203
> > 
> >
> > n->close()

I did think about that, but what if the userspace code is:

KNotification *n = new KNoti..
n->ref();
n->sendEvent();

we wouldn't want to close immediately.


- David


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129146/#review99930
---


On Oct. 11, 2016, 11:19 a.m., David Edmundson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129146/
> ---
> 
> (Updated Oct. 11, 2016, 11:19 a.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: knotifications
> 
> 
> Description
> ---
> 
> KNotificatitionManager::self() has an early check if a notification has
> no actions. If it has no actions it tries to close the notification
> early. However it's dereferencing an object that it has not referenced,
> sending the ref count to -1, a corrupt state that is not handled
> gracefully in KNotification, and we don't ever close the notification.
> 
> By referencing and dereferencing we're still calling the cleanup in
> KNotification (if applicable) but without sending the ref count negative.
> 
> This fixes ksmserver waiting for the logout sound to play; which
> currently never emits close, as the default setup has no logout sound.
> 
> 
> Diffs
> -
> 
>   autotests/knotification_test.cpp 5d063f1a19d7f5e4d8c77d7e6301e81223401b08 
>   src/knotificationmanager.cpp c315db9a3f17771d4095cfdc7982475949213a1c 
> 
> Diff: https://git.reviewboard.kde.org/r/129146/diff/
> 
> 
> Testing
> ---
> 
> Autotest that used to fail included
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

Re: Review Request 129146: Fix emitting close when an event has no actions

[Anthony Fieroni]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129146/#review99930
---




src/knotificationmanager.cpp (line 203)


n->close()


- Anthony Fieroni


On Oct. 11, 2016, 2:19 p.m., David Edmundson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129146/
> ---
> 
> (Updated Oct. 11, 2016, 2:19 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: knotifications
> 
> 
> Description
> ---
> 
> KNotificatitionManager::self() has an early check if a notification has
> no actions. If it has no actions it tries to close the notification
> early. However it's dereferencing an object that it has not referenced,
> sending the ref count to -1, a corrupt state that is not handled
> gracefully in KNotification, and we don't ever close the notification.
> 
> By referencing and dereferencing we're still calling the cleanup in
> KNotification (if applicable) but without sending the ref count negative.
> 
> This fixes ksmserver waiting for the logout sound to play; which
> currently never emits close, as the default setup has no logout sound.
> 
> 
> Diffs
> -
> 
>   autotests/knotification_test.cpp 5d063f1a19d7f5e4d8c77d7e6301e81223401b08 
>   src/knotificationmanager.cpp c315db9a3f17771d4095cfdc7982475949213a1c 
> 
> Diff: https://git.reviewboard.kde.org/r/129146/diff/
> 
> 
> Testing
> ---
> 
> Autotest that used to fail included
> 
> 
> Thanks,
> 
> David Edmundson
> 
>