Re: Review Request 124281: Remove KService and KIconThemes usage from KNotifications
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124281/ --- (Updated July 21, 2015, 6:39 p.m.) Status -- This change has been marked as submitted. Review request for KDE Frameworks and Martin Gräßlin. Changes --- Submitted with commit ba267253dbaeac32bb033e2f519d10651075c766 by Martin Klapetek to branch master. 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 7b600a4 metainfo.yaml 7fc15f7 src/CMakeLists.txt 1cebece src/knotificationmanager.cpp 8d4f953 src/knotificationplugin.cpp 7315c17 src/notifybypopup.cpp bb96465 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
Re: Review Request 124281: Remove KService and KIconThemes usage from KNotifications
On July 13, 2015, 8:44 p.m., Sune Vuorela wrote: src/notifybypopup.cpp, line 565 https://git.reviewboard.kde.org/r/124281/diff/4/?file=383900#file383900line565 Isn,t QFont() the same as QApplication::font(), and then the #include QApplication seems unused. It is the same but Q(Gui)Application is still used. I'll change to QGuiApplication though. - Martin --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124281/#review82467 --- On July 9, 2015, 3: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, 3: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 ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 124281: Remove KService and KIconThemes usage from KNotifications
--- 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 ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 124281: Remove KService and KIconThemes usage from KNotifications
On July 9, 2015, 4 p.m., Kai Uwe Broulik wrote: src/knotificationmanager.cpp, lines 28-29 https://git.reviewboard.kde.org/r/124281/diff/4/?file=383898#file383898line28 Unused Fixed locally, thanks. - Martin --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124281/#review82269 --- On July 9, 2015, 3: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, 3: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 ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 124281: Remove KService and KIconThemes usage from KNotifications
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124281/#review82269 --- src/knotificationmanager.cpp (lines 28 - 29) https://git.reviewboard.kde.org/r/124281/#comment56648 Unused - Kai Uwe Broulik On Juli 9, 2015, 1:10 nachm., Martin Klapetek wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124281/ --- (Updated Juli 9, 2015, 1:10 nachm.) 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
Re: Review Request 124281: Remove KService and KIconThemes usage from KNotifications
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124281/ --- (Updated July 9, 2015, 3:10 p.m.) Review request for KDE Frameworks and Martin Gräßlin. Changes --- Pass relative path to KPluginLoader 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 (updated) - 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
Re: Review Request 124281: Remove KService and KIconThemes usage from KNotifications
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124281/#review82234 --- src/knotificationmanager.cpp (line 92) https://git.reviewboard.kde.org/r/124281/#comment56610 There is no need to iterate over the library paths: If a relative path is given for directory, all entries of QCoreApplication::libraryPaths() will be checked with directory appended as a subdirectory. If an absolute path is given only that directory will be searched. `QListQObject* plugins = KPluginLoader::instantiatePlugins(knotification/notifyplugins, nullptr, this);` - Alex Richardson On July 7, 2015, 8 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, 8 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
Re: Review Request 124281: Remove KService and KIconThemes usage from KNotifications
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
Re: Review Request 124281: Remove KService and KIconThemes usage from KNotifications
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124281/#review82195 --- src/knotificationmanager.cpp (line 87) https://git.reviewboard.kde.org/r/124281/#comment56593 Don't you have a memory leak here? You have a list of pointers which you own here, but QList doesn't know that. When that list goes out of scope it doesn't delete the pointers as far as i know. The simplest way i can think of is to make plugins a class member and call qDeleteAll(m_plugins) in the class destructor which then deletes all objects. Or you could go for smart pointers which you store in the QList, that would also clean it up nicely when going out of scope. 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 - Mark Gaiser 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
Re: Review Request 124281: Remove KService and KIconThemes usage from KNotifications
On July 7, 2015, 5:10 p.m., Alex Richardson wrote: src/knotificationmanager.cpp, line 89 https://git.reviewboard.kde.org/r/124281/diff/2/?file=383580#file383580line89 Use `KPluginLoader::findPlugins` or `KPluginLoader::instantiatePlugins` and then do the qobject_cast to avoid that duplicate code Fwiw, the KPluginLoader docu says this: Therefore, if you do not need the plugin version feature, you can (and should) just use QPluginLoader instead. Looks like it could be updated. On July 7, 2015, 5:10 p.m., Alex Richardson wrote: src/notifybypopup.cpp, line 278 https://git.reviewboard.kde.org/r/124281/diff/2/?file=383581#file383581line278 nullptr? or Q_NULLPTR if that's not allowed. Would make it explicit that it's a pointer. Yeah, good point. - Martin --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124281/#review82179 --- On July 7, 2015, 4: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, 4: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
Re: Review Request 124281: Remove KService and KIconThemes usage from KNotifications
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124281/ --- (Updated July 7, 2015, 9 p.m.) Review request for KDE Frameworks and Martin Gräßlin. Changes --- KPluginLoader + Q_NULLPTR 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 (updated) - 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
Re: Review Request 124281: Remove KService and KIconThemes usage from KNotifications
On July 7, 2015, 9:38 p.m., Mark Gaiser wrote: src/knotificationmanager.cpp, line 89 https://git.reviewboard.kde.org/r/124281/diff/3/?file=383605#file383605line89 Don't you have a memory leak here? You have a list of pointers which you own here, but QList doesn't know that. When that list goes out of scope it doesn't delete the pointers as far as i know. The simplest way i can think of is to make plugins a class member and call qDeleteAll(m_plugins) in the class destructor which then deletes all objects. Or you could go for smart pointers which you store in the QList, that would also clean it up nicely when going out of scope. Don't you have a memory leak here? No; the plugins wanted are deleted when KNotificationManager is deleted, the plugins not wanted are deleted on line 101. The QObject *s are owned by KNotificationManager, that's what the last arg to KPluginLoader::instantiatePlugins does. On July 7, 2015, 9: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 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. - Martin --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124281/#review82195 --- On July 7, 2015, 9 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, 9 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
Re: Review Request 124281: Remove KService and KIconThemes usage from KNotifications
--- 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
Review Request 124281: Remove KService and KIconThemes usage from KNotifications
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124281/ --- 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
Re: Review Request 124281: Remove KService and KIconThemes usage from KNotifications
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124281/#review82175 --- I like it! - Martin Gräßlin 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 ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 124281: Remove KService and KIconThemes usage from KNotifications
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124281/#review82172 --- src/knotificationmanager.cpp (line 87) https://git.reviewboard.kde.org/r/124281/#comment56560 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? src/notifybypopup.cpp (line 561) https://git.reviewboard.kde.org/r/124281/#comment56561 Will that pass the DPI test? Have you tested with a different QT_DEVICE_PIXEL_RATIO? - Aleix Pol Gonzalez 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 ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 124281: Remove KService and KIconThemes usage from KNotifications
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 ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 124281: Remove KService and KIconThemes usage from KNotifications
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124281/ --- (Updated July 7, 2015, 4:42 p.m.) Review request for KDE Frameworks and Martin Gräßlin. Changes --- Set Qt::AA_UseHighDpiPixmaps attribute on the kpassivepopup test Apps are meant to set it and so should the tests 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 (updated) - 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
