> On March 18, 2016, 5:20 a.m., Anthony Fieroni wrote: > > src/knotification.cpp, line 63 > > <https://git.reviewboard.kde.org/r/127398/diff/1/?file=453383#file453383line63> > > > > 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