Review Request 124281: Remove KService and KIconThemes usage from KNotifications

[Martin Klapetek]

> On July 7, 2015, 3:56 p.m., Aleix Pol Gonzalez wrote:
> > src/knotificationmanager.cpp, line 89
> > <https://git.reviewboard.kde.org/r/124281/diff/1/?file=383552#file383552line89>
> >
> >     Would it make any sense to remove this altogether?
> >     
> >     If nobody is using it, it sounds useless.
> >     Was it used for those Ubuntu-specific notifications, maybe?

No, there were requests for custom plugins, but nobody was using it because the plugin class was not public until very recently. So yeah, this is wanted feature and I'd like to keep it in.


> On July 7, 2015, 3:56 p.m., Aleix Pol Gonzalez wrote:
> > src/notifybypopup.cpp, line 565
> > <https://git.reviewboard.kde.org/r/124281/diff/1/?file=383553#file383553line565>
> >
> >     Will that pass the DPI test? Have you tested with  a different QT_DEVICE_PIXEL_RATIO?

Yeah it works fine if the app has the Qt::AA_UseHighDpiPixmaps attribute set (which they are supposed to be setting for hidpi support), then it scales correctly with any QT_DEVICE_PIXEL_RATIO.


- Martin


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124281/#review82172
-----------------------------------------------------------


On July 7, 2015, 2:52 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, 2:52 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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20150707/4f6fe6ea/attachment.html>