> On May 2, 2014, 2:12 p.m., David Edmundson wrote:
> > applets/notifications/package/contents/ui/Notifications.qml, line 41
> > <https://git.reviewboard.kde.org/r/117903/diff/1/?file=270229#file270229line41>
> >
> >     personally I would have parsed the QQmlComponent object to cpp then 
> > done the create using http://qt-project.org/doc/qt-5/qqmlcomponent.html
> >     It makes them easier to track

I tried loading it in cpp (using KDeclarative and Plasma::Package), the problem 
was that it didn't have access to the rest of the plasmoid and vice-versa -> 
the ultimate result was that the plasmoid was unable to communicate with the 
dataengine which meant no actions or unable to close the notification from the 
dataengine (like if you switch to a chat window and the notification should 
just close). So I went for this way, it's the easiest and works just fine.


> On May 2, 2014, 2:12 p.m., David Edmundson wrote:
> > applets/notifications/plugin/notificationshelper.h, line 45
> > <https://git.reviewboard.kde.org/r/117903/diff/1/?file=270230#file270230line45>
> >
> >     slot and invokable?
> >     
> >     doubly invokable!
> >     
> >     also can you document the QVariantMap a bit to say what the keys are 
> > (or that it's the data from the dataengine or something..)

eheh, yeah, this was just moved from above the slots; will fix


> On May 2, 2014, 2:12 p.m., David Edmundson wrote:
> > applets/notifications/plugin/notificationshelper.cpp, line 142
> > <https://git.reviewboard.kde.org/r/117903/diff/1/?file=270231#file270231line142>
> >
> >     is there a nice constant in plasma for the 300?
> >     
> >     There's a shortDuration and longDuration exposed to plasma from 
> > somewhere.

That's in units, which have no equiv in cpp (except reading the config file 
directly). I guess I could just pass it over from QML if really needed


- Martin


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


On April 30, 2014, 4:27 p.m., Martin Klapetek wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/117903/
> -----------------------------------------------------------
> 
> (Updated April 30, 2014, 4:27 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Repository: plasma-workspace
> 
> 
> Description
> -------
> 
> Before, the notifications would create new Dialog for each incoming 
> notification. Now it reuses only 3 Dialogs to save resources.
> 
> The notification popup is a QML component; I decided to handle all this in 
> the cpp plugin as JS Array is quite crap compared to QList and I had quite 
> some crashes in QV4 on slightly heavier Array processing. But as the QML 
> component needs to talk to the dataengine (like when you close the 
> notification or exectue an action) and uses some methods from the rest of the 
> plasmoid, I can't just instantiate it from the cpp side. So I create the 
> component in QML, pass it to CPP, set the ownership to CPP so it's not 
> deleted ever and then handle it all there.
> 
> 
> Diffs
> -----
> 
>   applets/notifications/package/contents/ui/NotificationPopup.qml 9265352 
>   applets/notifications/package/contents/ui/Notifications.qml 64d80a7 
>   applets/notifications/plugin/notificationshelper.h 1e1f6c2 
>   applets/notifications/plugin/notificationshelper.cpp 1edfbad 
> 
> Diff: https://git.reviewboard.kde.org/r/117903/diff/
> 
> 
> Testing
> -------
> 
> Plasma no more leaks memory with every notification (the root memleak, coming 
> from Dialog, is quite possibly still present, but the notifications don't 
> expose it anymore). Notifications work as before, even better.
> 
> For testing purposes, you can try running 
> 
> for i in {1..10}; do notify-send asasd$i -i kde; done
> 
> 
> Thanks,
> 
> Martin Klapetek
> 
>

_______________________________________________
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Reply via email to