Re: Review Request 124306: Don't choke on empty QIconItem
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124306/#review82278 --- Ship it! it would be more performant to delete the node if it's empty. though this is still fine. - David Edmundson On July 9, 2015, 2:07 p.m., Aleix Pol Gonzalez wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124306/ --- (Updated July 9, 2015, 2:07 p.m.) Review request for KDE Frameworks. Repository: kdeclarative Description --- Sometimes QML components have 0 width and height and that's perfectly fine. If we try to paint it, we get warnings such as: `QPainter::begin: Paint device returned engine == 0, type: 2` Diffs - src/qmlcontrols/kquickcontrolsaddons/qiconitem.cpp 3a9dd17 Diff: https://git.reviewboard.kde.org/r/124306/diff/ Testing --- Fixes the issue in muon-discover, doesn't break tests. Thanks, Aleix Pol Gonzalez ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 124328: Man pages for kapidox
On July 12, 2015, 4:45 p.m., Kevin Krammer wrote: I thought our man pages are written in docbook and then translated by the usual docbook workflow? Albert Astals Cid wrote: Correct. Luigi Toscano wrote: Yes, because they can be translated. But that means a dependency on kdoctools. On the other side, kapidox is a tier1 (I would say tier0) project, it's python, it's not really Qt based. I don't have a proper solution for this problem right now. Scott Kitterman wrote: It's really different than the other frameworks. It's not a framework as much as a tool (Python application) for developing the frameworks. I don't think it can/should be done exactly the same as everything else. Albert Astals Cid wrote: I'm not happy for not keeping our requeriment that our user visible stuff is translatable. Scott Kitterman wrote: The --help output (which is the only documentation that's currently provided) is already not translatable, so this is no different. This is not user visible. It's not even visible to application developers, IIUC. It's an internal tool, much like kde-dev-scripts (in fact this stuff could have been put in kde-dev-scripts...), where none of the scripts have translatable help. - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124328/#review82397 --- On July 12, 2015, 4 p.m., Scott Kitterman wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124328/ --- (Updated July 12, 2015, 4 p.m.) Review request for KDE Frameworks, Alex Merry, Aurélien Gâteau, and Kevin Krammer. Repository: kapidox Description --- Add man pages for kapidox scripts Diffs - docs/depdiagram-generate-all.1 PRE-CREATION docs/depdiagram-generate.1 PRE-CREATION docs/depdiagram-prepare.1 PRE-CREATION docs/kgenapidox.1 PRE-CREATION docs/kgenframeworksapidox.1 PRE-CREATION setup.py 083543f Diff: https://git.reviewboard.kde.org/r/124328/diff/ Testing --- Built and installed updated version and verified man pages were correctly installed and readable in the KDE man page viewer. Thanks, Scott Kitterman ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 124105: Adhere a bit better to the spec
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124105/#review82468 --- Ship it! Ship It! - Aleix Pol Gonzalez On July 4, 2015, 9:18 p.m., Sune Vuorela wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124105/ --- (Updated July 4, 2015, 9:18 p.m.) Review request for KDE Frameworks, Aleix Pol Gonzalez, Christoph Feck, David Faure, Eike Hein, and Martin Klapetek. Repository: kiconthemes Description --- According to table 2 in the icon theme spec, the Context per directory key is optional, and the Type key defaults to Threshold if not specified. Let's make the code do the same. Diffs - autotests/breeze.theme 8aef88d autotests/kiconloader_unittest.cpp d1a52fc src/kicontheme.cpp 4f0e522 Diff: https://git.reviewboard.kde.org/r/124105/diff/ Testing --- Thanks, Sune Vuorela ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 123030: Let KHtml be useable w/o searching for private deps
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123030/ --- (Updated July 13, 2015, 6:54 p.m.) Status -- This change has been marked as submitted. Review request for KDE Frameworks and Martin Tobias Holmedahl Sandsmark. Changes --- Submitted with commit 093c96c87b3267ed16dce6faae98df139e1c1407 by Hrvoje Senjan to branch master. Repository: khtml Description --- Only search for public deps in cmake config Diffs - KF5KHtmlConfig.cmake.in 74e822c src/CMakeLists.txt c6f5fab Diff: https://git.reviewboard.kde.org/r/123030/diff/ Testing --- Thanks, Hrvoje Senjan ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 124331: New proxy: KExtraColumnsProxyModel, allows to add columns to an existing model.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124331/#review82472 --- I like the concept, and have a whole bunch of nitpickery and a couple of more important bits. src/kextracolumnsproxymodel.h (line 92) https://git.reviewboard.kde.org/r/124331/#comment56881 where is the implementation of this? (and the unit tests ? :) src/kextracolumnsproxymodel.cpp (line 43) https://git.reviewboard.kde.org/r/124331/#comment56877 Are this one of the cases where mmutz and mwolff won't hunt you down for using QList ? src/kextracolumnsproxymodel.cpp (line 77) https://git.reviewboard.kde.org/r/124331/#comment56875 Is this assert actually needed? isn't it kind of valid to pass in a nullptr ? src/kextracolumnsproxymodel.cpp (line 78) https://git.reviewboard.kde.org/r/124331/#comment56876 shouldn't the old source model's about to be changed signals be disconnected here, or are QAPM taking care of that ? src/kextracolumnsproxymodel.cpp (line 99) https://git.reviewboard.kde.org/r/124331/#comment56880 isn't this kind of a normal state. is noisy output warranted here? src/kextracolumnsproxymodel.cpp (line 115) https://git.reviewboard.kde.org/r/124331/#comment56878 I know it is widely used outside Qt but .. Q_D is actually not public Qt api. Similar for Q_Q. src/kextracolumnsproxymodel.cpp (line 170) https://git.reviewboard.kde.org/r/124331/#comment56879 this is surprising me a bit. I'd expect that I by default would have emitted dataChanged inside my implementation of setExtraColumnData, especially if setting a piece of data influences multiple roles. src/kextracolumnsproxymodel.cpp (line 194) https://git.reviewboard.kde.org/r/124331/#comment56874 Wouldn't it be better to redelegate this to a set of separate functions, just like the actual data so that other roles are actually supported? (In my similar, but not as thorough implementations of this, I've often needed other roles like color and/or decoration) - Sune Vuorela On July 13, 2015, 8:31 a.m., David Faure wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124331/ --- (Updated July 13, 2015, 8:31 a.m.) Review request for KDE Frameworks, Jesper Pedersen and Stephen Kelly. Repository: kitemmodels Description --- REVIEW: 124331 Diffs - autotests/CMakeLists.txt f98e87d49b965998f31c5ede1fefb03b159a150f autotests/kextracolumnsproxymodeltest.cpp PRE-CREATION autotests/test_model_helpers.h a44db8a9cb985f69b52be96f0ffe389ebd903d6f src/CMakeLists.txt 82d776f1fcc2f7b15e74f0ed47702ed570ce9892 src/kextracolumnsproxymodel.h PRE-CREATION src/kextracolumnsproxymodel.cpp PRE-CREATION Diff: https://git.reviewboard.kde.org/r/124331/diff/ Testing --- Wrote autotests (as extensive as possible); used this in one real project, AFAIK Jesper (blackie) did as well. Thanks, David Faure ___ 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 124328: Man pages for kapidox
On July 12, 2015, 4:45 p.m., Kevin Krammer wrote: I thought our man pages are written in docbook and then translated by the usual docbook workflow? Albert Astals Cid wrote: Correct. Luigi Toscano wrote: Yes, because they can be translated. But that means a dependency on kdoctools. On the other side, kapidox is a tier1 (I would say tier0) project, it's python, it's not really Qt based. I don't have a proper solution for this problem right now. Scott Kitterman wrote: It's really different than the other frameworks. It's not a framework as much as a tool (Python application) for developing the frameworks. I don't think it can/should be done exactly the same as everything else. Albert Astals Cid wrote: I'm not happy for not keeping our requeriment that our user visible stuff is translatable. Scott Kitterman wrote: The --help output (which is the only documentation that's currently provided) is already not translatable, so this is no different. David Faure wrote: This is not user visible. It's not even visible to application developers, IIUC. It's an internal tool, much like kde-dev-scripts (in fact this stuff could have been put in kde-dev-scripts...), where none of the scripts have translatable help. then I drop my objection. I can read troff well enough :) - Allen --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124328/#review82397 --- On July 12, 2015, 4 p.m., Scott Kitterman wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124328/ --- (Updated July 12, 2015, 4 p.m.) Review request for KDE Frameworks, Alex Merry, Aurélien Gâteau, and Kevin Krammer. Repository: kapidox Description --- Add man pages for kapidox scripts Diffs - docs/depdiagram-generate-all.1 PRE-CREATION docs/depdiagram-generate.1 PRE-CREATION docs/depdiagram-prepare.1 PRE-CREATION docs/kgenapidox.1 PRE-CREATION docs/kgenframeworksapidox.1 PRE-CREATION setup.py 083543f Diff: https://git.reviewboard.kde.org/r/124328/diff/ Testing --- Built and installed updated version and verified man pages were correctly installed and readable in the KDE man page viewer. Thanks, Scott Kitterman ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Jenkins-kde-ci: plasma-framework master kf5-qt5 » Linux,All,gcc - Build # 43 - Fixed!
GENERAL INFO BUILD SUCCESS Build URL: https://build.kde.org/job/plasma-framework%20master%20kf5-qt5/PLATFORM=Linux,Variation=All,compiler=gcc/43/ Project: PLATFORM=Linux,Variation=All,compiler=gcc Date of build: Mon, 13 Jul 2015 18:13:44 + Build duration: 6 min 57 sec CHANGE SET Revision 81dbda8e17a59b94e74fd6545bfe88c14324b593 by kde: (Rename software-updates.svgz to software.svgz) change: add src/desktoptheme/breeze/icons/software.svgz change: delete src/desktoptheme/breeze/icons/software-updates.svgz JUNIT RESULTS Name: (root) Failed: 0 test(s), Passed: 11 test(s), Skipped: 0 test(s), Total: 11 test(s) COBERTURA RESULTS Cobertura Coverage Report PACKAGES 5/7 (71%)FILES 63/104 (61%)CLASSES 63/104 (61%)LINE 3882/10012 (39%)CONDITIONAL 1893/2982 (63%) By packages autotests FILES 20/20 (100%)CLASSES 20/20 (100%)LINE 563/567 (99%)CONDITIONAL 352/634 (56%) src.declarativeimports.core FILES 7/20 (35%)CLASSES 7/20 (35%)LINE 286/1987 (14%)CONDITIONAL 83/146 (57%) src.plasma FILES 14/21 (67%)CLASSES 14/21 (67%)LINE 1612/3623 (44%)CONDITIONAL 798/1202 (66%) src.plasma.private FILES 18/26 (69%)CLASSES 18/26 (69%)LINE 945/1743 (54%)CONDITIONAL 410/600 (68%) src.plasma.scripting FILES 0/3 (0%)CLASSES 0/3 (0%)LINE 0/194 (0%)CONDITIONAL 0/0 (100%) src.plasmaquick FILES 4/11 (36%)CLASSES 4/11 (36%)LINE 476/1785 (27%)CONDITIONAL 250/400 (63%) src.plasmaquick.private FILES 0/3 (0%)CLASSES 0/3 (0%)LINE 0/113 (0%)CONDITIONAL 0/0 (100%)___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 124331: New proxy: KExtraColumnsProxyModel, allows to add columns to an existing model.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124331/ --- (Updated July 13, 2015, 8:31 a.m.) Review request for KDE Frameworks, Jesper Pedersen and Stephen Kelly. Changes --- add missing license header to autotest Repository: kitemmodels Description (updated) --- REVIEW: 124331 Diffs (updated) - autotests/CMakeLists.txt f98e87d49b965998f31c5ede1fefb03b159a150f autotests/kextracolumnsproxymodeltest.cpp PRE-CREATION autotests/test_model_helpers.h a44db8a9cb985f69b52be96f0ffe389ebd903d6f src/CMakeLists.txt 82d776f1fcc2f7b15e74f0ed47702ed570ce9892 src/kextracolumnsproxymodel.h PRE-CREATION src/kextracolumnsproxymodel.cpp PRE-CREATION Diff: https://git.reviewboard.kde.org/r/124331/diff/ Testing --- Wrote autotests (as extensive as possible); used this in one real project, AFAIK Jesper (blackie) did as well. Thanks, David Faure ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
