----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124281/#review82179 -----------------------------------------------------------
Looks good to me otherwise src/knotificationmanager.cpp (line 87) <https://git.reviewboard.kde.org/r/124281/#comment56570> Use `KPluginLoader::findPlugins` or `KPluginLoader::instantiatePlugins` and then do the qobject_cast to avoid that duplicate code src/notifybypopup.cpp (line 274) <https://git.reviewboard.kde.org/r/124281/#comment56571> nullptr? or Q_NULLPTR if that's not allowed. Would make it explicit that it's a pointer. - Alex Richardson On July 7, 2015, 3:42 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, 3:42 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/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
