----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117903/#review57120 -----------------------------------------------------------
Ship it! Looks good, minor comments which you can chose to ignore. applets/notifications/package/contents/ui/Notifications.qml <https://git.reviewboard.kde.org/r/117903/#comment39806> 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 applets/notifications/plugin/notificationshelper.h <https://git.reviewboard.kde.org/r/117903/#comment39807> 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..) applets/notifications/plugin/notificationshelper.cpp <https://git.reviewboard.kde.org/r/117903/#comment39808> is there a nice constant in plasma for the 300? There's a shortDuration and longDuration exposed to plasma from somewhere. - David Edmundson On April 30, 2014, 2: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, 2: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
