Review Request 124281: Remove KService and KIconThemes usage from KNotifications
Sune Vuorela
kde at pusling.com
Mon Jul 13 18:44:45 UTC 2015
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124281/#review82467
-----------------------------------------------------------
In general looks good.
src/knotificationmanager.cpp (line 29)
<https://git.reviewboard.kde.org/r/124281/#comment56872>
Unused ?
src/notifybypopup.cpp (line 561)
<https://git.reviewboard.kde.org/r/124281/#comment56873>
Isn,t QFont() the same as QApplication::font(), and then the #include <QApplication> seems unused.
- Sune Vuorela
On July 9, 2015, 1:10 p.m., Martin Klapetek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124281/
> -----------------------------------------------------------
>
> (Updated July 9, 2015, 1:10 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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20150713/4266b04c/attachment.html>
More information about the Kde-frameworks-devel
mailing list