On jul 7, 2015, 7: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 > > Martin Klapetek wrote: > 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.
Thank you for correcting me on both comments :) - Mark ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124281/#review82195 ----------------------------------------------------------- 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
