Re: Review Request 127326: [KNotification] KNofication remove leak and missing emit closed
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127326/ --- (Updated Март 16, 2016, 11:33 след обяд) Status -- This change has been discarded. Review request for KDE Frameworks, Kai Uwe Broulik, David Edmundson, and Martin Klapetek. Repository: knotifications Description --- If KNotification is not notified, he don't ref() notification, if call close() on it cause a leak, emu closed() is not triggered. https://git.reviewboard.kde.org/r/127318/ Diffs - src/knotification.cpp 17b0be2 Diff: https://git.reviewboard.kde.org/r/127326/diff/ Testing --- No real test, just blame me if i'm not right. Thanks, Anthony Fieroni ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 127326: [KNotification] KNofication remove leak and missing emit closed
> On Март 10, 2016, 3:56 след обяд, Martin Klapetek wrote:
> > Can you please explain clearly the problem this should solve and explain
> > more clearly how this solves that problem?
> >
> > Also I see with your every review "I have not tested this", please do test
> > your patches before posting them.
>
> Anthony Fieroni wrote:
> Looks like my English is really bad :)
> auto n = new KNotification;
> n->close(); <--- ref = 0, closed() is not emitted,
> deleteLater() is not called => leak
>
> Martin Klapetek wrote:
> This is a wrong fix for it then. Proper fix would be to init d->id to -1
> rather than 0, which I have now done, but I'll keep testing it for a bit.
Also notice
100 if (d->id > 0) {
101 KNotificationManager::self()->close(d->id);
102 }
273 if (d->id >= 0) {
274 KNotificationManager::self()->close(d->id);
275 }
Looks errorish.
- Anthony
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127326/#review93387
---
On Март 10, 2016, 11:28 преди обяд, Anthony Fieroni wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127326/
> ---
>
> (Updated Март 10, 2016, 11:28 преди обяд)
>
>
> Review request for KDE Frameworks, Kai Uwe Broulik, David Edmundson, and
> Martin Klapetek.
>
>
> Repository: knotifications
>
>
> Description
> ---
>
> If KNotification is not notified, he don't ref() notification, if call
> close() on it cause a leak, emu closed() is not triggered.
> https://git.reviewboard.kde.org/r/127318/
>
>
> Diffs
> -
>
> src/knotification.cpp 17b0be2
>
> Diff: https://git.reviewboard.kde.org/r/127326/diff/
>
>
> Testing
> ---
>
> No real test, just blame me if i'm not right.
>
>
> Thanks,
>
> Anthony Fieroni
>
>
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 127326: [KNotification] KNofication remove leak and missing emit closed
> On March 10, 2016, 2:56 p.m., Martin Klapetek wrote: > > Can you please explain clearly the problem this should solve and explain > > more clearly how this solves that problem? > > > > Also I see with your every review "I have not tested this", please do test > > your patches before posting them. > > Anthony Fieroni wrote: > Looks like my English is really bad :) > auto n = new KNotification; > n->close(); <--- ref = 0, closed() is not emitted, > deleteLater() is not called => leak This is a wrong fix for it then. Proper fix would be to init d->id to -1 rather than 0, which I have now done, but I'll keep testing it for a bit. - Martin --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127326/#review93387 --- On March 10, 2016, 10:28 a.m., Anthony Fieroni wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/127326/ > --- > > (Updated March 10, 2016, 10:28 a.m.) > > > Review request for KDE Frameworks, Kai Uwe Broulik, David Edmundson, and > Martin Klapetek. > > > Repository: knotifications > > > Description > --- > > If KNotification is not notified, he don't ref() notification, if call > close() on it cause a leak, emu closed() is not triggered. > https://git.reviewboard.kde.org/r/127318/ > > > Diffs > - > > src/knotification.cpp 17b0be2 > > Diff: https://git.reviewboard.kde.org/r/127326/diff/ > > > Testing > --- > > No real test, just blame me if i'm not right. > > > Thanks, > > Anthony Fieroni > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 127326: [KNotification] KNofication remove leak and missing emit closed
> On Март 10, 2016, 3:56 след обяд, Martin Klapetek wrote: > > Can you please explain clearly the problem this should solve and explain > > more clearly how this solves that problem? > > > > Also I see with your every review "I have not tested this", please do test > > your patches before posting them. Looks like my English is really bad :) auto n = new KNotification; n->close(); <--- ref = 0, closed() is not emitted, deleteLater() is not called => leak - Anthony --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127326/#review93387 --- On Март 10, 2016, 11:28 преди обяд, Anthony Fieroni wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/127326/ > --- > > (Updated Март 10, 2016, 11:28 преди обяд) > > > Review request for KDE Frameworks, Kai Uwe Broulik, David Edmundson, and > Martin Klapetek. > > > Repository: knotifications > > > Description > --- > > If KNotification is not notified, he don't ref() notification, if call > close() on it cause a leak, emu closed() is not triggered. > https://git.reviewboard.kde.org/r/127318/ > > > Diffs > - > > src/knotification.cpp 17b0be2 > > Diff: https://git.reviewboard.kde.org/r/127326/diff/ > > > Testing > --- > > No real test, just blame me if i'm not right. > > > Thanks, > > Anthony Fieroni > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 127326: [KNotification] KNofication remove leak and missing emit closed
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127326/#review93387 --- Can you please explain clearly the problem this should solve and explain more clearly how this solves that problem? Also I see with your every review "I have not tested this", please do test your patches before posting them. - Martin Klapetek On March 10, 2016, 10:28 a.m., Anthony Fieroni wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/127326/ > --- > > (Updated March 10, 2016, 10:28 a.m.) > > > Review request for KDE Frameworks, Kai Uwe Broulik, David Edmundson, and > Martin Klapetek. > > > Repository: knotifications > > > Description > --- > > If KNotification is not notified, he don't ref() notification, if call > close() on it cause a leak, emu closed() is not triggered. > https://git.reviewboard.kde.org/r/127318/ > > > Diffs > - > > src/knotification.cpp 17b0be2 > > Diff: https://git.reviewboard.kde.org/r/127326/diff/ > > > Testing > --- > > No real test, just blame me if i'm not right. > > > Thanks, > > Anthony Fieroni > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Review Request 127326: [KNotification] KNofication remove leak and missing emit closed
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127326/ --- Review request for KDE Frameworks, Kai Uwe Broulik, David Edmundson, and Martin Klapetek. Repository: knotifications Description --- If KNotification is not notified, he don't ref() notification, if call close() on it cause a leak, emu closed() is not triggered. https://git.reviewboard.kde.org/r/127318/ Diffs - src/knotification.cpp 17b0be2 Diff: https://git.reviewboard.kde.org/r/127326/diff/ Testing --- No real test, just blame me if i'm not right. Thanks, Anthony Fieroni ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
