Re: Review Request 127398: Add unit tests for KNotification and fix a whole bunch of issues uncovered thanks to them

2016-03-30 Thread Martin Klapetek 

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

(Updated March 31, 2016, 2:50 a.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks.


Changes
---

Submitted with commit d4dd58759284ffe09420664e04a7a31e338e9ff1 by Martin 
Klapetek to branch master.


Repository: knotifications


Description
---

Adds basic set of unit tests including fake notifications server.

This helped uncover and fix these issues:

* Calling close() on KNotification that was not "sent" would not emit closed() 
and would not delete it
* Closing a notification can delete the KNotification object prematurely
* Invoking an action leads to unnecessary dbus roundtrip
* Invoking an action would fail to properly close and delete the KNotification 
object


Diffs
-

  CMakeLists.txt 51f6c22 
  autotests/CMakeLists.txt PRE-CREATION 
  autotests/fake_notifications_server.h PRE-CREATION 
  autotests/fake_notifications_server.cpp PRE-CREATION 
  autotests/knotification_test.cpp PRE-CREATION 
  autotests/knotifications5/qttest.notifyrc PRE-CREATION 
  autotests/qtest_dbus.h PRE-CREATION 
  src/knotification.cpp 17b0be2 
  src/knotificationmanager.cpp e80d48c 
  src/knotificationplugin.cpp acf964c 
  src/notifybypopup.cpp c0050b2 

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


Testing
---

Unit tests all pass.


Thanks,

Martin Klapetek

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127398: Add unit tests for KNotification and fix a whole bunch of issues uncovered thanks to them

2016-03-29 Thread Anthony Fieroni 

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



+1 for 5.21

- Anthony Fieroni


On Март 21, 2016, 11:59 след обяд, Martin Klapetek wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127398/
> ---
> 
> (Updated Март 21, 2016, 11:59 след обяд)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: knotifications
> 
> 
> Description
> ---
> 
> Adds basic set of unit tests including fake notifications server.
> 
> This helped uncover and fix these issues:
> 
> * Calling close() on KNotification that was not "sent" would not emit 
> closed() and would not delete it
> * Closing a notification can delete the KNotification object prematurely
> * Invoking an action leads to unnecessary dbus roundtrip
> * Invoking an action would fail to properly close and delete the 
> KNotification object
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt 51f6c22 
>   autotests/CMakeLists.txt PRE-CREATION 
>   autotests/fake_notifications_server.h PRE-CREATION 
>   autotests/fake_notifications_server.cpp PRE-CREATION 
>   autotests/knotification_test.cpp PRE-CREATION 
>   autotests/knotifications5/qttest.notifyrc PRE-CREATION 
>   autotests/qtest_dbus.h PRE-CREATION 
>   src/knotification.cpp 17b0be2 
>   src/knotificationmanager.cpp e80d48c 
>   src/knotificationplugin.cpp acf964c 
>   src/notifybypopup.cpp c0050b2 
> 
> Diff: https://git.reviewboard.kde.org/r/127398/diff/
> 
> 
> Testing
> ---
> 
> Unit tests all pass.
> 
> 
> Thanks,
> 
> Martin Klapetek
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127398: Add unit tests for KNotification and fix a whole bunch of issues uncovered thanks to them

2016-03-21 Thread Martin Klapetek 

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

(Updated March 21, 2016, 10:59 p.m.)


Review request for KDE Frameworks.


Changes
---

Start assigning the ids at 0 rather than 1 with -1 still being invalid.

There's now a special test for the ids but it's limited as there's currently
no way to test the id changes other than those two. This would require a special
signal being useful only for the test, which I'm not sure is good to add?


Repository: knotifications


Description
---

Adds basic set of unit tests including fake notifications server.

This helped uncover and fix these issues:

* Calling close() on KNotification that was not "sent" would not emit closed() 
and would not delete it
* Closing a notification can delete the KNotification object prematurely
* Invoking an action leads to unnecessary dbus roundtrip
* Invoking an action would fail to properly close and delete the KNotification 
object


Diffs (updated)
-

  CMakeLists.txt 51f6c22 
  autotests/CMakeLists.txt PRE-CREATION 
  autotests/fake_notifications_server.h PRE-CREATION 
  autotests/fake_notifications_server.cpp PRE-CREATION 
  autotests/knotification_test.cpp PRE-CREATION 
  autotests/knotifications5/qttest.notifyrc PRE-CREATION 
  autotests/qtest_dbus.h PRE-CREATION 
  src/knotification.cpp 17b0be2 
  src/knotificationmanager.cpp e80d48c 
  src/knotificationplugin.cpp acf964c 
  src/notifybypopup.cpp c0050b2 

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


Testing
---

Unit tests all pass.


Thanks,

Martin Klapetek

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127398: Add unit tests for KNotification and fix a whole bunch of issues uncovered thanks to them

2016-03-21 Thread Albert Astals Cid 


> On March 18, 2016, 5:20 a.m., Anthony Fieroni wrote:
> > src/knotification.cpp, line 63
> > 
> >
> > ref can't be UINT_MAX, but id can, no? Negative rage will brake all, 
> > it's this possible, maybe not. Counters it's not good idea to be signed.
> 
> Martin Klapetek wrote:
> Yes...if your application will emit more than 32767 notifications in its 
> lifetime. At which point the application is probalby broken.
> 
> Albert Astals Cid wrote:
> Is it? I don't use KTP, but assuming KTP sends Knotifications and 
> assuming you get like 1 chat notification per minute (not crazy imho) get say 
> around 500 notifications per day, so in 66 days you'd go over the limit. Sure 
> it's corner-case-y but i would not call it impossible nor the app being broken
> 
> Martin Klapetek wrote:
> This wouldn't actually break in the KTp case (in normal KNotification 
> usage, the id is not really needed), however this code (going back to 2009) 
> apparently chose real-life pragmatism over such corner cases.
> 
> To expand on that, the id uses values -1 and -2 to indicate "invalid" and 
> "about to be deleted" states. I can either introduce some bool/enum making 
> every "if (..)" twice as complicated, or start the counter from 2, reserving 
> 0 and 1. Tbh given this wasn't a problem for the past 7 years, I don't think 
> the code needs to be more complex making special assumptions for highly 
> unlikely corner cases.

ok, a bug to fix/discuss later in time :)


- Albert


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


On March 16, 2016, 8:23 p.m., Martin Klapetek wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127398/
> ---
> 
> (Updated March 16, 2016, 8:23 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: knotifications
> 
> 
> Description
> ---
> 
> Adds basic set of unit tests including fake notifications server.
> 
> This helped uncover and fix these issues:
> 
> * Calling close() on KNotification that was not "sent" would not emit 
> closed() and would not delete it
> * Closing a notification can delete the KNotification object prematurely
> * Invoking an action leads to unnecessary dbus roundtrip
> * Invoking an action would fail to properly close and delete the 
> KNotification object
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt 6d09051 
>   autotests/CMakeLists.txt PRE-CREATION 
>   autotests/fake_notifications_server.h PRE-CREATION 
>   autotests/fake_notifications_server.cpp PRE-CREATION 
>   autotests/knotification_test.cpp PRE-CREATION 
>   autotests/knotifications5/qttest.notifyrc PRE-CREATION 
>   autotests/qtest_dbus.h PRE-CREATION 
>   src/knotification.cpp 17b0be2 
>   src/knotificationmanager.cpp e80d48c 
>   src/knotificationplugin.cpp acf964c 
>   src/notifybypopup.cpp b9489c1 
> 
> Diff: https://git.reviewboard.kde.org/r/127398/diff/
> 
> 
> Testing
> ---
> 
> Unit tests all pass.
> 
> 
> Thanks,
> 
> Martin Klapetek
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127398: Add unit tests for KNotification and fix a whole bunch of issues uncovered thanks to them

2016-03-21 Thread Martin Klapetek 


> On March 18, 2016, 6:20 a.m., Anthony Fieroni wrote:
> > src/knotification.cpp, line 63
> > 
> >
> > ref can't be UINT_MAX, but id can, no? Negative rage will brake all, 
> > it's this possible, maybe not. Counters it's not good idea to be signed.
> 
> Martin Klapetek wrote:
> Yes...if your application will emit more than 32767 notifications in its 
> lifetime. At which point the application is probalby broken.
> 
> Albert Astals Cid wrote:
> Is it? I don't use KTP, but assuming KTP sends Knotifications and 
> assuming you get like 1 chat notification per minute (not crazy imho) get say 
> around 500 notifications per day, so in 66 days you'd go over the limit. Sure 
> it's corner-case-y but i would not call it impossible nor the app being broken

This wouldn't actually break in the KTp case (in normal KNotification usage, 
the id is not really needed), however this code (going back to 2009) apparently 
chose real-life pragmatism over such corner cases.

To expand on that, the id uses values -1 and -2 to indicate "invalid" and 
"about to be deleted" states. I can either introduce some bool/enum making 
every "if (..)" twice as complicated, or start the counter from 2, reserving 0 
and 1. Tbh given this wasn't a problem for the past 7 years, I don't think the 
code needs to be more complex making special assumptions for highly unlikely 
corner cases.


- Martin


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


On March 16, 2016, 9:23 p.m., Martin Klapetek wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127398/
> ---
> 
> (Updated March 16, 2016, 9:23 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: knotifications
> 
> 
> Description
> ---
> 
> Adds basic set of unit tests including fake notifications server.
> 
> This helped uncover and fix these issues:
> 
> * Calling close() on KNotification that was not "sent" would not emit 
> closed() and would not delete it
> * Closing a notification can delete the KNotification object prematurely
> * Invoking an action leads to unnecessary dbus roundtrip
> * Invoking an action would fail to properly close and delete the 
> KNotification object
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt 6d09051 
>   autotests/CMakeLists.txt PRE-CREATION 
>   autotests/fake_notifications_server.h PRE-CREATION 
>   autotests/fake_notifications_server.cpp PRE-CREATION 
>   autotests/knotification_test.cpp PRE-CREATION 
>   autotests/knotifications5/qttest.notifyrc PRE-CREATION 
>   autotests/qtest_dbus.h PRE-CREATION 
>   src/knotification.cpp 17b0be2 
>   src/knotificationmanager.cpp e80d48c 
>   src/knotificationplugin.cpp acf964c 
>   src/notifybypopup.cpp b9489c1 
> 
> Diff: https://git.reviewboard.kde.org/r/127398/diff/
> 
> 
> Testing
> ---
> 
> Unit tests all pass.
> 
> 
> Thanks,
> 
> Martin Klapetek
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127398: Add unit tests for KNotification and fix a whole bunch of issues uncovered thanks to them

2016-03-21 Thread Albert Astals Cid 


> On March 18, 2016, 5:20 a.m., Anthony Fieroni wrote:
> > src/knotification.cpp, line 63
> > 
> >
> > ref can't be UINT_MAX, but id can, no? Negative rage will brake all, 
> > it's this possible, maybe not. Counters it's not good idea to be signed.
> 
> Martin Klapetek wrote:
> Yes...if your application will emit more than 32767 notifications in its 
> lifetime. At which point the application is probalby broken.

Is it? I don't use KTP, but assuming KTP sends Knotifications and assuming you 
get like 1 chat notification per minute (not crazy imho) get say around 500 
notifications per day, so in 66 days you'd go over the limit. Sure it's 
corner-case-y but i would not call it impossible nor the app being broken


- Albert


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


On March 16, 2016, 8:23 p.m., Martin Klapetek wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127398/
> ---
> 
> (Updated March 16, 2016, 8:23 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: knotifications
> 
> 
> Description
> ---
> 
> Adds basic set of unit tests including fake notifications server.
> 
> This helped uncover and fix these issues:
> 
> * Calling close() on KNotification that was not "sent" would not emit 
> closed() and would not delete it
> * Closing a notification can delete the KNotification object prematurely
> * Invoking an action leads to unnecessary dbus roundtrip
> * Invoking an action would fail to properly close and delete the 
> KNotification object
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt 6d09051 
>   autotests/CMakeLists.txt PRE-CREATION 
>   autotests/fake_notifications_server.h PRE-CREATION 
>   autotests/fake_notifications_server.cpp PRE-CREATION 
>   autotests/knotification_test.cpp PRE-CREATION 
>   autotests/knotifications5/qttest.notifyrc PRE-CREATION 
>   autotests/qtest_dbus.h PRE-CREATION 
>   src/knotification.cpp 17b0be2 
>   src/knotificationmanager.cpp e80d48c 
>   src/knotificationplugin.cpp acf964c 
>   src/notifybypopup.cpp b9489c1 
> 
> Diff: https://git.reviewboard.kde.org/r/127398/diff/
> 
> 
> Testing
> ---
> 
> Unit tests all pass.
> 
> 
> Thanks,
> 
> Martin Klapetek
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127398: Add unit tests for KNotification and fix a whole bunch of issues uncovered thanks to them

2016-03-21 Thread Martin Klapetek 


> On March 18, 2016, 6:20 a.m., Anthony Fieroni wrote:
> > src/knotification.cpp, line 63
> > 
> >
> > ref can't be UINT_MAX, but id can, no? Negative rage will brake all, 
> > it's this possible, maybe not. Counters it's not good idea to be signed.

Yes...if your application will emit more than 32767 notifications in its 
lifetime. At which point the application is probalby broken.


> On March 18, 2016, 6:20 a.m., Anthony Fieroni wrote:
> > src/knotificationmanager.cpp, line 168
> > 
> >
> > force = true, id not present how can take value? force is compromised

The force is actually unused and should go at some later point.


- Martin


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


On March 16, 2016, 9:23 p.m., Martin Klapetek wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127398/
> ---
> 
> (Updated March 16, 2016, 9:23 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: knotifications
> 
> 
> Description
> ---
> 
> Adds basic set of unit tests including fake notifications server.
> 
> This helped uncover and fix these issues:
> 
> * Calling close() on KNotification that was not "sent" would not emit 
> closed() and would not delete it
> * Closing a notification can delete the KNotification object prematurely
> * Invoking an action leads to unnecessary dbus roundtrip
> * Invoking an action would fail to properly close and delete the 
> KNotification object
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt 6d09051 
>   autotests/CMakeLists.txt PRE-CREATION 
>   autotests/fake_notifications_server.h PRE-CREATION 
>   autotests/fake_notifications_server.cpp PRE-CREATION 
>   autotests/knotification_test.cpp PRE-CREATION 
>   autotests/knotifications5/qttest.notifyrc PRE-CREATION 
>   autotests/qtest_dbus.h PRE-CREATION 
>   src/knotification.cpp 17b0be2 
>   src/knotificationmanager.cpp e80d48c 
>   src/knotificationplugin.cpp acf964c 
>   src/notifybypopup.cpp b9489c1 
> 
> Diff: https://git.reviewboard.kde.org/r/127398/diff/
> 
> 
> Testing
> ---
> 
> Unit tests all pass.
> 
> 
> Thanks,
> 
> Martin Klapetek
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Review Request 127398: Add unit tests for KNotification and fix a whole bunch of issues uncovered thanks to them

2016-03-19 Thread Martin Klapetek 

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

Review request for KDE Frameworks.


Repository: knotifications


Description
---

Adds basic set of unit tests including fake notifications server.

This helped uncover and fix these issues:

* Calling close() on KNotification that was not "sent" would not emit closed() 
and would not delete it
* Closing a notification can delete the KNotification object prematurely
* Invoking an action leads to unnecessary dbus roundtrip
* Invoking an action would fail to properly close and delete the KNotification 
object


Diffs
-

  CMakeLists.txt 6d09051 
  autotests/CMakeLists.txt PRE-CREATION 
  autotests/fake_notifications_server.h PRE-CREATION 
  autotests/fake_notifications_server.cpp PRE-CREATION 
  autotests/knotification_test.cpp PRE-CREATION 
  autotests/knotifications5/qttest.notifyrc PRE-CREATION 
  autotests/qtest_dbus.h PRE-CREATION 
  src/knotification.cpp 17b0be2 
  src/knotificationmanager.cpp e80d48c 
  src/knotificationplugin.cpp acf964c 
  src/notifybypopup.cpp b9489c1 

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


Testing
---

Unit tests all pass.


Thanks,

Martin Klapetek

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127398: Add unit tests for KNotification and fix a whole bunch of issues uncovered thanks to them

2016-03-19 Thread David Edmundson 

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




src/knotification.cpp (line 396)


what if id == 0 ?


- David Edmundson


On March 16, 2016, 8:23 p.m., Martin Klapetek wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127398/
> ---
> 
> (Updated March 16, 2016, 8:23 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: knotifications
> 
> 
> Description
> ---
> 
> Adds basic set of unit tests including fake notifications server.
> 
> This helped uncover and fix these issues:
> 
> * Calling close() on KNotification that was not "sent" would not emit 
> closed() and would not delete it
> * Closing a notification can delete the KNotification object prematurely
> * Invoking an action leads to unnecessary dbus roundtrip
> * Invoking an action would fail to properly close and delete the 
> KNotification object
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt 6d09051 
>   autotests/CMakeLists.txt PRE-CREATION 
>   autotests/fake_notifications_server.h PRE-CREATION 
>   autotests/fake_notifications_server.cpp PRE-CREATION 
>   autotests/knotification_test.cpp PRE-CREATION 
>   autotests/knotifications5/qttest.notifyrc PRE-CREATION 
>   autotests/qtest_dbus.h PRE-CREATION 
>   src/knotification.cpp 17b0be2 
>   src/knotificationmanager.cpp e80d48c 
>   src/knotificationplugin.cpp acf964c 
>   src/notifybypopup.cpp b9489c1 
> 
> Diff: https://git.reviewboard.kde.org/r/127398/diff/
> 
> 
> Testing
> ---
> 
> Unit tests all pass.
> 
> 
> Thanks,
> 
> Martin Klapetek
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127398: Add unit tests for KNotification and fix a whole bunch of issues uncovered thanks to them

2016-03-19 Thread Anthony Fieroni 

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




src/knotification.cpp (line 63)


ref can't be UINT_MAX, but id can, no? Negative rage will brake all, it's 
this possible, maybe not. Counters it's not good idea to be signed.



src/knotificationmanager.cpp (line 168)


force = true, id not present how can take value? force is compromised


- Anthony Fieroni


On Март 16, 2016, 10:23 след обяд, Martin Klapetek wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127398/
> ---
> 
> (Updated Март 16, 2016, 10:23 след обяд)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: knotifications
> 
> 
> Description
> ---
> 
> Adds basic set of unit tests including fake notifications server.
> 
> This helped uncover and fix these issues:
> 
> * Calling close() on KNotification that was not "sent" would not emit 
> closed() and would not delete it
> * Closing a notification can delete the KNotification object prematurely
> * Invoking an action leads to unnecessary dbus roundtrip
> * Invoking an action would fail to properly close and delete the 
> KNotification object
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt 6d09051 
>   autotests/CMakeLists.txt PRE-CREATION 
>   autotests/fake_notifications_server.h PRE-CREATION 
>   autotests/fake_notifications_server.cpp PRE-CREATION 
>   autotests/knotification_test.cpp PRE-CREATION 
>   autotests/knotifications5/qttest.notifyrc PRE-CREATION 
>   autotests/qtest_dbus.h PRE-CREATION 
>   src/knotification.cpp 17b0be2 
>   src/knotificationmanager.cpp e80d48c 
>   src/knotificationplugin.cpp acf964c 
>   src/notifybypopup.cpp b9489c1 
> 
> Diff: https://git.reviewboard.kde.org/r/127398/diff/
> 
> 
> Testing
> ---
> 
> Unit tests all pass.
> 
> 
> Thanks,
> 
> Martin Klapetek
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127398: Add unit tests for KNotification and fix a whole bunch of issues uncovered thanks to them

2016-03-19 Thread Martin Klapetek 


> On March 18, 2016, 2:56 a.m., David Edmundson wrote:
> > src/knotification.cpp, line 399
> > 
> >
> > what if id == 0 ?

It actually never ever is. Which begs the question if the id should perhaps 
start at 0 rather than 1. Or maybe the invalid/default state should be 0 rather 
than -1.


- Martin


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


On March 16, 2016, 9:23 p.m., Martin Klapetek wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127398/
> ---
> 
> (Updated March 16, 2016, 9:23 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: knotifications
> 
> 
> Description
> ---
> 
> Adds basic set of unit tests including fake notifications server.
> 
> This helped uncover and fix these issues:
> 
> * Calling close() on KNotification that was not "sent" would not emit 
> closed() and would not delete it
> * Closing a notification can delete the KNotification object prematurely
> * Invoking an action leads to unnecessary dbus roundtrip
> * Invoking an action would fail to properly close and delete the 
> KNotification object
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt 6d09051 
>   autotests/CMakeLists.txt PRE-CREATION 
>   autotests/fake_notifications_server.h PRE-CREATION 
>   autotests/fake_notifications_server.cpp PRE-CREATION 
>   autotests/knotification_test.cpp PRE-CREATION 
>   autotests/knotifications5/qttest.notifyrc PRE-CREATION 
>   autotests/qtest_dbus.h PRE-CREATION 
>   src/knotification.cpp 17b0be2 
>   src/knotificationmanager.cpp e80d48c 
>   src/knotificationplugin.cpp acf964c 
>   src/notifybypopup.cpp b9489c1 
> 
> Diff: https://git.reviewboard.kde.org/r/127398/diff/
> 
> 
> Testing
> ---
> 
> Unit tests all pass.
> 
> 
> Thanks,
> 
> Martin Klapetek
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel