Review Request 124281: Remove KService and KIconThemes usage from KNotifications

[Mark Gaiser]

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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20150708/925f1206/attachment.html>