Review Request 121360: Rework Plasma's notification positioning code

[Martin Klapetek]

> On Jan. 3, 2015, 9:08 p.m., Kai Uwe Broulik wrote:
> > applets/notifications/plugin/notificationshelper.cpp, line 51
> > <https://git.reviewboard.kde.org/r/121360/diff/2/?file=337844#file337844line51>
> >
> >     Use QScopedPointer
> 
> Martin Klapetek wrote:
>     What's wrong with properly used delete?
> 
> David Edmundson wrote:
>     better yet, make the mutex on the stack.
>     
>     one less malloc == faster and less fragmented memory and you dont' have to worry about the memory at all.
> 
> Martin Klapetek wrote:
>     I had it originally that way, however if you want to make it recursive, you need to pass it to the constructor and the operator=() is private, so it cannot be assigned to stack variable. So I made it a pointer.
>     
>     But it's not like there's any need of management for it, it's simply created in constructor and deleted in the destructor, nothing else to worry about.
> 
> Martin Gräßlin wrote:
>     > I had it originally that way, however if you want to make it recursive, you need to pass it to the constructor and the operator=() is private, so it cannot be assigned to stack variable.
>     
>     That's not a problem, just initialize it in the initializer list of the ctor.
>     
>     Concerning the discussion on the management: I personally prefer QScopedPointer for cases like this as it's just something less to worry about and ideally you can get in a situation where one can use the default dtor which (at least I have read so) is more optimized than a written one.
> 
> Àlex Fiestas wrote:
>     This this case QScopedPointer is proof for future changes in the code, while delete it is easiert to screw it up (new return, move code around, etc).

Fair enough. But then again, there is no memory management involved anywhere except constructor and destructor (and shouldn't be and won't be), I really don't see the need for special pointers here


- Martin


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


On Jan. 6, 2015, 2:32 p.m., Martin Klapetek wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/121360/
> -----------------------------------------------------------
> 
> (Updated Jan. 6, 2015, 2:32 p.m.)
> 
> 
> Review request for Plasma and Kai Uwe Broulik.
> 
> 
> Bugs: 339732
>     https://bugs.kde.org/show_bug.cgi?id=339732
> 
> 
> Repository: plasma-workspace
> 
> 
> Description
> -------
> 
> There can easily be situations where the popups could overlap one another or result in strange animations. This patch rewrites the notifications so that all actions such as show/reposition/hide are handled from a one single place and every action is properly queued and protected around, which makes it more robust, more predictive and less chaotic. There's also a slight delay between every action so it's also visually much more cleaner and easier to see what's going on. 
> 
> 
> Diffs
> -----
> 
>   applets/notifications/plugin/notificationshelper.h af8f6fa 
>   applets/notifications/plugin/notificationshelper.cpp 425f0d6 
>   dataengines/notifications/notificationsengine.cpp d4b7f19 
>   applets/notifications/package/contents/ui/NotificationPopup.qml 8eb1912 
> 
> Diff: https://git.reviewboard.kde.org/r/121360/diff/
> 
> 
> Testing
> -------
> 
> Tested whole day plus stress-tested with something like for i in {1..200}; do notify-send "$i - $RANDOM" "$RANDOM sdf sdf sdfwefhsdjfnskdfbkwefnos igodsfgn sodifgj asodfgnsdlfgdf g"; done executed from 4 terminals at once, all works fine and as expected.
> 
> 
> Thanks,
> 
> Martin Klapetek
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20150106/803e3688/attachment.html>