----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124281/#review82195 -----------------------------------------------------------
src/knotificationmanager.cpp (line 87) <https://git.reviewboard.kde.org/r/124281/#comment56593> Don't you have a memory leak here? You have a list of pointers which you own here, but QList doesn't know that. When that list goes out of scope it doesn't delete the pointers as far as i know. The simplest way i can think of is to make plugins a class member and call qDeleteAll(m_plugins) in the class destructor which then deletes all objects. Or you could go for smart pointers which you store in the QList, that would also clean it up nicely when going out of scope. I'm looking at the KNotifications dependency graph here [1] and see that KWindowSystem is only required for KCrash. So err, can't that one go as well since you removed KService which required KCrash which then required KWindowSystem? I could be very wrong if KNotifications is using KWindowSystem somewhere, obviously. But the graph doesn't give me that impression. [1] http://api.kde.org/frameworks-api/frameworks5-apidocs/knotifications/html/knotifications-dependencies.html - Mark Gaiser On jul 7, 2015, 7 p.m., Martin Klapetek wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/124281/ > ----------------------------------------------------------- > > (Updated jul 7, 2015, 7 p.m.) > > > Review request for KDE Frameworks and Martin Gräßlin. > > > Repository: knotifications > > > Description > ------- > > This patch reduces the dependencies of KNotifications framework and > effectively makes it a tier 2 framework. > > KService is used only for locating additional notification plugins and to my > knowledge, > there are none such plugins currently existing, at least not in all around > KDE plus > the class for the plugins wasn't exported until about two months ago, so this > should > be safe without a legacy support. > > KIconThemes is used only to get "KIconLoader::Small" icon pixmap for > notifications > using KPassivePopup. After some playing around it turns out that it puts the > icon into > the KPassivePopup title and makes it as big as the text. So I've made the > icon size to > be the same as the text height. So this keeps things visually the same + > still DPI aware, > though I believe the KPassivePopup should receive a complete visual overhaul > anyway. > > Additionally, KCodecs dependency has again one single usage for decoding html > entities > to QChars within QXmlStreamReader parser, so eventually could also be > removed/replaced > with QTextDocument::toPlainText() which seems to do the same job as > QXmlStreamReader+KCodecs. > > > Diffs > ----- > > CMakeLists.txt 2d5437b > metainfo.yaml 7fc15f7 > src/CMakeLists.txt 1cebece > src/knotificationmanager.cpp 8d4f953 > src/knotificationplugin.cpp 7315c17 > src/notifybypopup.cpp e377051 > tests/kpassivepopuptest.h bc0dedc > tests/kpassivepopuptest.cpp 2486fd8 > > Diff: https://git.reviewboard.kde.org/r/124281/diff/ > > > Testing > ------- > > Everything still compiles + I added a test for KPassivePopup with an icon. > > > Thanks, > > Martin Klapetek > >
_______________________________________________ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
