Re: Review Request 121299: Add NET::OSD window type
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121299/#review71540 --- src/netwm_def.h https://git.reviewboard.kde.org/r/121299/#comment49897 Please add a NON STANDARD the autotest for NET::Supported is not extended for the new supported type - Martin Gräßlin On Dec. 1, 2014, 7:49 p.m., Kai Uwe Broulik wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121299/ --- (Updated Dec. 1, 2014, 7:49 p.m.) Review request for KDE Frameworks, kwin and Martin Gräßlin. Repository: kwindowsystem Description --- This adds a NET::OSD window type for the OSD (eg. volume feedback) so it can be placed even ontop of active fullscreen windows in contrast to the Notifications. Not sure whether a kde netwm thing is required or we could just use some other method in KWin to decide placement. Also dunno what impact on ABI this has, I tried to only add enum values at the end. Diffs - autotests/netwininfotestclient.cpp 16ba4b3 src/netwm.cpp 1ccba32 src/netwm_def.h 0352f33 Diff: https://git.reviewboard.kde.org/r/121299/diff/ Testing --- In conjunction with Review 121300 these are now ontop of fullscreen windows. Thanks, Kai Uwe Broulik ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: KIO's KDirListerTest::testRefreshRootItem on RHEL7 and XFS
It looks like there are more failing tests, and in a non-deterministic manner. An example of a failing one is [1] while I've also seen all of them succeeding, [2]. Are you guys sure that these tests are really doing the right thing and that there are no failures when you run them a thousand times in a while loop? With kind regards, Jan [1] http://ci-logs.kde.flaska.net/06/206/3/check/check-kf5qt5-generic-test-el7/297d450/shell_output.log [2] http://ci-logs.kde.flaska.net/04/204/2/check/check-kf5qt5-generic-test-el7/ed72480/shell_output.log -- Trojitá, a fast Qt IMAP e-mail client -- http://trojita.flaska.net/ ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 121090: Move kio-mtp to kio-extras
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121090/#review71545 --- Looks good. It's too much code for me to review properly; but as it's basically the same as the KDE4 version. +1. mtp/CMakeLists.txt https://git.reviewboard.kde.org/r/121090/#comment49902 You're using i18n, but I don't see you set a translation domain anywhere. mtp/README https://git.reviewboard.kde.org/r/121090/#comment49900 This is all lies. mtp/kio_mtp.cpp https://git.reviewboard.kde.org/r/121090/#comment49903 who deletes this? mtp/kio_mtp.cpp https://git.reviewboard.kde.org/r/121090/#comment49901 this has _new_ in the functtion name, so I assume it allocates memory, if so who deletes it? It's not being done in this scope. - David Edmundson On Nov. 10, 2014, 9:30 a.m., Jan Grulich wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121090/ --- (Updated Nov. 10, 2014, 9:30 a.m.) Review request for KDE Frameworks, David Faure and Philipp Schmidt. Repository: kio-extras Description --- This patch moves kio-mtp from its frameworks branch to kio-extras. I believe that this kioslave should be with the rest and not alone somewhere without release. I also formatted it a bit to follow kdelibs coding style. Discussion about this move can be found here http://lists.kde.org/?l=kde-develm=141500903106761w=2 Diffs - mtp/kio_mtp_helpers.h PRE-CREATION mtp/kio_mtp_helpers.cpp PRE-CREATION mtp/mtp-network.desktop PRE-CREATION mtp/mtp.protocol PRE-CREATION mtp/solid_mtp.desktop PRE-CREATION mtp/filecache.cpp PRE-CREATION mtp/kio-mtp.kdev4 PRE-CREATION mtp/kio_mtp.h PRE-CREATION mtp/kio_mtp.cpp PRE-CREATION CMakeLists.txt 8f82688 mtp/CMakeLists.txt PRE-CREATION mtp/COPYING PRE-CREATION mtp/LICENCE PRE-CREATION mtp/Messages.sh PRE-CREATION mtp/README PRE-CREATION mtp/devicecache.h PRE-CREATION mtp/devicecache.cpp PRE-CREATION mtp/filecache.h PRE-CREATION Diff: https://git.reviewboard.kde.org/r/121090/diff/ Testing --- I have tested copying files, creating and removing folders in Dolphin with my Nexus 4 and everything seems to work fine. Thanks, Jan Grulich ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Review Request 121390: make Qt5 part build on Linux again
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121390/ --- Review request for KDE Frameworks, Qt KDE and Yichao Yu. Repository: qtcurve Description --- Yesterday's patches for OS X building broke the build of the Qt5 parts on Linux (and other Unix/X11 platforms). I had presumed that Q_WS_X11 would be defined in that context as it is when building Qt4 code, but apparently it isn't. This patch restores building on Unix/X11 by replacing `#ifdef Q_WS_X11` with `#if defined(Q_OS_UNIX) !defined(Q_OS_OSX)` please verify if that catches all possible contexts where X11 is to be used?! (Qt/Cygwin might use X11?) Diffs - qt5/style/blurhelper.cpp 5dcc95c qt5/style/qtcurve.cpp 7b0d7e6 qt5/style/qtcurve_plugin.cpp febc977 qt5/style/qtcurve_utils.cpp 728c26a qt5/style/shadowhelper.cpp a239cf1 qt5/style/utils.cpp 0680442 qt5/style/windowmanager.cpp 3c2bc1c Diff: https://git.reviewboard.kde.org/r/121390/diff/ Testing --- On KUbuntu 14.04 with Qt 5.3.2 and KF5 in the (sadly discontinued) Project Neon5 environment. Thanks, René J.V. Bertin ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 121390: make Qt5 theme build on Linux again
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121390/ --- (Updated Dec. 8, 2014, 2:38 p.m.) Review request for KDE Frameworks, Qt KDE and Yichao Yu. Summary (updated) - make Qt5 theme build on Linux again Repository: qtcurve Description --- Yesterday's patches for OS X building broke the build of the Qt5 parts on Linux (and other Unix/X11 platforms). I had presumed that Q_WS_X11 would be defined in that context as it is when building Qt4 code, but apparently it isn't. This patch restores building on Unix/X11 by replacing `#ifdef Q_WS_X11` with `#if defined(Q_OS_UNIX) !defined(Q_OS_OSX)` please verify if that catches all possible contexts where X11 is to be used?! (Qt/Cygwin might use X11?) Diffs - qt5/style/blurhelper.cpp 5dcc95c qt5/style/qtcurve.cpp 7b0d7e6 qt5/style/qtcurve_plugin.cpp febc977 qt5/style/qtcurve_utils.cpp 728c26a qt5/style/shadowhelper.cpp a239cf1 qt5/style/utils.cpp 0680442 qt5/style/windowmanager.cpp 3c2bc1c Diff: https://git.reviewboard.kde.org/r/121390/diff/ Testing --- On KUbuntu 14.04 with Qt 5.3.2 and KF5 in the (sadly discontinued) Project Neon5 environment. Thanks, René J.V. Bertin ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 121299: Add NET::OSD window type
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121299/ --- (Updated Dez. 8, 2014, 1:40 nachm.) Review request for KDE Frameworks, kwin and Martin Gräßlin. Changes --- Address comments Repository: kwindowsystem Description --- This adds a NET::OSD window type for the OSD (eg. volume feedback) so it can be placed even ontop of active fullscreen windows in contrast to the Notifications. Not sure whether a kde netwm thing is required or we could just use some other method in KWin to decide placement. Also dunno what impact on ABI this has, I tried to only add enum values at the end. Diffs (updated) - autotests/kwindowinfox11test.cpp 5c8ee8a autotests/netwininfotestclient.cpp 16ba4b3 src/netwm.cpp 1ccba32 src/netwm_def.h 0352f33 Diff: https://git.reviewboard.kde.org/r/121299/diff/ Testing --- In conjunction with Review 121300 these are now ontop of fullscreen windows. Thanks, Kai Uwe Broulik ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: KF 5.5.0 changelog
On Sat, Dec 6, 2014 at 3:59 PM, David Faure fa...@kde.org wrote: Here's the changelog I wrote for 5.5.0. Please everyone remember to use CHANGELOG in your commits, with a standalone description of the issue (i.e. which doesn't use the modified filenames as implicit context). I tried to grab what I could from the commit logs, but I don't always understand everything in e.g. plasma-framework. Looks good to me. +1 By the way, I tried to check the full log and I cannot find some tags for some frameworks (i.e. kcoreaddons and plasma-framework are the ones I looked into). Do you know what can be wrong? Thanks! Aleix ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 121390: make Qt5 theme build on Linux again
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121390/#review71553 --- this is wrong - please have a look at various frameworks on how to do it properly. In the end it should be: #if HAVE_X11 // x11 specific stuff #endif obviously it also needs a runtime check: if (QX11Info::isPlatformX11()) as we no longer should assume that X11 is the only platform on Unix(non OSX). - Martin Gräßlin On Dec. 8, 2014, 2:38 p.m., René J.V. Bertin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121390/ --- (Updated Dec. 8, 2014, 2:38 p.m.) Review request for KDE Frameworks, Qt KDE and Yichao Yu. Repository: qtcurve Description --- Yesterday's patches for OS X building broke the build of the Qt5 parts on Linux (and other Unix/X11 platforms). I had presumed that Q_WS_X11 would be defined in that context as it is when building Qt4 code, but apparently it isn't. This patch restores building on Unix/X11 by replacing `#ifdef Q_WS_X11` with `#if defined(Q_OS_UNIX) !defined(Q_OS_OSX)` please verify if that catches all possible contexts where X11 is to be used?! (Qt/Cygwin might use X11?) Diffs - qt5/style/blurhelper.cpp 5dcc95c qt5/style/qtcurve.cpp 7b0d7e6 qt5/style/qtcurve_plugin.cpp febc977 qt5/style/qtcurve_utils.cpp 728c26a qt5/style/shadowhelper.cpp a239cf1 qt5/style/utils.cpp 0680442 qt5/style/windowmanager.cpp 3c2bc1c Diff: https://git.reviewboard.kde.org/r/121390/diff/ Testing --- On KUbuntu 14.04 with Qt 5.3.2 and KF5 in the (sadly discontinued) Project Neon5 environment. Thanks, René J.V. Bertin ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 121299: Add NET::OSD window type
On Dec. 8, 2014, 10:34 a.m., Kai Uwe Broulik wrote: the autotest for NET::Supported is not extended for the new supported type I'm still missing the extension of the autotest. In fact I'd expect that at least one autotest is currently failing. - Martin --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121299/#review71540 --- On Dec. 8, 2014, 2:40 p.m., Kai Uwe Broulik wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121299/ --- (Updated Dec. 8, 2014, 2:40 p.m.) Review request for KDE Frameworks, kwin and Martin Gräßlin. Repository: kwindowsystem Description --- This adds a NET::OSD window type for the OSD (eg. volume feedback) so it can be placed even ontop of active fullscreen windows in contrast to the Notifications. Not sure whether a kde netwm thing is required or we could just use some other method in KWin to decide placement. Also dunno what impact on ABI this has, I tried to only add enum values at the end. Diffs - autotests/kwindowinfox11test.cpp 5c8ee8a autotests/netwininfotestclient.cpp 16ba4b3 src/netwm.cpp 1ccba32 src/netwm_def.h 0352f33 Diff: https://git.reviewboard.kde.org/r/121299/diff/ Testing --- In conjunction with Review 121300 these are now ontop of fullscreen windows. Thanks, Kai Uwe Broulik ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 121090: Move kio-mtp to kio-extras
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121090/ --- (Updated Pro. 8, 2014, 2:34 odp.) Review request for KDE Frameworks, David Faure and Philipp Schmidt. Changes --- Updated diff fixing mentioned issues. Repository: kio-extras Description --- This patch moves kio-mtp from its frameworks branch to kio-extras. I believe that this kioslave should be with the rest and not alone somewhere without release. I also formatted it a bit to follow kdelibs coding style. Discussion about this move can be found here http://lists.kde.org/?l=kde-develm=141500903106761w=2 Diffs (updated) - CMakeLists.txt 8f82688 mtp/CMakeLists.txt PRE-CREATION mtp/COPYING PRE-CREATION mtp/LICENCE PRE-CREATION mtp/Messages.sh PRE-CREATION mtp/README PRE-CREATION mtp/devicecache.h PRE-CREATION mtp/devicecache.cpp PRE-CREATION mtp/filecache.h PRE-CREATION mtp/filecache.cpp PRE-CREATION mtp/kio-mtp.kdev4 PRE-CREATION mtp/kio_mtp.h PRE-CREATION mtp/kio_mtp.cpp PRE-CREATION mtp/kio_mtp_helpers.h PRE-CREATION mtp/kio_mtp_helpers.cpp PRE-CREATION mtp/mtp-network.desktop PRE-CREATION mtp/mtp.protocol PRE-CREATION mtp/solid_mtp.desktop PRE-CREATION Diff: https://git.reviewboard.kde.org/r/121090/diff/ Testing --- I have tested copying files, creating and removing folders in Dolphin with my Nexus 4 and everything seems to work fine. Thanks, Jan Grulich ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 121390: make Qt5 theme build on Linux again
On Dec. 8, 2014, 3:07 p.m., Martin Gräßlin wrote: this is wrong - please have a look at various frameworks on how to do it properly. In the end it should be: #if HAVE_X11 // x11 specific stuff #endif obviously it also needs a runtime check: if (QX11Info::isPlatformX11()) as we no longer should assume that X11 is the only platform on Unix(non OSX). I found a reference to HAVE_X11 online, but that token is not defined. Note also that the Qt5 theme is supposed to build without KF5 too, for pure Qt5 applications, so this kind of token should rather be provided by the Qt cmake files rather than KDE's. I'll leave the runtime checks to the QtCurve devs, as they obviously should be made in lots of locations and it's their call. I myself don't see the point in doing a runtime check whether a platform is X11, though. It's known at build time if a platform is X11. Doing a runtime check before each theming action if `Q11Info::isX11Active()` (or some such call) seems to be an expensive concession to a rather improbable set-up. If distros really decide to give the user a choice between X11 and Wayland at login ... let them figure out how to streamline that first, and then add runtime checks for the active graphics backend where really needed... (In fact, I myself would avoid anything tied to the display layer as much as possible; the fact I had to go back in a few months after the previous porting effort goes to show how easy it is to break builds on other platforms with that kind of functionality - as if my own error wasn't enough already.) - René J.V. --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121390/#review71553 --- On Dec. 8, 2014, 2:38 p.m., René J.V. Bertin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121390/ --- (Updated Dec. 8, 2014, 2:38 p.m.) Review request for KDE Frameworks, Qt KDE and Yichao Yu. Repository: qtcurve Description --- Yesterday's patches for OS X building broke the build of the Qt5 parts on Linux (and other Unix/X11 platforms). I had presumed that Q_WS_X11 would be defined in that context as it is when building Qt4 code, but apparently it isn't. This patch restores building on Unix/X11 by replacing `#ifdef Q_WS_X11` with `#if defined(Q_OS_UNIX) !defined(Q_OS_OSX)` please verify if that catches all possible contexts where X11 is to be used?! (Qt/Cygwin might use X11?) Diffs - qt5/style/blurhelper.cpp 5dcc95c qt5/style/qtcurve.cpp 7b0d7e6 qt5/style/qtcurve_plugin.cpp febc977 qt5/style/qtcurve_utils.cpp 728c26a qt5/style/shadowhelper.cpp a239cf1 qt5/style/utils.cpp 0680442 qt5/style/windowmanager.cpp 3c2bc1c Diff: https://git.reviewboard.kde.org/r/121390/diff/ Testing --- On KUbuntu 14.04 with Qt 5.3.2 and KF5 in the (sadly discontinued) Project Neon5 environment. Thanks, René J.V. Bertin ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 121299: Add NET::OSD window type
On Dez. 8, 2014, 9:34 vorm., Kai Uwe Broulik wrote: the autotest for NET::Supported is not extended for the new supported type Martin Gräßlin wrote: I'm still missing the extension of the autotest. In fact I'd expect that at least one autotest is currently failing. Could you be more specific about which test you are referring to? I added OSD testdata whereever I found a reference to the Notification type. - Kai Uwe --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121299/#review71540 --- On Dez. 8, 2014, 1:40 nachm., Kai Uwe Broulik wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121299/ --- (Updated Dez. 8, 2014, 1:40 nachm.) Review request for KDE Frameworks, kwin and Martin Gräßlin. Repository: kwindowsystem Description --- This adds a NET::OSD window type for the OSD (eg. volume feedback) so it can be placed even ontop of active fullscreen windows in contrast to the Notifications. Not sure whether a kde netwm thing is required or we could just use some other method in KWin to decide placement. Also dunno what impact on ABI this has, I tried to only add enum values at the end. Diffs - autotests/kwindowinfox11test.cpp 5c8ee8a autotests/netwininfotestclient.cpp 16ba4b3 src/netwm.cpp 1ccba32 src/netwm_def.h 0352f33 Diff: https://git.reviewboard.kde.org/r/121299/diff/ Testing --- In conjunction with Review 121300 these are now ontop of fullscreen windows. Thanks, Kai Uwe Broulik ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 121299: Add NET::OSD window type
On Dec. 8, 2014, 10:34 a.m., Kai Uwe Broulik wrote: the autotest for NET::Supported is not extended for the new supported type Martin Gräßlin wrote: I'm still missing the extension of the autotest. In fact I'd expect that at least one autotest is currently failing. Kai Uwe Broulik wrote: Could you be more specific about which test you are referring to? I added OSD testdata whereever I found a reference to the Notification type. NetRootInfoTestWM::testSupported() - Martin --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121299/#review71540 --- On Dec. 8, 2014, 2:40 p.m., Kai Uwe Broulik wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121299/ --- (Updated Dec. 8, 2014, 2:40 p.m.) Review request for KDE Frameworks, kwin and Martin Gräßlin. Repository: kwindowsystem Description --- This adds a NET::OSD window type for the OSD (eg. volume feedback) so it can be placed even ontop of active fullscreen windows in contrast to the Notifications. Not sure whether a kde netwm thing is required or we could just use some other method in KWin to decide placement. Also dunno what impact on ABI this has, I tried to only add enum values at the end. Diffs - autotests/kwindowinfox11test.cpp 5c8ee8a autotests/netwininfotestclient.cpp 16ba4b3 src/netwm.cpp 1ccba32 src/netwm_def.h 0352f33 Diff: https://git.reviewboard.kde.org/r/121299/diff/ Testing --- In conjunction with Review 121300 these are now ontop of fullscreen windows. Thanks, Kai Uwe Broulik ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 121390: make Qt5 theme build on Linux again
On Dec. 8, 2014, 3:07 p.m., Martin Gräßlin wrote: this is wrong - please have a look at various frameworks on how to do it properly. In the end it should be: #if HAVE_X11 // x11 specific stuff #endif obviously it also needs a runtime check: if (QX11Info::isPlatformX11()) as we no longer should assume that X11 is the only platform on Unix(non OSX). René J.V. Bertin wrote: I found a reference to HAVE_X11 online, but that token is not defined. Note also that the Qt5 theme is supposed to build without KF5 too, for pure Qt5 applications, so this kind of token should rather be provided by the Qt cmake files rather than KDE's. I'll leave the runtime checks to the QtCurve devs, as they obviously should be made in lots of locations and it's their call. I myself don't see the point in doing a runtime check whether a platform is X11, though. It's known at build time if a platform is X11. Doing a runtime check before each theming action if `Q11Info::isX11Active()` (or some such call) seems to be an expensive concession to a rather improbable set-up. If distros really decide to give the user a choice between X11 and Wayland at login ... let them figure out how to streamline that first, and then add runtime checks for the active graphics backend where really needed... (In fact, I myself would avoid anything tied to the display layer as much as possible; the fact I had to go back in a few months after the previous porting effort goes to show how easy it is to break builds on other platforms with that kind of functionality - as if my own error wasn't enough already.) HAVE_X11 is neither defined by Qt5 nor by KF5. It needs to be set manually depending on whether the source is built with or without X11 support. Concerning the runtime check: kwrite -platform wayland and your app is going to crash like hell if there is no runtime checks. - Martin --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121390/#review71553 --- On Dec. 8, 2014, 2:38 p.m., René J.V. Bertin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121390/ --- (Updated Dec. 8, 2014, 2:38 p.m.) Review request for KDE Frameworks, Qt KDE and Yichao Yu. Repository: qtcurve Description --- Yesterday's patches for OS X building broke the build of the Qt5 parts on Linux (and other Unix/X11 platforms). I had presumed that Q_WS_X11 would be defined in that context as it is when building Qt4 code, but apparently it isn't. This patch restores building on Unix/X11 by replacing `#ifdef Q_WS_X11` with `#if defined(Q_OS_UNIX) !defined(Q_OS_OSX)` please verify if that catches all possible contexts where X11 is to be used?! (Qt/Cygwin might use X11?) Diffs - qt5/style/blurhelper.cpp 5dcc95c qt5/style/qtcurve.cpp 7b0d7e6 qt5/style/qtcurve_plugin.cpp febc977 qt5/style/qtcurve_utils.cpp 728c26a qt5/style/shadowhelper.cpp a239cf1 qt5/style/utils.cpp 0680442 qt5/style/windowmanager.cpp 3c2bc1c Diff: https://git.reviewboard.kde.org/r/121390/diff/ Testing --- On KUbuntu 14.04 with Qt 5.3.2 and KF5 in the (sadly discontinued) Project Neon5 environment. Thanks, René J.V. Bertin ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: libnm-qt - a new KF5 framework for Tier 1?
Hi, I would like to finally finish this. According to [1] I believe we satisfy all conditions to become a framework. We also already have own bugzilla component [2] and repository in reviewboard. There were also no objections except a comment from David Edmunson regarding propertiesChanged() slots being in public API, but this is already solved. It's been almost a month since my first email so can you please take libnm-qt among other frameworks and add it to kde:sysadmin/release-tools? I'll then request moving the repo under frameworks and update version accordingly. [1] - https://community.kde.org/Frameworks/CreationGuidelines [2] - https://bugs.kde.org/describecomponents.cgi?product=frameworks-networkmanager-qt Thanks a lot. Regards, Jan -- Jan Grulich Red Hat Czech, s.r.o jgrul...@redhat.com ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 121390: make Qt5 theme build on Linux again
On Dec. 8, 2014, 3:07 p.m., Martin Gräßlin wrote: this is wrong - please have a look at various frameworks on how to do it properly. In the end it should be: #if HAVE_X11 // x11 specific stuff #endif obviously it also needs a runtime check: if (QX11Info::isPlatformX11()) as we no longer should assume that X11 is the only platform on Unix(non OSX). René J.V. Bertin wrote: I found a reference to HAVE_X11 online, but that token is not defined. Note also that the Qt5 theme is supposed to build without KF5 too, for pure Qt5 applications, so this kind of token should rather be provided by the Qt cmake files rather than KDE's. I'll leave the runtime checks to the QtCurve devs, as they obviously should be made in lots of locations and it's their call. I myself don't see the point in doing a runtime check whether a platform is X11, though. It's known at build time if a platform is X11. Doing a runtime check before each theming action if `Q11Info::isX11Active()` (or some such call) seems to be an expensive concession to a rather improbable set-up. If distros really decide to give the user a choice between X11 and Wayland at login ... let them figure out how to streamline that first, and then add runtime checks for the active graphics backend where really needed... (In fact, I myself would avoid anything tied to the display layer as much as possible; the fact I had to go back in a few months after the previous porting effort goes to show how easy it is to break builds on other platforms with that kind of functionality - as if my own error wasn't enough already.) Martin Gräßlin wrote: HAVE_X11 is neither defined by Qt5 nor by KF5. It needs to be set manually depending on whether the source is built with or without X11 support. Concerning the runtime check: kwrite -platform wayland and your app is going to crash like hell if there is no runtime checks. ``` neon5-exec /opt/project-neon5/bin/kwrite -platform wayland This application failed to start because it could not find or load the Qt platform plugin wayland. Available platform plugins are: linuxfb, minimal, offscreen, xcb. Reinstalling the application may fix this problem. Abort (core dumped) ``` Right, so with runtime checks it doesn't crash, it just self-destructs. Very useful difference :) Of course an application will crash if it tries to use an unavailable displaying method, or if the linker tries to load shared libraries that aren't present. In fact, with X11 it might actually exit nicely with a message about a display rather than crash. This just underlines my point: the only use for runtime checks in this context if is the same binaries are supposed to work with multiple displaying backends (or platform plugins). Whether QtCurve ought to support that is a call for its developers to make, like I said above. The only way to do that properly without (too much) overhead is to do the check at initialisation time rather than preceding each backend-specific call, i.e. use functionpointers or some OO/C++ alternative. I don't know how preferable Wayland is to X11; without that I see only an interest for people working on Wayland development under X11 for this kind of runtime switch support. To put this into context: I've often thought how it'd be nice if Qt-mac would be able to use X11, but I've always dismissed the possibility that that might be a runtime switch, exactly because it would introduce too much overhead and/or complexity for a feature that'd be used only rarely. - René J.V. --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121390/#review71553 --- On Dec. 8, 2014, 2:38 p.m., René J.V. Bertin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121390/ --- (Updated Dec. 8, 2014, 2:38 p.m.) Review request for KDE Frameworks, Qt KDE and Yichao Yu. Repository: qtcurve Description --- Yesterday's patches for OS X building broke the build of the Qt5 parts on Linux (and other Unix/X11 platforms). I had presumed that Q_WS_X11 would be defined in that context as it is when building Qt4 code, but apparently it isn't. This patch restores building on Unix/X11 by replacing `#ifdef Q_WS_X11` with `#if defined(Q_OS_UNIX) !defined(Q_OS_OSX)` please verify if that catches all possible contexts where X11 is to be used?! (Qt/Cygwin might use X11?) Diffs - qt5/style/blurhelper.cpp 5dcc95c qt5/style/qtcurve.cpp 7b0d7e6 qt5/style/qtcurve_plugin.cpp febc977 qt5/style/qtcurve_utils.cpp 728c26a qt5/style/shadowhelper.cpp a239cf1
Re: Review Request 121390: make Qt5 theme build on Linux again
On Dez. 8, 2014, 2:07 nachm., Martin Gräßlin wrote: this is wrong - please have a look at various frameworks on how to do it properly. In the end it should be: #if HAVE_X11 // x11 specific stuff #endif obviously it also needs a runtime check: if (QX11Info::isPlatformX11()) as we no longer should assume that X11 is the only platform on Unix(non OSX). René J.V. Bertin wrote: I found a reference to HAVE_X11 online, but that token is not defined. Note also that the Qt5 theme is supposed to build without KF5 too, for pure Qt5 applications, so this kind of token should rather be provided by the Qt cmake files rather than KDE's. I'll leave the runtime checks to the QtCurve devs, as they obviously should be made in lots of locations and it's their call. I myself don't see the point in doing a runtime check whether a platform is X11, though. It's known at build time if a platform is X11. Doing a runtime check before each theming action if `Q11Info::isX11Active()` (or some such call) seems to be an expensive concession to a rather improbable set-up. If distros really decide to give the user a choice between X11 and Wayland at login ... let them figure out how to streamline that first, and then add runtime checks for the active graphics backend where really needed... (In fact, I myself would avoid anything tied to the display layer as much as possible; the fact I had to go back in a few months after the previous porting effort goes to show how easy it is to break builds on other platforms with that kind of functionality - as if my own error wasn't enough already.) Martin Gräßlin wrote: HAVE_X11 is neither defined by Qt5 nor by KF5. It needs to be set manually depending on whether the source is built with or without X11 support. Concerning the runtime check: kwrite -platform wayland and your app is going to crash like hell if there is no runtime checks. René J.V. Bertin wrote: ``` neon5-exec /opt/project-neon5/bin/kwrite -platform wayland This application failed to start because it could not find or load the Qt platform plugin wayland. Available platform plugins are: linuxfb, minimal, offscreen, xcb. Reinstalling the application may fix this problem. Abort (core dumped) ``` Right, so with runtime checks it doesn't crash, it just self-destructs. Very useful difference :) Of course an application will crash if it tries to use an unavailable displaying method, or if the linker tries to load shared libraries that aren't present. In fact, with X11 it might actually exit nicely with a message about a display rather than crash. This just underlines my point: the only use for runtime checks in this context if is the same binaries are supposed to work with multiple displaying backends (or platform plugins). Whether QtCurve ought to support that is a call for its developers to make, like I said above. The only way to do that properly without (too much) overhead is to do the check at initialisation time rather than preceding each backend-specific call, i.e. use functionpointers or some OO/C++ alternative. I don't know how preferable Wayland is to X11; without that I see only an interest for people working on Wayland development under X11 for this kind of runtime switch support. To put this into context: I've often thought how it'd be nice if Qt-mac would be able to use X11, but I've always dismissed the possibility that that might be a runtime switch, exactly because it would introduce too much overhead and/or complexity for a feature that'd be used only rarely. Right, so with runtime checks it doesn't crash, it just self-destructs. You're missing the point entirely. The compile time checks have no implication on the runtime environment. Of course you cannot use the wayland platform plugin if it's not available, but you *can* compile Qt/KDE w/ X11 and wayland present - but making X11 calls when running on the wayland PP will crash the application - thus you must check whether you're operating on X11/xdg at runtime. Also testing for UNIX but not OSX to make X11 calls is plain wrong. Could be framebuffer console or wayland and no X11 installed at all. - Thomas --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121390/#review71553 --- On Dez. 8, 2014, 1:38 nachm., René J.V. Bertin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121390/ --- (Updated Dez. 8, 2014, 1:38 nachm.) Review request for KDE Frameworks, Qt KDE and Yichao Yu. Repository: qtcurve
Re: Review Request 121390: make Qt5 theme build on Linux again
On Dec. 8, 2014, 3:07 p.m., Martin Gräßlin wrote: this is wrong - please have a look at various frameworks on how to do it properly. In the end it should be: #if HAVE_X11 // x11 specific stuff #endif obviously it also needs a runtime check: if (QX11Info::isPlatformX11()) as we no longer should assume that X11 is the only platform on Unix(non OSX). René J.V. Bertin wrote: I found a reference to HAVE_X11 online, but that token is not defined. Note also that the Qt5 theme is supposed to build without KF5 too, for pure Qt5 applications, so this kind of token should rather be provided by the Qt cmake files rather than KDE's. I'll leave the runtime checks to the QtCurve devs, as they obviously should be made in lots of locations and it's their call. I myself don't see the point in doing a runtime check whether a platform is X11, though. It's known at build time if a platform is X11. Doing a runtime check before each theming action if `Q11Info::isX11Active()` (or some such call) seems to be an expensive concession to a rather improbable set-up. If distros really decide to give the user a choice between X11 and Wayland at login ... let them figure out how to streamline that first, and then add runtime checks for the active graphics backend where really needed... (In fact, I myself would avoid anything tied to the display layer as much as possible; the fact I had to go back in a few months after the previous porting effort goes to show how easy it is to break builds on other platforms with that kind of functionality - as if my own error wasn't enough already.) Martin Gräßlin wrote: HAVE_X11 is neither defined by Qt5 nor by KF5. It needs to be set manually depending on whether the source is built with or without X11 support. Concerning the runtime check: kwrite -platform wayland and your app is going to crash like hell if there is no runtime checks. René J.V. Bertin wrote: ``` neon5-exec /opt/project-neon5/bin/kwrite -platform wayland This application failed to start because it could not find or load the Qt platform plugin wayland. Available platform plugins are: linuxfb, minimal, offscreen, xcb. Reinstalling the application may fix this problem. Abort (core dumped) ``` Right, so with runtime checks it doesn't crash, it just self-destructs. Very useful difference :) Of course an application will crash if it tries to use an unavailable displaying method, or if the linker tries to load shared libraries that aren't present. In fact, with X11 it might actually exit nicely with a message about a display rather than crash. This just underlines my point: the only use for runtime checks in this context if is the same binaries are supposed to work with multiple displaying backends (or platform plugins). Whether QtCurve ought to support that is a call for its developers to make, like I said above. The only way to do that properly without (too much) overhead is to do the check at initialisation time rather than preceding each backend-specific call, i.e. use functionpointers or some OO/C++ alternative. I don't know how preferable Wayland is to X11; without that I see only an interest for people working on Wayland development under X11 for this kind of runtime switch support. To put this into context: I've often thought how it'd be nice if Qt-mac would be able to use X11, but I've always dismissed the possibility that that might be a runtime switch, exactly because it would introduce too much overhead and/or complexity for a feature that'd be used only rarely. Thomas Lübking wrote: Right, so with runtime checks it doesn't crash, it just self-destructs. You're missing the point entirely. The compile time checks have no implication on the runtime environment. Of course you cannot use the wayland platform plugin if it's not available, but you *can* compile Qt/KDE w/ X11 and wayland present - but making X11 calls when running on the wayland PP will crash the application - thus you must check whether you're operating on X11/xdg at runtime. Also testing for UNIX but not OSX to make X11 calls is plain wrong. Could be framebuffer console or wayland and no X11 installed at all. for more information please see my blog post: http://blog.martin-graesslin.com/blog/2014/02/running-frameworks-powered-applications-on-wayland/ Btw. the QtWayland PPA will be available starting with Qt 5.4 - a version I'm already using. - Martin --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121390/#review71553 --- On Dec. 8, 2014, 2:38 p.m., René J.V. Bertin wrote:
Re: Review Request 121390: make Qt5 theme build on Linux again
On Dec. 8, 2014, 3:07 p.m., Martin Gräßlin wrote: this is wrong - please have a look at various frameworks on how to do it properly. In the end it should be: #if HAVE_X11 // x11 specific stuff #endif obviously it also needs a runtime check: if (QX11Info::isPlatformX11()) as we no longer should assume that X11 is the only platform on Unix(non OSX). René J.V. Bertin wrote: I found a reference to HAVE_X11 online, but that token is not defined. Note also that the Qt5 theme is supposed to build without KF5 too, for pure Qt5 applications, so this kind of token should rather be provided by the Qt cmake files rather than KDE's. I'll leave the runtime checks to the QtCurve devs, as they obviously should be made in lots of locations and it's their call. I myself don't see the point in doing a runtime check whether a platform is X11, though. It's known at build time if a platform is X11. Doing a runtime check before each theming action if `Q11Info::isX11Active()` (or some such call) seems to be an expensive concession to a rather improbable set-up. If distros really decide to give the user a choice between X11 and Wayland at login ... let them figure out how to streamline that first, and then add runtime checks for the active graphics backend where really needed... (In fact, I myself would avoid anything tied to the display layer as much as possible; the fact I had to go back in a few months after the previous porting effort goes to show how easy it is to break builds on other platforms with that kind of functionality - as if my own error wasn't enough already.) Martin Gräßlin wrote: HAVE_X11 is neither defined by Qt5 nor by KF5. It needs to be set manually depending on whether the source is built with or without X11 support. Concerning the runtime check: kwrite -platform wayland and your app is going to crash like hell if there is no runtime checks. René J.V. Bertin wrote: ``` neon5-exec /opt/project-neon5/bin/kwrite -platform wayland This application failed to start because it could not find or load the Qt platform plugin wayland. Available platform plugins are: linuxfb, minimal, offscreen, xcb. Reinstalling the application may fix this problem. Abort (core dumped) ``` Right, so with runtime checks it doesn't crash, it just self-destructs. Very useful difference :) Of course an application will crash if it tries to use an unavailable displaying method, or if the linker tries to load shared libraries that aren't present. In fact, with X11 it might actually exit nicely with a message about a display rather than crash. This just underlines my point: the only use for runtime checks in this context if is the same binaries are supposed to work with multiple displaying backends (or platform plugins). Whether QtCurve ought to support that is a call for its developers to make, like I said above. The only way to do that properly without (too much) overhead is to do the check at initialisation time rather than preceding each backend-specific call, i.e. use functionpointers or some OO/C++ alternative. I don't know how preferable Wayland is to X11; without that I see only an interest for people working on Wayland development under X11 for this kind of runtime switch support. To put this into context: I've often thought how it'd be nice if Qt-mac would be able to use X11, but I've always dismissed the possibility that that might be a runtime switch, exactly because it would introduce too much overhead and/or complexity for a feature that'd be used only rarely. Thomas Lübking wrote: Right, so with runtime checks it doesn't crash, it just self-destructs. You're missing the point entirely. The compile time checks have no implication on the runtime environment. Of course you cannot use the wayland platform plugin if it's not available, but you *can* compile Qt/KDE w/ X11 and wayland present - but making X11 calls when running on the wayland PP will crash the application - thus you must check whether you're operating on X11/xdg at runtime. Also testing for UNIX but not OSX to make X11 calls is plain wrong. Could be framebuffer console or wayland and no X11 installed at all. Martin Gräßlin wrote: for more information please see my blog post: http://blog.martin-graesslin.com/blog/2014/02/running-frameworks-powered-applications-on-wayland/ Btw. the QtWayland PPA will be available starting with Qt 5.4 - a version I'm already using. @Thomas: we're not talking about compile time checks. Those evidently don't have any implication on the runtime environment (if done correctly :)). I guess my point is simply that the fact that you can (= it's possible to) compile Qt/KDE with every conceivable display/rendering engine present doesn't mean that
Re: Review Request 121390: make Qt5 theme build on Linux again
On Dec. 8, 2014, 3:07 p.m., Martin Gräßlin wrote: this is wrong - please have a look at various frameworks on how to do it properly. In the end it should be: #if HAVE_X11 // x11 specific stuff #endif obviously it also needs a runtime check: if (QX11Info::isPlatformX11()) as we no longer should assume that X11 is the only platform on Unix(non OSX). René J.V. Bertin wrote: I found a reference to HAVE_X11 online, but that token is not defined. Note also that the Qt5 theme is supposed to build without KF5 too, for pure Qt5 applications, so this kind of token should rather be provided by the Qt cmake files rather than KDE's. I'll leave the runtime checks to the QtCurve devs, as they obviously should be made in lots of locations and it's their call. I myself don't see the point in doing a runtime check whether a platform is X11, though. It's known at build time if a platform is X11. Doing a runtime check before each theming action if `Q11Info::isX11Active()` (or some such call) seems to be an expensive concession to a rather improbable set-up. If distros really decide to give the user a choice between X11 and Wayland at login ... let them figure out how to streamline that first, and then add runtime checks for the active graphics backend where really needed... (In fact, I myself would avoid anything tied to the display layer as much as possible; the fact I had to go back in a few months after the previous porting effort goes to show how easy it is to break builds on other platforms with that kind of functionality - as if my own error wasn't enough already.) Martin Gräßlin wrote: HAVE_X11 is neither defined by Qt5 nor by KF5. It needs to be set manually depending on whether the source is built with or without X11 support. Concerning the runtime check: kwrite -platform wayland and your app is going to crash like hell if there is no runtime checks. René J.V. Bertin wrote: ``` neon5-exec /opt/project-neon5/bin/kwrite -platform wayland This application failed to start because it could not find or load the Qt platform plugin wayland. Available platform plugins are: linuxfb, minimal, offscreen, xcb. Reinstalling the application may fix this problem. Abort (core dumped) ``` Right, so with runtime checks it doesn't crash, it just self-destructs. Very useful difference :) Of course an application will crash if it tries to use an unavailable displaying method, or if the linker tries to load shared libraries that aren't present. In fact, with X11 it might actually exit nicely with a message about a display rather than crash. This just underlines my point: the only use for runtime checks in this context if is the same binaries are supposed to work with multiple displaying backends (or platform plugins). Whether QtCurve ought to support that is a call for its developers to make, like I said above. The only way to do that properly without (too much) overhead is to do the check at initialisation time rather than preceding each backend-specific call, i.e. use functionpointers or some OO/C++ alternative. I don't know how preferable Wayland is to X11; without that I see only an interest for people working on Wayland development under X11 for this kind of runtime switch support. To put this into context: I've often thought how it'd be nice if Qt-mac would be able to use X11, but I've always dismissed the possibility that that might be a runtime switch, exactly because it would introduce too much overhead and/or complexity for a feature that'd be used only rarely. Thomas Lübking wrote: Right, so with runtime checks it doesn't crash, it just self-destructs. You're missing the point entirely. The compile time checks have no implication on the runtime environment. Of course you cannot use the wayland platform plugin if it's not available, but you *can* compile Qt/KDE w/ X11 and wayland present - but making X11 calls when running on the wayland PP will crash the application - thus you must check whether you're operating on X11/xdg at runtime. Also testing for UNIX but not OSX to make X11 calls is plain wrong. Could be framebuffer console or wayland and no X11 installed at all. Martin Gräßlin wrote: for more information please see my blog post: http://blog.martin-graesslin.com/blog/2014/02/running-frameworks-powered-applications-on-wayland/ Btw. the QtWayland PPA will be available starting with Qt 5.4 - a version I'm already using. René J.V. Bertin wrote: @Thomas: we're not talking about compile time checks. Those evidently don't have any implication on the runtime environment (if done correctly :)). I guess my point is simply that the fact that you can (= it's possible to) compile Qt/KDE with every conceivable display/rendering
Re: Review Request 121390: make Qt5 theme build on Linux again
On Dec. 8, 2014, 3:07 p.m., Martin Gräßlin wrote: this is wrong - please have a look at various frameworks on how to do it properly. In the end it should be: #if HAVE_X11 // x11 specific stuff #endif obviously it also needs a runtime check: if (QX11Info::isPlatformX11()) as we no longer should assume that X11 is the only platform on Unix(non OSX). René J.V. Bertin wrote: I found a reference to HAVE_X11 online, but that token is not defined. Note also that the Qt5 theme is supposed to build without KF5 too, for pure Qt5 applications, so this kind of token should rather be provided by the Qt cmake files rather than KDE's. I'll leave the runtime checks to the QtCurve devs, as they obviously should be made in lots of locations and it's their call. I myself don't see the point in doing a runtime check whether a platform is X11, though. It's known at build time if a platform is X11. Doing a runtime check before each theming action if `Q11Info::isX11Active()` (or some such call) seems to be an expensive concession to a rather improbable set-up. If distros really decide to give the user a choice between X11 and Wayland at login ... let them figure out how to streamline that first, and then add runtime checks for the active graphics backend where really needed... (In fact, I myself would avoid anything tied to the display layer as much as possible; the fact I had to go back in a few months after the previous porting effort goes to show how easy it is to break builds on other platforms with that kind of functionality - as if my own error wasn't enough already.) Martin Gräßlin wrote: HAVE_X11 is neither defined by Qt5 nor by KF5. It needs to be set manually depending on whether the source is built with or without X11 support. Concerning the runtime check: kwrite -platform wayland and your app is going to crash like hell if there is no runtime checks. René J.V. Bertin wrote: ``` neon5-exec /opt/project-neon5/bin/kwrite -platform wayland This application failed to start because it could not find or load the Qt platform plugin wayland. Available platform plugins are: linuxfb, minimal, offscreen, xcb. Reinstalling the application may fix this problem. Abort (core dumped) ``` Right, so with runtime checks it doesn't crash, it just self-destructs. Very useful difference :) Of course an application will crash if it tries to use an unavailable displaying method, or if the linker tries to load shared libraries that aren't present. In fact, with X11 it might actually exit nicely with a message about a display rather than crash. This just underlines my point: the only use for runtime checks in this context if is the same binaries are supposed to work with multiple displaying backends (or platform plugins). Whether QtCurve ought to support that is a call for its developers to make, like I said above. The only way to do that properly without (too much) overhead is to do the check at initialisation time rather than preceding each backend-specific call, i.e. use functionpointers or some OO/C++ alternative. I don't know how preferable Wayland is to X11; without that I see only an interest for people working on Wayland development under X11 for this kind of runtime switch support. To put this into context: I've often thought how it'd be nice if Qt-mac would be able to use X11, but I've always dismissed the possibility that that might be a runtime switch, exactly because it would introduce too much overhead and/or complexity for a feature that'd be used only rarely. Thomas Lübking wrote: Right, so with runtime checks it doesn't crash, it just self-destructs. You're missing the point entirely. The compile time checks have no implication on the runtime environment. Of course you cannot use the wayland platform plugin if it's not available, but you *can* compile Qt/KDE w/ X11 and wayland present - but making X11 calls when running on the wayland PP will crash the application - thus you must check whether you're operating on X11/xdg at runtime. Also testing for UNIX but not OSX to make X11 calls is plain wrong. Could be framebuffer console or wayland and no X11 installed at all. Martin Gräßlin wrote: for more information please see my blog post: http://blog.martin-graesslin.com/blog/2014/02/running-frameworks-powered-applications-on-wayland/ Btw. the QtWayland PPA will be available starting with Qt 5.4 - a version I'm already using. René J.V. Bertin wrote: @Thomas: we're not talking about compile time checks. Those evidently don't have any implication on the runtime environment (if done correctly :)). I guess my point is simply that the fact that you can (= it's possible to) compile Qt/KDE with every conceivable display/rendering
Re: Review Request 121390: make Qt5 theme build on Linux again
On Dec. 8, 2014, 3:07 p.m., Martin Gräßlin wrote: this is wrong - please have a look at various frameworks on how to do it properly. In the end it should be: #if HAVE_X11 // x11 specific stuff #endif obviously it also needs a runtime check: if (QX11Info::isPlatformX11()) as we no longer should assume that X11 is the only platform on Unix(non OSX). René J.V. Bertin wrote: I found a reference to HAVE_X11 online, but that token is not defined. Note also that the Qt5 theme is supposed to build without KF5 too, for pure Qt5 applications, so this kind of token should rather be provided by the Qt cmake files rather than KDE's. I'll leave the runtime checks to the QtCurve devs, as they obviously should be made in lots of locations and it's their call. I myself don't see the point in doing a runtime check whether a platform is X11, though. It's known at build time if a platform is X11. Doing a runtime check before each theming action if `Q11Info::isX11Active()` (or some such call) seems to be an expensive concession to a rather improbable set-up. If distros really decide to give the user a choice between X11 and Wayland at login ... let them figure out how to streamline that first, and then add runtime checks for the active graphics backend where really needed... (In fact, I myself would avoid anything tied to the display layer as much as possible; the fact I had to go back in a few months after the previous porting effort goes to show how easy it is to break builds on other platforms with that kind of functionality - as if my own error wasn't enough already.) Martin Gräßlin wrote: HAVE_X11 is neither defined by Qt5 nor by KF5. It needs to be set manually depending on whether the source is built with or without X11 support. Concerning the runtime check: kwrite -platform wayland and your app is going to crash like hell if there is no runtime checks. René J.V. Bertin wrote: ``` neon5-exec /opt/project-neon5/bin/kwrite -platform wayland This application failed to start because it could not find or load the Qt platform plugin wayland. Available platform plugins are: linuxfb, minimal, offscreen, xcb. Reinstalling the application may fix this problem. Abort (core dumped) ``` Right, so with runtime checks it doesn't crash, it just self-destructs. Very useful difference :) Of course an application will crash if it tries to use an unavailable displaying method, or if the linker tries to load shared libraries that aren't present. In fact, with X11 it might actually exit nicely with a message about a display rather than crash. This just underlines my point: the only use for runtime checks in this context if is the same binaries are supposed to work with multiple displaying backends (or platform plugins). Whether QtCurve ought to support that is a call for its developers to make, like I said above. The only way to do that properly without (too much) overhead is to do the check at initialisation time rather than preceding each backend-specific call, i.e. use functionpointers or some OO/C++ alternative. I don't know how preferable Wayland is to X11; without that I see only an interest for people working on Wayland development under X11 for this kind of runtime switch support. To put this into context: I've often thought how it'd be nice if Qt-mac would be able to use X11, but I've always dismissed the possibility that that might be a runtime switch, exactly because it would introduce too much overhead and/or complexity for a feature that'd be used only rarely. Thomas Lübking wrote: Right, so with runtime checks it doesn't crash, it just self-destructs. You're missing the point entirely. The compile time checks have no implication on the runtime environment. Of course you cannot use the wayland platform plugin if it's not available, but you *can* compile Qt/KDE w/ X11 and wayland present - but making X11 calls when running on the wayland PP will crash the application - thus you must check whether you're operating on X11/xdg at runtime. Also testing for UNIX but not OSX to make X11 calls is plain wrong. Could be framebuffer console or wayland and no X11 installed at all. Martin Gräßlin wrote: for more information please see my blog post: http://blog.martin-graesslin.com/blog/2014/02/running-frameworks-powered-applications-on-wayland/ Btw. the QtWayland PPA will be available starting with Qt 5.4 - a version I'm already using. René J.V. Bertin wrote: @Thomas: we're not talking about compile time checks. Those evidently don't have any implication on the runtime environment (if done correctly :)). I guess my point is simply that the fact that you can (= it's possible to) compile Qt/KDE with every conceivable display/rendering
Re: Review Request 121390: make Qt5 theme build on Linux again
On Dez. 8, 2014, 2:07 nachm., Martin Gräßlin wrote: this is wrong - please have a look at various frameworks on how to do it properly. In the end it should be: #if HAVE_X11 // x11 specific stuff #endif obviously it also needs a runtime check: if (QX11Info::isPlatformX11()) as we no longer should assume that X11 is the only platform on Unix(non OSX). René J.V. Bertin wrote: I found a reference to HAVE_X11 online, but that token is not defined. Note also that the Qt5 theme is supposed to build without KF5 too, for pure Qt5 applications, so this kind of token should rather be provided by the Qt cmake files rather than KDE's. I'll leave the runtime checks to the QtCurve devs, as they obviously should be made in lots of locations and it's their call. I myself don't see the point in doing a runtime check whether a platform is X11, though. It's known at build time if a platform is X11. Doing a runtime check before each theming action if `Q11Info::isX11Active()` (or some such call) seems to be an expensive concession to a rather improbable set-up. If distros really decide to give the user a choice between X11 and Wayland at login ... let them figure out how to streamline that first, and then add runtime checks for the active graphics backend where really needed... (In fact, I myself would avoid anything tied to the display layer as much as possible; the fact I had to go back in a few months after the previous porting effort goes to show how easy it is to break builds on other platforms with that kind of functionality - as if my own error wasn't enough already.) Martin Gräßlin wrote: HAVE_X11 is neither defined by Qt5 nor by KF5. It needs to be set manually depending on whether the source is built with or without X11 support. Concerning the runtime check: kwrite -platform wayland and your app is going to crash like hell if there is no runtime checks. René J.V. Bertin wrote: ``` neon5-exec /opt/project-neon5/bin/kwrite -platform wayland This application failed to start because it could not find or load the Qt platform plugin wayland. Available platform plugins are: linuxfb, minimal, offscreen, xcb. Reinstalling the application may fix this problem. Abort (core dumped) ``` Right, so with runtime checks it doesn't crash, it just self-destructs. Very useful difference :) Of course an application will crash if it tries to use an unavailable displaying method, or if the linker tries to load shared libraries that aren't present. In fact, with X11 it might actually exit nicely with a message about a display rather than crash. This just underlines my point: the only use for runtime checks in this context if is the same binaries are supposed to work with multiple displaying backends (or platform plugins). Whether QtCurve ought to support that is a call for its developers to make, like I said above. The only way to do that properly without (too much) overhead is to do the check at initialisation time rather than preceding each backend-specific call, i.e. use functionpointers or some OO/C++ alternative. I don't know how preferable Wayland is to X11; without that I see only an interest for people working on Wayland development under X11 for this kind of runtime switch support. To put this into context: I've often thought how it'd be nice if Qt-mac would be able to use X11, but I've always dismissed the possibility that that might be a runtime switch, exactly because it would introduce too much overhead and/or complexity for a feature that'd be used only rarely. Thomas Lübking wrote: Right, so with runtime checks it doesn't crash, it just self-destructs. You're missing the point entirely. The compile time checks have no implication on the runtime environment. Of course you cannot use the wayland platform plugin if it's not available, but you *can* compile Qt/KDE w/ X11 and wayland present - but making X11 calls when running on the wayland PP will crash the application - thus you must check whether you're operating on X11/xdg at runtime. Also testing for UNIX but not OSX to make X11 calls is plain wrong. Could be framebuffer console or wayland and no X11 installed at all. Martin Gräßlin wrote: for more information please see my blog post: http://blog.martin-graesslin.com/blog/2014/02/running-frameworks-powered-applications-on-wayland/ Btw. the QtWayland PPA will be available starting with Qt 5.4 - a version I'm already using. René J.V. Bertin wrote: @Thomas: we're not talking about compile time checks. Those evidently don't have any implication on the runtime environment (if done correctly :)). I guess my point is simply that the fact that you can (= it's possible to) compile Qt/KDE with every conceivable
Re: Review Request 121263: Prevent API abuse of calling setters on temporary objects.
On Nov. 27, 2014, 7:29 a.m., Christoph Cullmann wrote: I actually would prefer no such hack in the public headers. If that is just to make porting easier, you can use that locally as a patch until the kdevplatform code is cleaned up. I still don't get why you think this is a hack, or why it would be bad to have it in public headers. Any consumer of your API could shoot yourself in the foot... - Milian --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121263/#review70996 --- On Nov. 27, 2014, 1:15 a.m., Milian Wolff wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121263/ --- (Updated Nov. 27, 2014, 1:15 a.m.) Review request for KDE Frameworks, Christoph Cullmann, Dominik Haumann, and Kevin Funk. Repository: ktexteditor Description --- In KDevelop we currently hit this often since our old class previously returned a reference for the start/end getters of range and cursor. With the help of C++11 ref qualifiers, we can detect that and let the compiler give the user an error message: error: cannot initialize object parameter of type 'KTextEditor::Cursor' with an expression of type 'KTextEditor::Cursor' documentRange().start().setColumn(42); ^~~ Yes, the error message is pretty bad. But better than nothing? We could also mark the overloads of these functions as explictily deleted, which would slightly improve the error message... Diffs - src/include/CMakeLists.txt 94b8e79e2f2b273ec344a963ba6ac81ec5a481c6 src/include/ktexteditor/cursor.h 4ebe38fc1bffb2dad02150884fd225fe3ca9e193 src/include/ktexteditor/global.h PRE-CREATION src/include/ktexteditor/range.h 1a2fc5b200c70364c3d99223e43a2ad6179055de Diff: https://git.reviewboard.kde.org/r/121263/diff/ Testing --- with the fixes to kdev's codebase, all of ktexteditor, kate and kdev* builds fine. Thanks, Milian Wolff ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: KF 5.5.0 changelog
2014-12-06 14:59 GMT+00:00 David Faure fa...@kde.org: Here's the changelog I wrote for 5.5.0. Please everyone remember to use CHANGELOG in your commits, with a standalone description of the issue (i.e. which doesn't use the modified filenames as implicit context). I tried to grab what I could from the commit logs, but I don't always understand everything in e.g. plasma-framework. In the KCoreAddons changelog KPluginMetaData::metaDataSource() should be KPluginMetaData::metaDataFileName(). I forgot to update the commit message to match the actual change. -- David Faure, fa...@kde.org, http://www.davidfaure.fr Working on KDE Frameworks 5 ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 121263: Prevent API abuse of calling setters on temporary objects.
On Nov. 27, 2014, 7:29 a.m., Christoph Cullmann wrote: I actually would prefer no such hack in the public headers. If that is just to make porting easier, you can use that locally as a patch until the kdevplatform code is cleaned up. Milian Wolff wrote: I still don't get why you think this is a hack, or why it would be bad to have it in public headers. Any consumer of your API could shoot yourself in the foot... I must rephrase: I think, without any guard define, this is not even source compatible (even if the use might be in most cases an error). And with a guard define, this is a hack, as you need to turn it on, which most people won't do at all. It might have been a good idea to add to the API in KF 5.0, but as we missed that, its now too late, or? - Christoph --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121263/#review70996 --- On Nov. 27, 2014, 1:15 a.m., Milian Wolff wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121263/ --- (Updated Nov. 27, 2014, 1:15 a.m.) Review request for KDE Frameworks, Christoph Cullmann, Dominik Haumann, and Kevin Funk. Repository: ktexteditor Description --- In KDevelop we currently hit this often since our old class previously returned a reference for the start/end getters of range and cursor. With the help of C++11 ref qualifiers, we can detect that and let the compiler give the user an error message: error: cannot initialize object parameter of type 'KTextEditor::Cursor' with an expression of type 'KTextEditor::Cursor' documentRange().start().setColumn(42); ^~~ Yes, the error message is pretty bad. But better than nothing? We could also mark the overloads of these functions as explictily deleted, which would slightly improve the error message... Diffs - src/include/CMakeLists.txt 94b8e79e2f2b273ec344a963ba6ac81ec5a481c6 src/include/ktexteditor/cursor.h 4ebe38fc1bffb2dad02150884fd225fe3ca9e193 src/include/ktexteditor/global.h PRE-CREATION src/include/ktexteditor/range.h 1a2fc5b200c70364c3d99223e43a2ad6179055de Diff: https://git.reviewboard.kde.org/r/121263/diff/ Testing --- with the fixes to kdev's codebase, all of ktexteditor, kate and kdev* builds fine. Thanks, Milian Wolff ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
OSX/CI: Anyone interested in test results of all KF5 frameworks and many KF5 apps?
Hi, I am soon about to finalise the first complete rebuild of KF5 on my OSX/CI system NOW INCLUDING ALL EXISTING TESTS ! Anyone interested in all those test results for FWs apps with test failures? Greets, Marko ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 121372: Differeniate between bookmarks for documents and the web
On Dec. 8, 2014, 8:40 p.m., Albert Astals Cid wrote: I don't think this makes sense at a framework level. If some application is so special that really needs a special case, they can call setText over the kaction themselves, otherwise we're going to end up havin 20 different Add Bookmark for all the minor technicalities of over what it is applied. I'm the person who originally filed the bug. Since I'm just translating and new to KDE, can you give me a recommendation if I should translate this as bookmarks or webmarks then? I don't know if web or document applications are more frequent in KDE, and we should minimize the number of applications that I will have to file bugs with. This is a linguistic issue rather than an application being special - English is just more ambiguous than my language in this case. - Gun --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121372/#review71585 --- On Dec. 6, 2014, 8:49 p.m., Matthew Dawson wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121372/ --- (Updated Dec. 6, 2014, 8:49 p.m.) Review request for KDE Frameworks, Localization and Translation (l10n) and Matthew Dawson. Bugs: 341279 https://bugs.kde.org/show_bug.cgi?id=341279 Repository: kconfig Description --- As requested in bug #1164383, this adds a new standard shortcut for adding bookmarks for web pages. BUG: 1164383 I'm not sure if this is the best approach. If this is accpted, I'll commit a similar change into KConfigWidgets. Diffs - src/gui/kstandardshortcut.h 5bb07fb9f060b0d5950aed251da985ce3aa46661 src/gui/kstandardshortcut.cpp 84007374c2836c1d61cb9b9361bd4217aa4ddc32 Diff: https://git.reviewboard.kde.org/r/121372/diff/ Testing --- Compiles, verified context extracted properly into pot file. Thanks, Matthew Dawson ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 121372: Differeniate between bookmarks for documents and the web
On des. 8, 2014, 8:40 p.m., Albert Astals Cid wrote: I don't think this makes sense at a framework level. If some application is so special that really needs a special case, they can call setText over the kaction themselves, otherwise we're going to end up havin 20 different Add Bookmark for all the minor technicalities of over what it is applied. Gun Chleoc wrote: I'm the person who originally filed the bug. Since I'm just translating and new to KDE, can you give me a recommendation if I should translate this as bookmarks or webmarks then? I don't know if web or document applications are more frequent in KDE, and we should minimize the number of applications that I will have to file bugs with. This is a linguistic issue rather than an application being special - English is just more ambiguous than my language in this case. You should translate bookmark. If you need bookmark in a web application to be translatable different than bookmark, then file a bug agains that application and ask them to. I am unconvinced a framework has to support all the combinatorials of application use of bookmark multiplied by the various languages that can have different translations for the various uses of bookrmak in various cases. - Albert --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121372/#review71585 --- On des. 6, 2014, 8:49 p.m., Matthew Dawson wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121372/ --- (Updated des. 6, 2014, 8:49 p.m.) Review request for KDE Frameworks, Localization and Translation (l10n) and Matthew Dawson. Bugs: 341279 https://bugs.kde.org/show_bug.cgi?id=341279 Repository: kconfig Description --- As requested in bug #1164383, this adds a new standard shortcut for adding bookmarks for web pages. BUG: 1164383 I'm not sure if this is the best approach. If this is accpted, I'll commit a similar change into KConfigWidgets. Diffs - src/gui/kstandardshortcut.h 5bb07fb9f060b0d5950aed251da985ce3aa46661 src/gui/kstandardshortcut.cpp 84007374c2836c1d61cb9b9361bd4217aa4ddc32 Diff: https://git.reviewboard.kde.org/r/121372/diff/ Testing --- Compiles, verified context extracted properly into pot file. Thanks, Matthew Dawson ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 121390: make Qt5 theme build on Linux again
On Dec. 8, 2014, 3:07 p.m., Martin Gräßlin wrote: this is wrong - please have a look at various frameworks on how to do it properly. In the end it should be: #if HAVE_X11 // x11 specific stuff #endif obviously it also needs a runtime check: if (QX11Info::isPlatformX11()) as we no longer should assume that X11 is the only platform on Unix(non OSX). René J.V. Bertin wrote: I found a reference to HAVE_X11 online, but that token is not defined. Note also that the Qt5 theme is supposed to build without KF5 too, for pure Qt5 applications, so this kind of token should rather be provided by the Qt cmake files rather than KDE's. I'll leave the runtime checks to the QtCurve devs, as they obviously should be made in lots of locations and it's their call. I myself don't see the point in doing a runtime check whether a platform is X11, though. It's known at build time if a platform is X11. Doing a runtime check before each theming action if `Q11Info::isX11Active()` (or some such call) seems to be an expensive concession to a rather improbable set-up. If distros really decide to give the user a choice between X11 and Wayland at login ... let them figure out how to streamline that first, and then add runtime checks for the active graphics backend where really needed... (In fact, I myself would avoid anything tied to the display layer as much as possible; the fact I had to go back in a few months after the previous porting effort goes to show how easy it is to break builds on other platforms with that kind of functionality - as if my own error wasn't enough already.) Martin Gräßlin wrote: HAVE_X11 is neither defined by Qt5 nor by KF5. It needs to be set manually depending on whether the source is built with or without X11 support. Concerning the runtime check: kwrite -platform wayland and your app is going to crash like hell if there is no runtime checks. René J.V. Bertin wrote: ``` neon5-exec /opt/project-neon5/bin/kwrite -platform wayland This application failed to start because it could not find or load the Qt platform plugin wayland. Available platform plugins are: linuxfb, minimal, offscreen, xcb. Reinstalling the application may fix this problem. Abort (core dumped) ``` Right, so with runtime checks it doesn't crash, it just self-destructs. Very useful difference :) Of course an application will crash if it tries to use an unavailable displaying method, or if the linker tries to load shared libraries that aren't present. In fact, with X11 it might actually exit nicely with a message about a display rather than crash. This just underlines my point: the only use for runtime checks in this context if is the same binaries are supposed to work with multiple displaying backends (or platform plugins). Whether QtCurve ought to support that is a call for its developers to make, like I said above. The only way to do that properly without (too much) overhead is to do the check at initialisation time rather than preceding each backend-specific call, i.e. use functionpointers or some OO/C++ alternative. I don't know how preferable Wayland is to X11; without that I see only an interest for people working on Wayland development under X11 for this kind of runtime switch support. To put this into context: I've often thought how it'd be nice if Qt-mac would be able to use X11, but I've always dismissed the possibility that that might be a runtime switch, exactly because it would introduce too much overhead and/or complexity for a feature that'd be used only rarely. Thomas Lübking wrote: Right, so with runtime checks it doesn't crash, it just self-destructs. You're missing the point entirely. The compile time checks have no implication on the runtime environment. Of course you cannot use the wayland platform plugin if it's not available, but you *can* compile Qt/KDE w/ X11 and wayland present - but making X11 calls when running on the wayland PP will crash the application - thus you must check whether you're operating on X11/xdg at runtime. Also testing for UNIX but not OSX to make X11 calls is plain wrong. Could be framebuffer console or wayland and no X11 installed at all. Martin Gräßlin wrote: for more information please see my blog post: http://blog.martin-graesslin.com/blog/2014/02/running-frameworks-powered-applications-on-wayland/ Btw. the QtWayland PPA will be available starting with Qt 5.4 - a version I'm already using. René J.V. Bertin wrote: @Thomas: we're not talking about compile time checks. Those evidently don't have any implication on the runtime environment (if done correctly :)). I guess my point is simply that the fact that you can (= it's possible to) compile Qt/KDE with every conceivable display/rendering
Re: Review Request 121390: make Qt5 theme build on Linux again
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121390/ --- (Updated Dec. 8, 2014, 10:59 p.m.) Review request for KDE Frameworks, Qt KDE and Yichao Yu. Changes --- The compile-time check now uses QtCurve's own `QTC_ENABLE_X11` token that is defined when building for X11. My primary concern with this ticket is to restore something I broke; if Yichao would like me to add runtime-checks he is free to ask :) Repository: qtcurve Description --- Yesterday's patches for OS X building broke the build of the Qt5 parts on Linux (and other Unix/X11 platforms). I had presumed that Q_WS_X11 would be defined in that context as it is when building Qt4 code, but apparently it isn't. This patch restores building on Unix/X11 by replacing `#ifdef Q_WS_X11` with `#if defined(Q_OS_UNIX) !defined(Q_OS_OSX)` please verify if that catches all possible contexts where X11 is to be used?! (Qt/Cygwin might use X11?) Diffs (updated) - qt5/style/blurhelper.cpp 5dcc95c qt5/style/qtcurve.cpp 7b0d7e6 qt5/style/qtcurve_plugin.cpp febc977 qt5/style/qtcurve_utils.cpp 728c26a qt5/style/shadowhelper.cpp a239cf1 qt5/style/utils.cpp 0680442 qt5/style/windowmanager.cpp 3c2bc1c Diff: https://git.reviewboard.kde.org/r/121390/diff/ Testing --- On KUbuntu 14.04 with Qt 5.3.2 and KF5 in the (sadly discontinued) Project Neon5 environment. Thanks, René J.V. Bertin ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 121356: Do not change the volume when playing a notification
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121356/ --- (Updated des. 8, 2014, 10:39 p.m.) Status -- This change has been discarded. Review request for KDE Frameworks and David Edmundson. Repository: knotifications Description --- Playing a notification should be that, playing a notification. Inspired by David's comment on https://git.reviewboard.kde.org/r/121278/ Diffs - src/notifybysound.cpp 09a80dd Diff: https://git.reviewboard.kde.org/r/121356/diff/ Testing --- Thanks, Albert Astals Cid ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Review Request 121397: Remove NotifyBySound
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121397/ --- Review request for KDE Frameworks, David Edmundson and Martin Klapetek. Repository: knotifications Description --- NotifyByAudio implements the Sound notification already Diffs - src/knotificationmanager.cpp 9f180b4 src/notifybysound.h 1efa620 src/notifybysound.cpp 09a80dd Diff: https://git.reviewboard.kde.org/r/121397/diff/ Testing --- Thanks, Albert Astals Cid ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 121397: Remove NotifyBySound
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121397/ --- (Updated des. 8, 2014, 10:42 p.m.) Review request for KDE Frameworks, David Edmundson and Martin Klapetek. Repository: knotifications Description (updated) --- NotifyByAudio implements the Sound notification already Based on comments from https://git.reviewboard.kde.org/r/121356/ Diffs - src/knotificationmanager.cpp 9f180b4 src/notifybysound.h 1efa620 src/notifybysound.cpp 09a80dd Diff: https://git.reviewboard.kde.org/r/121397/diff/ Testing --- Thanks, Albert Astals Cid ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 121397: Remove NotifyBySound
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121397/#review71596 --- Ship it! src/knotificationmanager.cpp https://git.reviewboard.kde.org/r/121397/#comment49924 Can you also remove this line please, makes no sense now - Martin Klapetek On Dec. 8, 2014, 11:42 p.m., Albert Astals Cid wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121397/ --- (Updated Dec. 8, 2014, 11:42 p.m.) Review request for KDE Frameworks, David Edmundson and Martin Klapetek. Repository: knotifications Description --- NotifyByAudio implements the Sound notification already Based on comments from https://git.reviewboard.kde.org/r/121356/ Diffs - src/knotificationmanager.cpp 9f180b4 src/notifybysound.h 1efa620 src/notifybysound.cpp 09a80dd Diff: https://git.reviewboard.kde.org/r/121397/diff/ Testing --- Thanks, Albert Astals Cid ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 121397: Remove NotifyBySound
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121397/ --- (Updated Dec. 8, 2014, 10:52 p.m.) Status -- This change has been marked as submitted. Review request for KDE Frameworks, David Edmundson and Martin Klapetek. Repository: knotifications Description --- NotifyByAudio implements the Sound notification already Based on comments from https://git.reviewboard.kde.org/r/121356/ Diffs - src/knotificationmanager.cpp 9f180b4 src/notifybysound.h 1efa620 src/notifybysound.cpp 09a80dd Diff: https://git.reviewboard.kde.org/r/121397/diff/ Testing --- Thanks, Albert Astals Cid ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 121094: KCoreAddons: add KTextToHTML class for plaintext - HTML conversion
https://git.reviewboard.kde.org/r/121094/ This change fails to compile on Windows, because the unit test has ktexttohtml.cpp as a source file *and* it's linking to the kcoreaddons library which obviously defines the same function. In addition, ktexttohtml.h in this context is declaring the function as __declspec(dllimport), so it expects them to find the function in a linked library. All this causes a multiple definition error when linking the unit test. I see it's being done this way in order to test the private API, but I'm not sure how to fix it. Is anyone else testing non-exported API in KF5? -- Nicolás ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 121094: KCoreAddons: add KTextToHTML class for plaintext - HTML conversion
2014-12-08 23:37 GMT-03:00 Nicolás Alvarez nicolas.alva...@gmail.com: https://git.reviewboard.kde.org/r/121094/ This change fails to compile on Windows, because the unit test has ktexttohtml.cpp as a source file *and* it's linking to the kcoreaddons library which obviously defines the same function. In addition, ktexttohtml.h in this context is declaring the function as __declspec(dllimport), so it expects them to find the function in a linked library. All this causes a multiple definition error when linking the unit test. I see it's being done this way in order to test the private API, but I'm not sure how to fix it. Is anyone else testing non-exported API in KF5? I have fixed this build error (d5aa34a3), based on how knewstuff does it :) -- Nicolás ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 121372: Differeniate between bookmarks for documents and the web
On Dec. 8, 2014, 3:40 p.m., Albert Astals Cid wrote: I don't think this makes sense at a framework level. If some application is so special that really needs a special case, they can call setText over the kaction themselves, otherwise we're going to end up havin 20 different Add Bookmark for all the minor technicalities of over what it is applied. Gun Chleoc wrote: I'm the person who originally filed the bug. Since I'm just translating and new to KDE, can you give me a recommendation if I should translate this as bookmarks or webmarks then? I don't know if web or document applications are more frequent in KDE, and we should minimize the number of applications that I will have to file bugs with. This is a linguistic issue rather than an application being special - English is just more ambiguous than my language in this case. Albert Astals Cid wrote: You should translate bookmark. If you need bookmark in a web application to be translatable different than bookmark, then file a bug agains that application and ask them to. I am unconvinced a framework has to support all the combinatorials of application use of bookmark multiplied by the various languages that can have different translations for the various uses of bookrmak in various cases. Based on these comments then, I'm going to discard this RR and close the bug as WONTFIX with a pointer back to here. Thanks for the help! - Matthew --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121372/#review71585 --- On Dec. 6, 2014, 3:49 p.m., Matthew Dawson wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121372/ --- (Updated Dec. 6, 2014, 3:49 p.m.) Review request for KDE Frameworks, Localization and Translation (l10n) and Matthew Dawson. Bugs: 341279 https://bugs.kde.org/show_bug.cgi?id=341279 Repository: kconfig Description --- As requested in bug #1164383, this adds a new standard shortcut for adding bookmarks for web pages. BUG: 1164383 I'm not sure if this is the best approach. If this is accpted, I'll commit a similar change into KConfigWidgets. Diffs - src/gui/kstandardshortcut.h 5bb07fb9f060b0d5950aed251da985ce3aa46661 src/gui/kstandardshortcut.cpp 84007374c2836c1d61cb9b9361bd4217aa4ddc32 Diff: https://git.reviewboard.kde.org/r/121372/diff/ Testing --- Compiles, verified context extracted properly into pot file. Thanks, Matthew Dawson ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Review Request 121401: Add Windows VERSIONINFO to KF5CoreAddons.dll
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121401/ --- Review request for KDE Frameworks. Repository: kcoreaddons Description --- Add a Windows VERSIONINFO resource to KF5CoreAddons.dll, using the new macro in ECM (see /r/121400). Diffs - src/lib/CMakeLists.txt b2b46b1 Diff: https://git.reviewboard.kde.org/r/121401/diff/ Testing --- Tested with CMake 3.0.2, MSVC2013, Windows 7. Explorer tooltip shows description and version. File properties shows all the new information. Thanks, Nicolás Alvarez ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Review Request 121400: Add a new macro to create VERSIONINFO resources for Windows
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121400/ --- Review request for Build System and KDE Frameworks. Repository: extra-cmake-modules Description --- Windows has a way for executables and shared libraries (DLLs) to contain version information in a machine-readable format. This information can be seen in Explorer in the file properties (http://goo.gl/UbJGte) and also in the tooltip when hovering the file. It's also used by Windows Installer to avoid overwriting an existing DLL with an older version. This change adds a CMake macro to generate a Windows resource script with version information passed to the macro. The macro documentation is a single line, obviously needs work. Diffs - modules/ECMCreateVersionResource.cmake PRE-CREATION modules/ECMVersionResource.rc.in PRE-CREATION Diff: https://git.reviewboard.kde.org/r/121400/diff/ Testing --- Tested with CMake 3.0.2 in kcoreaddons (review /r/121401), MSVC2013, Windows 7. Explorer tooltip shows description and version. File properties shows all the new information. Thanks, Nicolás Alvarez ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
