> On July 7, 2015, 9:38 p.m., Mark Gaiser wrote: > > src/knotificationmanager.cpp, line 89 > > <https://git.reviewboard.kde.org/r/124281/diff/3/?file=383605#file383605line89> > > > > 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.
> Don't you have a memory leak here? No; the plugins wanted are deleted when KNotificationManager is deleted, the plugins not wanted are deleted on line 101. The QObject *s are owned by KNotificationManager, that's what the last arg to KPluginLoader::instantiatePlugins does. On July 7, 2015, 9:38 p.m., Martin Klapetek wrote: > > 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 No; KWS is used much more than that, do a grep in the repo. Basically it's used for window activation/raising and related stuff, not _only_ because of KCrash. In fact, KCrash is only a dependency of KService and not used at all in KNotifications, KService is gone but KWindowSystem is still very much needed. - Martin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124281/#review82195 ----------------------------------------------------------- On July 7, 2015, 9 p.m., Martin Klapetek wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/124281/ > ----------------------------------------------------------- > > (Updated July 7, 2015, 9 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
