Review Request 121360: Rework Plasma's notification positioning code

[Martin Klapetek]

> On Jan. 5, 2015, 4:19 p.m., David Edmundson wrote:
> > applets/notifications/plugin/notificationshelper.cpp, line 111
> > <https://git.reviewboard.kde.org/r/121360/diff/2/?file=337844#file337844line111>
> >
> >     you're liable to break here.
> >     
> >     sometimes you call this directly not via the Qt meta system. (i.e processHide)
> >     
> >     what calls processHide? could be the user pressing close, could be a timeout. If so the sender() will be that timer. It's the sender of the last signal we're processing, not the last method call made.
> >     
> >     I suggest maybe 
> >     
> >         connect(m_dispatchTimer, &QTimer::timeout, this, [this]{m_busy=false ; processQueues()}
> >     
> >     instead
> 
> Martin Klapetek wrote:
>     processHide is called just and only from processQueues. Basically processQueues works as sort of dispatcher and processHide just returns the control back to it, it will then simply process next item in the queue. It's perfectly safe as processQueues is ever only called either manually (sender() == 0) or by the timer (sender() == QTimer). The lambda slot is maybe nicer, I just thought that it makes the code a bit harder to read...but I don't care that much.
> 
> David Edmundson wrote:
>     calling it manually does not set the sender to 0.

Yes it does.


- Martin


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


On Jan. 5, 2015, 2:33 p.m., Martin Klapetek wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/121360/
> -----------------------------------------------------------
> 
> (Updated Jan. 5, 2015, 2:33 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/package/contents/ui/NotificationPopup.qml 4491230 
>   applets/notifications/plugin/notificationshelper.h af8f6fa 
>   applets/notifications/plugin/notificationshelper.cpp 425f0d6 
>   dataengines/notifications/notificationsengine.cpp d4b7f19 
> 
> 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/20150105/742f2a7d/attachment.html>