Re: Review Request 122981: add KGlobalAccel::loadShortcutFromGlobalSettings
On mar. 22, 2015, 3:56 p.m., Albert Astals Cid wrote: src/kglobalaccel.h, line 260 https://git.reviewboard.kde.org/r/122981/diff/2/?file=356138#file356138line260 typo in following Gregor Mi wrote: About why it is needed: see KGlobalShortcutTest::testLoadShortcutFromGlobalSettings: shortcut() does not behave as expected when the new method loadShortcutFromGlobalSettings() is not called I don't know if always calling loadShortcutFromGlobalSettings() before shortcut() would break existing code. E.g. in kwin/effects/desktopgrid/desktopgrid_config.cpp shortcut() is used without prior call of loadShortcutFromGlobalSettings(). On the other hand, a call to loadShortcutFromGlobalSettings() might be harmless but I don't have a good overview of the KGlobalAccel usage patterns. Albert Astals Cid wrote: Honestly i have no idea on how this kglobalaccel thing works, but it seems to me you're just adding a way to workaround a bug instead of fixing it. Gregor Mi wrote: Me neither :-) I was hoping that someone with more experience with kglobalaccel can say more about the proposed solution. Like you Thomas suggested to put the loading method into shortcut() (which is essentially a one-liner: d-updateGlobalShortcut(action, KGlobalAccelPrivate::ActiveShortcut, KGlobalAccel::Autoloading);). Martin Gräßlin wrote: well we could try it: add the one-line change and try whether the unit tests pass and then try whether the case in DesktopGridConfig works. Unfortunately I'm also not that into KGloalAccel code yet to be 100 % whether it's a valid change :-( Gregor Mi wrote: I tried to call loadShortcutFromGlobalSettings() from within shortcut() and found that this violates const constraints: shortcut() is const d-updateGlobalShortcut is not const So this is not possible. How to proceed now? make mutable the members that updateGlobalShortcut modifies - Albert --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122981/#review77912 --- On abr. 2, 2015, 2:56 p.m., Gregor Mi wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122981/ --- (Updated abr. 2, 2015, 2:56 p.m.) Review request for KDE Frameworks, Martin Gräßlin and Thomas Lübking. Repository: kglobalaccel Description --- In some cases you need to call loadShortcutFromGlobalSettings() in order not to get a an empty list when calling shortcut() (which is const). See discussion in https://git.reviewboard.kde.org/r/122249/ (libksysguard: add Kill Window to End Process button and show correct keyboard shortcut). Diffs - autotests/kglobalshortcuttest.h 2a7b40d34875e16345a659fb9764e4f536ad79c6 autotests/kglobalshortcuttest.cpp 169bf92ffbff985cd4381e771c2fcecaff77156b src/kglobalaccel.h 3fe20ca8e4ec6ceb0bb9e54235aef7f1aeeb8c16 src/kglobalaccel.cpp 1b6b3f5cb6d42401d684e6a491d12a6e57248fd1 Diff: https://git.reviewboard.kde.org/r/122981/diff/ Testing --- Thanks, Gregor Mi ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Build failed in Jenkins: kinfocenter_master_qt5 #91
See http://build.kde.org/job/kinfocenter_master_qt5/91/changes Changes: [lueck] update to kf5 -- [...truncated 114 lines...] == Configuring Build -- The C compiler identification is GNU 4.8.2 -- The CXX compiler identification is GNU 4.8.2 -- Check for working C compiler: /home/jenkins/bin/cc -- Check for working C compiler: /home/jenkins/bin/cc -- works -- Detecting C compiler ABI info -- Detecting C compiler ABI info - done -- Detecting C compile features -- Detecting C compile features - done -- Check for working CXX compiler: /home/jenkins/bin/c++ -- Check for working CXX compiler: /home/jenkins/bin/c++ -- works -- Detecting CXX compiler ABI info -- Detecting CXX compiler ABI info - done -- Detecting CXX compile features -- Detecting CXX compile features - done -- Looking for __GLIBC__ -- Looking for __GLIBC__ - found -- Performing Test _OFFT_IS_64BIT -- Performing Test _OFFT_IS_64BIT - Success -- Found KF5Completion: /srv/jenkins/install/linux/x86_64/g++/kf5-qt5/frameworks/kcompletion/inst/lib64/cmake/KF5Completion/KF5CompletionConfig.cmake (found version 5.8.0) -- Found KF5Config: /srv/jenkins/install/linux/x86_64/g++/kf5-qt5/frameworks/kconfig/inst/lib64/cmake/KF5Config/KF5ConfigConfig.cmake (found version 5.8.0) -- Found KF5ConfigWidgets: /srv/jenkins/install/linux/x86_64/g++/kf5-qt5/frameworks/kconfigwidgets/inst/lib64/cmake/KF5ConfigWidgets/KF5ConfigWidgetsConfig.cmake (found version 5.8.0) -- Found KF5CoreAddons: /srv/jenkins/install/linux/x86_64/g++/kf5-qt5/frameworks/kcoreaddons/inst/lib64/cmake/KF5CoreAddons/KF5CoreAddonsConfig.cmake (found version 5.8.0) -- Found KF5DBusAddons: /srv/jenkins/install/linux/x86_64/g++/kf5-qt5/frameworks/kdbusaddons/inst/lib64/cmake/KF5DBusAddons/KF5DBusAddonsConfig.cmake (found version 5.8.0) -- Found KF5DocTools: /srv/jenkins/install/linux/x86_64/g++/kf5-qt5/frameworks/kdoctools/inst/lib64/cmake/KF5DocTools/KF5DocToolsConfig.cmake (found version 5.8.0) -- Found Gettext: /usr/bin/msgmerge (found version 0.18.1) -- Found PythonInterp: /usr/bin/python (found version 2.7.3) -- Found KF5I18n: /srv/jenkins/install/linux/x86_64/g++/kf5-qt5/frameworks/ki18n/inst/lib64/cmake/KF5I18n/KF5I18nConfig.cmake (found version 5.8.0) -- Found KF5IconThemes: /srv/jenkins/install/linux/x86_64/g++/kf5-qt5/frameworks/kiconthemes/inst/lib64/cmake/KF5IconThemes/KF5IconThemesConfig.cmake (found version 5.8.0) -- Found KF5KCMUtils: /srv/jenkins/install/linux/x86_64/g++/kf5-qt5/frameworks/kcmutils/inst/lib64/cmake/KF5KCMUtils/KF5KCMUtilsConfig.cmake (found version 5.8.0) -- Found KF5KDELibs4Support: /srv/jenkins/install/linux/x86_64/g++/kf5-qt5/frameworks/kdelibs4support/inst/lib64/cmake/KF5KDELibs4Support/KF5KDELibs4SupportConfig.cmake (found version 5.8.0) -- Found KF5KIO: /srv/jenkins/install/linux/x86_64/g++/kf5-qt5/frameworks/kio/inst/lib64/cmake/KF5KIO/KF5KIOConfig.cmake (found version 5.8.0) -- Found KF5Service: /srv/jenkins/install/linux/x86_64/g++/kf5-qt5/frameworks/kservice/inst/lib64/cmake/KF5Service/KF5ServiceConfig.cmake (found version 5.8.0) -- Found KF5Solid: /srv/jenkins/install/linux/x86_64/g++/kf5-qt5/frameworks/solid/inst/lib64/cmake/KF5Solid/KF5SolidConfig.cmake (found version 5.8.0) -- Found KF5WidgetsAddons: /srv/jenkins/install/linux/x86_64/g++/kf5-qt5/frameworks/kwidgetsaddons/inst/lib64/cmake/KF5WidgetsAddons/KF5WidgetsAddonsConfig.cmake (found version 5.8.0) -- Found KF5XmlGui: /srv/jenkins/install/linux/x86_64/g++/kf5-qt5/frameworks/kxmlgui/inst/lib64/cmake/KF5XmlGui/KF5XmlGuiConfig.cmake (found version 5.8.0) -- Found KF5: success (found version 5.8.0) found components: Completion Config ConfigWidgets CoreAddons DBusAddons DocTools I18n IconThemes KCMUtils KDELibs4Support KIO Service Solid WidgetsAddons XmlGui -- Found OpenGL: /usr/lib64/libGL.so -- Found PkgConfig: /usr/bin/pkg-config (found version 0.27.1) -- Found EGL: /usr/lib64/libEGL.so (found version 1.4) -- Looking for XOpenDisplay in /usr/lib64/libX11.so;/usr/lib64/libXext.so -- Looking for XOpenDisplay in /usr/lib64/libX11.so;/usr/lib64/libXext.so - found -- Looking for gethostbyname -- Looking for gethostbyname - found -- Looking for connect -- Looking for connect - found -- Looking for remove -- Looking for remove - found -- Looking for shmat -- Looking for shmat - found -- Looking for IceConnectionNumber in ICE -- Looking for IceConnectionNumber in ICE - found -- Found X11: /usr/lib64/libX11.so -- Looking for include file devinfo.h -- Looking for include file devinfo.h - not found -- Looking for include file sys/sockio.h -- Looking for include file sys/sockio.h - not found -- Looking for getnameinfo -- Looking for getnameinfo - found -- Performing Test HAVE_STRUCT_SOCKADDR_SA_LEN -- Performing Test HAVE_STRUCT_SOCKADDR_SA_LEN - Failed -- Looking for getifaddrs -- Looking for getifaddrs - found -- Found PCIUTILS: /usr/lib64/libpci.so;/usr/lib64/libresolv.so -- Enabling PCI module based on
Re: Review Request 120393: [kdelibs4support] Kill dead code
On March 18, 2015, 11:23 p.m., Vishesh Handa wrote: I'm all for getting rid of the Nepomuk code. However, I'm not too sure about the strigi part. That should still work. Hrvoje Senjan wrote: It does not ;-) Originally, this review added back the find_package(Strigi) call which was removed a while back (at least before 5.0.0), so this code was/is never compiled. Vishesh Handa wrote: I still cannot give it a ship it. We need to collectively decide if we want to let the Strigi integration be broken and remove the code. Or add the dependency again and see why it doesn't work. Albert Astals Cid wrote: Agreeing with Vishesh here, can you send a new email to kde-core-devel mailing list mentioning what should we do, if stop supporting strigi or not in kdelibs4support? This way we can get a more project wide discussion about it. Hrvoje Senjan wrote: Maybe there was a misunderstanding - i do not know do strigi related code works if compiled - problem was/is they where never compiled since, and before 5.0.0. Anyway, i'll compose a k-c-d mail laters... Ping. If you can split up just the Nepomuk parts, it's a ship it from me. - Vishesh --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120393/#review77714 --- On March 18, 2015, 6:24 p.m., Hrvoje Senjan wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120393/ --- (Updated March 18, 2015, 6:24 p.m.) Review request for KDE Frameworks, David Faure and Vishesh Handa. Repository: kdelibs4support Description --- Strigi check has been removed in commit c8f4c69650c71276b2a2263212addde63764e58b, and soprano wasn't even ported to Qt5 (afaik), so this was never compiled. Diffs - autotests/kfilemetainfotest.cpp c751cdd src/CMakeLists.txt b662893 src/config-kdelibs4support.h.cmake 1af3ee0 src/kio/kfilemetadataconfigurationwidget.cpp 259b205 src/kio/kfilemetadataprovider.cpp 3468546 src/kio/kfilemetadataprovider_p.h 31137b2 src/kio/kfilemetadatawidget.cpp 1edb069 src/kio/kfilemetainfo.cpp eae1295 src/kio/kfilemetainfoitem.cpp 62f760d src/kio/kfilemetainfoitem_p.h 8929e46 src/kio/knfotranslator.cpp 8eec6a1 Diff: https://git.reviewboard.kde.org/r/120393/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 122981: add KGlobalAccel::loadShortcutFromGlobalSettings
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122981/ --- (Updated April 3, 2015, 5:50 p.m.) Review request for KDE Frameworks, Martin Gräßlin and Thomas Lübking. Changes --- - Introduce 'const' at the right places to make a call of d-updateGlobalShortcut() in shortcut() possible. - But the call is commented out because more unit test fail Repository: kglobalaccel Description --- In some cases you need to call loadShortcutFromGlobalSettings() in order not to get a an empty list when calling shortcut() (which is const). See discussion in https://git.reviewboard.kde.org/r/122249/ (libksysguard: add Kill Window to End Process button and show correct keyboard shortcut). Diffs (updated) - autotests/kglobalshortcuttest.h b1122a8f5ca2f3f7afbe78f8edba87325426c1a6 autotests/kglobalshortcuttest.cpp 3b661bbb612807a3bbbe34835d4ae712c2ec74da src/kglobalaccel.h 3fe20ca8e4ec6ceb0bb9e54235aef7f1aeeb8c16 src/kglobalaccel.cpp 1b6b3f5cb6d42401d684e6a491d12a6e57248fd1 src/kglobalaccel_p.h eca7c52378ad60d0d5806561214b9788dd46a11e Diff: https://git.reviewboard.kde.org/r/122981/diff/ Testing --- Thanks, Gregor Mi ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 120648: Encode the URIs which end up in DTD files
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120648/#review78445
---
cmake/uriencode.cmake (line 15)
https://git.reviewboard.kde.org/r/120648/#comment53670
I've tried changing this to:
execute_process(COMMAND perl -MURI::Escape -e print
uri_escape_utf8(\${escaped_uri}\, \^A-Za-z0-9\\-\\._~\\/:\);
(...)
it works here, but I'm not sure if it'd be the proper fix.
- Andrius da Costa Ribas
On Fev. 28, 2015, 10:02 p.m., Luigi Toscano wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120648/
---
(Updated Fev. 28, 2015, 10:02 p.m.)
Review request for Build System, KDE Software on Mac OS X, KDE Frameworks,
and kdewin.
Repository: kdoctools
Description
---
The URI need to be encoded, because some valid characters for
filenames are not valid according RFC 2396.
Easy way to trigger the issue: when the path contains spaces,
as it happens on MacOSX builds.
See also https://git.reviewboard.kde.org/r/120649/ for the twin review on
kdelibs4support.
Diffs
-
cmake/uriencode.cmake PRE-CREATION
src/CMakeLists.txt 341ecf4
Diff: https://git.reviewboard.kde.org/r/120648/diff/
Testing
---
It compiles, but I can't properly test Mac and Windows scenarios
Thanks,
Luigi Toscano
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 122981: add KGlobalAccel::loadShortcutFromGlobalSettings
On March 22, 2015, 3:56 p.m., Albert Astals Cid wrote: src/kglobalaccel.h, line 260 https://git.reviewboard.kde.org/r/122981/diff/2/?file=356138#file356138line260 typo in following Gregor Mi wrote: About why it is needed: see KGlobalShortcutTest::testLoadShortcutFromGlobalSettings: shortcut() does not behave as expected when the new method loadShortcutFromGlobalSettings() is not called I don't know if always calling loadShortcutFromGlobalSettings() before shortcut() would break existing code. E.g. in kwin/effects/desktopgrid/desktopgrid_config.cpp shortcut() is used without prior call of loadShortcutFromGlobalSettings(). On the other hand, a call to loadShortcutFromGlobalSettings() might be harmless but I don't have a good overview of the KGlobalAccel usage patterns. Albert Astals Cid wrote: Honestly i have no idea on how this kglobalaccel thing works, but it seems to me you're just adding a way to workaround a bug instead of fixing it. Gregor Mi wrote: Me neither :-) I was hoping that someone with more experience with kglobalaccel can say more about the proposed solution. Like you Thomas suggested to put the loading method into shortcut() (which is essentially a one-liner: d-updateGlobalShortcut(action, KGlobalAccelPrivate::ActiveShortcut, KGlobalAccel::Autoloading);). Martin Gräßlin wrote: well we could try it: add the one-line change and try whether the unit tests pass and then try whether the case in DesktopGridConfig works. Unfortunately I'm also not that into KGloalAccel code yet to be 100 % whether it's a valid change :-( Gregor Mi wrote: I tried to call loadShortcutFromGlobalSettings() from within shortcut() and found that this violates const constraints: shortcut() is const d-updateGlobalShortcut is not const So this is not possible. How to proceed now? Albert Astals Cid wrote: make mutable the members that updateGlobalShortcut modifies I tried again and the call was even possible with only introducing some more consts. But the call of loadShortcutFromGlobalSettings() from within shortcut() makes more unit tests fail than without. = the general call of loadShortcutFromGlobalSettings() from within shortcut() seems to harm more than it helps. Currently, I have a problem with getting the unit tests to all pass (which is independet of this change; it happens also in master branch). See my analysis below and comment of the unit test itself: /* These tests could be better. They don't include actually triggering actions, and we just choose very improbable shortcuts to avoid conflicts with real applications' shortcuts. */ It would be nice to document some instructions of how to reset the global shortcuts to a defined state. - Gregor --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122981/#review77912 --- On April 3, 2015, 5:50 p.m., Gregor Mi wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122981/ --- (Updated April 3, 2015, 5:50 p.m.) Review request for KDE Frameworks, Martin Gräßlin and Thomas Lübking. Repository: kglobalaccel Description --- In some cases you need to call loadShortcutFromGlobalSettings() in order not to get a an empty list when calling shortcut() (which is const). See discussion in https://git.reviewboard.kde.org/r/122249/ (libksysguard: add Kill Window to End Process button and show correct keyboard shortcut). Diffs - autotests/kglobalshortcuttest.h b1122a8f5ca2f3f7afbe78f8edba87325426c1a6 autotests/kglobalshortcuttest.cpp 3b661bbb612807a3bbbe34835d4ae712c2ec74da src/kglobalaccel.h 3fe20ca8e4ec6ceb0bb9e54235aef7f1aeeb8c16 src/kglobalaccel.cpp 1b6b3f5cb6d42401d684e6a491d12a6e57248fd1 src/kglobalaccel_p.h eca7c52378ad60d0d5806561214b9788dd46a11e Diff: https://git.reviewboard.kde.org/r/122981/diff/ Testing --- Thanks, Gregor Mi ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 120648: Encode the URIs which end up in DTD files
On Abril 3, 2015, 3:35 p.m., Andrius da Costa Ribas wrote:
src/CMakeLists.txt, line 20
https://git.reviewboard.kde.org/r/120648/diff/2/?file=320785#file320785line20
It's already committed (but there's been a long time since I hadn't
build anything), but now I see that DocBookXML4_DTD_DIR is used only inside
the else(NOT WIN32) part, is this what's been intended? Also, this
doesn't work when the original DocBookXML4_DTD_DIR is an absolute Windows
path V: becomes V%3A and xmllint fails to recognise it as an absolute
path (see below) and CMake would fail to install to such location. (maybe
we should prepend it with file:// ?)
file:///V:/build/frameworks/kdoctools/work/msvc2013-RelWithDebInfo-master/src/customization/dtd/kdedbx45.dtd:102:
warning: failed to load external
///V:/build/frameworks/kdoctools/work/msvc2013-RelWithDebInfo-master/src/customization/dtd/v%3A/share/xml/docbook/schema/dtd/4.5/docbookx.dtd
%DocBookDTD;
Luigi Toscano wrote:
- DocBookXML4_DTD_DIR is properly touched both for non WIN32 (line 4) and
WIN32 (lines 4 and 20). The WIN32 logic is a bit complicated, but basically
it rewrites again the generated ${_custom_dtd_kdex} file using the DTD path
which is knows during the installation. Or something like that.
- Oh, that's the feedback I was looking for. Can you please copy the
exact content of the path to docbookx.dtd as referenced into kdedbx45.dtd?
!ENTITY % DocBookDTD PUBLIC
-//OASIS//DTD DocBook XML V4.5//EN
v%3A/share/xml/docbook/schema/dtd/4.5/docbookx.dtd
%DocBookDTD;
- Andrius da Costa
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120648/#review78442
---
On Fev. 28, 2015, 10:02 p.m., Luigi Toscano wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120648/
---
(Updated Fev. 28, 2015, 10:02 p.m.)
Review request for Build System, KDE Software on Mac OS X, KDE Frameworks,
and kdewin.
Repository: kdoctools
Description
---
The URI need to be encoded, because some valid characters for
filenames are not valid according RFC 2396.
Easy way to trigger the issue: when the path contains spaces,
as it happens on MacOSX builds.
See also https://git.reviewboard.kde.org/r/120649/ for the twin review on
kdelibs4support.
Diffs
-
cmake/uriencode.cmake PRE-CREATION
src/CMakeLists.txt 341ecf4
Diff: https://git.reviewboard.kde.org/r/120648/diff/
Testing
---
It compiles, but I can't properly test Mac and Windows scenarios
Thanks,
Luigi Toscano
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 123223: Fix bug 344614: kservice splits filename wrong
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123223/ --- (Updated April 3, 2015, 5:35 p.m.) Review request for KDE Frameworks. Changes --- use of QFileInfo(entryPath).completeBaseName() simplifies code Summary (updated) - Fix bug 344614: kservice splits filename wrong Repository: kservice Description --- Patch for https://bugs.kde.org/show_bug.cgi?id=344614. The difference between the old and new code (which is moved to a separated method in the .h file) is: name.indexOf is replaced by name.lastIndexOf There is one issue: I would like to move the implementation of the new method KServicePrivate::entryPathToName from the .h to .cpp file but I get at link time: .../autotests/kservicetest.cpp:824: undefined reference to 'KServicePrivate::entryPathToName(QString const)' Is it generally possible to unit test methods delcared *Private classes? Diffs (updated) - autotests/kservicetest.h 929cd9fefe22308909e44650e8e4bfb2456fd534 autotests/kservicetest.cpp d46f868185c3bf45138d80d04f4eb0d2840de9ca src/services/kservice.cpp 8c4e1a180a7628872d6f62486db9aee4b858547f src/services/kservice_p.h 9a5a31caadc7487f8daf898eff062b2855800313 Diff: https://git.reviewboard.kde.org/r/123223/diff/ Testing --- 1. new unit test succeeds 2. all other unit tests in ./kservicetest sill succeed 3. Call `kbuildsycoca5 --noincremental` without and with the patch and try out dolphin's Space Info menu. In the first case filelight is not found. With the patch filelight is found. Thanks, Gregor Mi ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 123223: [Preliminary] Fix bug 344614: kservice splits filename wrong
On April 2, 2015, 10:04 p.m., Dominik Haumann wrote: Good catch, maybe we can use QFileInfo::completeBaseName() instead? Good idea. This works and makes the code easier to understand. - Gregor --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123223/#review78424 --- On April 2, 2015, 11:55 a.m., Gregor Mi wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123223/ --- (Updated April 2, 2015, 11:55 a.m.) Review request for KDE Frameworks. Repository: kservice Description --- Patch for https://bugs.kde.org/show_bug.cgi?id=344614. The difference between the old and new code (which is moved to a separated method in the .h file) is: name.indexOf is replaced by name.lastIndexOf There is one issue: I would like to move the implementation of the new method KServicePrivate::entryPathToName from the .h to .cpp file but I get at link time: .../autotests/kservicetest.cpp:824: undefined reference to 'KServicePrivate::entryPathToName(QString const)' Is it generally possible to unit test methods delcared *Private classes? Diffs - autotests/kservicetest.h 929cd9fefe22308909e44650e8e4bfb2456fd534 autotests/kservicetest.cpp d46f868185c3bf45138d80d04f4eb0d2840de9ca src/services/kservice.cpp 8c4e1a180a7628872d6f62486db9aee4b858547f src/services/kservice_p.h 9a5a31caadc7487f8daf898eff062b2855800313 Diff: https://git.reviewboard.kde.org/r/123223/diff/ Testing --- 1. new unit test succeeds 2. all other unit tests in ./kservicetest sill succeed 3. Call `kbuildsycoca5 --noincremental` without and with the patch and try out dolphin's Space Info menu. In the first case filelight is not found. With the patch filelight is found. Thanks, Gregor Mi ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 122981: add KGlobalAccel::loadShortcutFromGlobalSettings
On mar. 22, 2015, 3:56 p.m., Albert Astals Cid wrote: src/kglobalaccel.h, line 260 https://git.reviewboard.kde.org/r/122981/diff/2/?file=356138#file356138line260 typo in following Gregor Mi wrote: About why it is needed: see KGlobalShortcutTest::testLoadShortcutFromGlobalSettings: shortcut() does not behave as expected when the new method loadShortcutFromGlobalSettings() is not called I don't know if always calling loadShortcutFromGlobalSettings() before shortcut() would break existing code. E.g. in kwin/effects/desktopgrid/desktopgrid_config.cpp shortcut() is used without prior call of loadShortcutFromGlobalSettings(). On the other hand, a call to loadShortcutFromGlobalSettings() might be harmless but I don't have a good overview of the KGlobalAccel usage patterns. Albert Astals Cid wrote: Honestly i have no idea on how this kglobalaccel thing works, but it seems to me you're just adding a way to workaround a bug instead of fixing it. Gregor Mi wrote: Me neither :-) I was hoping that someone with more experience with kglobalaccel can say more about the proposed solution. Like you Thomas suggested to put the loading method into shortcut() (which is essentially a one-liner: d-updateGlobalShortcut(action, KGlobalAccelPrivate::ActiveShortcut, KGlobalAccel::Autoloading);). Martin Gräßlin wrote: well we could try it: add the one-line change and try whether the unit tests pass and then try whether the case in DesktopGridConfig works. Unfortunately I'm also not that into KGloalAccel code yet to be 100 % whether it's a valid change :-( Gregor Mi wrote: I tried to call loadShortcutFromGlobalSettings() from within shortcut() and found that this violates const constraints: shortcut() is const d-updateGlobalShortcut is not const So this is not possible. How to proceed now? Albert Astals Cid wrote: make mutable the members that updateGlobalShortcut modifies Gregor Mi wrote: I tried again and the call was even possible with only introducing some more consts. But the call of loadShortcutFromGlobalSettings() from within shortcut() makes more unit tests fail than without. = the general call of loadShortcutFromGlobalSettings() from within shortcut() seems to harm more than it helps. Currently, I have a problem with getting the unit tests to all pass (which is independet of this change; it happens also in master branch). See my analysis below and comment of the unit test itself: /* These tests could be better. They don't include actually triggering actions, and we just choose very improbable shortcuts to avoid conflicts with real applications' shortcuts. */ It would be nice to document some instructions of how to reset the global shortcuts to a defined state. Use strace to see which files are being read. - Albert --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122981/#review77912 --- On abr. 3, 2015, 5:50 p.m., Gregor Mi wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122981/ --- (Updated abr. 3, 2015, 5:50 p.m.) Review request for KDE Frameworks, Martin Gräßlin and Thomas Lübking. Repository: kglobalaccel Description --- In some cases you need to call loadShortcutFromGlobalSettings() in order not to get a an empty list when calling shortcut() (which is const). See discussion in https://git.reviewboard.kde.org/r/122249/ (libksysguard: add Kill Window to End Process button and show correct keyboard shortcut). Diffs - autotests/kglobalshortcuttest.h b1122a8f5ca2f3f7afbe78f8edba87325426c1a6 autotests/kglobalshortcuttest.cpp 3b661bbb612807a3bbbe34835d4ae712c2ec74da src/kglobalaccel.h 3fe20ca8e4ec6ceb0bb9e54235aef7f1aeeb8c16 src/kglobalaccel.cpp 1b6b3f5cb6d42401d684e6a491d12a6e57248fd1 src/kglobalaccel_p.h eca7c52378ad60d0d5806561214b9788dd46a11e Diff: https://git.reviewboard.kde.org/r/122981/diff/ Testing --- Thanks, Gregor Mi ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Jenkins build is back to normal : kinfocenter_master_qt5 #92
See http://build.kde.org/job/kinfocenter_master_qt5/92/changes ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 120648: Encode the URIs which end up in DTD files
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120648/#review78442 --- src/CMakeLists.txt (line 20) https://git.reviewboard.kde.org/r/120648/#comment53667 It's already committed (but there's been a long time since I hadn't build anything), but now I see that DocBookXML4_DTD_DIR is used only inside the else(NOT WIN32) part, is this what's been intended? Also, this doesn't work when the original DocBookXML4_DTD_DIR is an absolute Windows path V: becomes V%3A and xmllint fails to recognise it as an absolute path (see below) and CMake would fail to install to such location. (maybe we should prepend it with file:// ?) file:///V:/build/frameworks/kdoctools/work/msvc2013-RelWithDebInfo-master/src/customization/dtd/kdedbx45.dtd:102: warning: failed to load external ///V:/build/frameworks/kdoctools/work/msvc2013-RelWithDebInfo-master/src/customization/dtd/v%3A/share/xml/docbook/schema/dtd/4.5/docbookx.dtd %DocBookDTD; - Andrius da Costa Ribas On Fev. 28, 2015, 10:02 p.m., Luigi Toscano wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120648/ --- (Updated Fev. 28, 2015, 10:02 p.m.) Review request for Build System, KDE Software on Mac OS X, KDE Frameworks, and kdewin. Repository: kdoctools Description --- The URI need to be encoded, because some valid characters for filenames are not valid according RFC 2396. Easy way to trigger the issue: when the path contains spaces, as it happens on MacOSX builds. See also https://git.reviewboard.kde.org/r/120649/ for the twin review on kdelibs4support. Diffs - cmake/uriencode.cmake PRE-CREATION src/CMakeLists.txt 341ecf4 Diff: https://git.reviewboard.kde.org/r/120648/diff/ Testing --- It compiles, but I can't properly test Mac and Windows scenarios Thanks, Luigi Toscano ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 120648: Encode the URIs which end up in DTD files
On April 3, 2015, 5:35 p.m., Andrius da Costa Ribas wrote:
src/CMakeLists.txt, line 20
https://git.reviewboard.kde.org/r/120648/diff/2/?file=320785#file320785line20
It's already committed (but there's been a long time since I hadn't
build anything), but now I see that DocBookXML4_DTD_DIR is used only inside
the else(NOT WIN32) part, is this what's been intended? Also, this
doesn't work when the original DocBookXML4_DTD_DIR is an absolute Windows
path V: becomes V%3A and xmllint fails to recognise it as an absolute
path (see below) and CMake would fail to install to such location. (maybe
we should prepend it with file:// ?)
file:///V:/build/frameworks/kdoctools/work/msvc2013-RelWithDebInfo-master/src/customization/dtd/kdedbx45.dtd:102:
warning: failed to load external
///V:/build/frameworks/kdoctools/work/msvc2013-RelWithDebInfo-master/src/customization/dtd/v%3A/share/xml/docbook/schema/dtd/4.5/docbookx.dtd
%DocBookDTD;
- DocBookXML4_DTD_DIR is properly touched both for non WIN32 (line 4) and WIN32
(lines 4 and 20). The WIN32 logic is a bit complicated, but basically it
rewrites again the generated ${_custom_dtd_kdex} file using the DTD path which
is knows during the installation. Or something like that.
- Oh, that's the feedback I was looking for. Can you please copy the exact
content of the path to docbookx.dtd as referenced into kdedbx45.dtd?
- Luigi
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120648/#review78442
---
On Feb. 28, 2015, 11:02 p.m., Luigi Toscano wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120648/
---
(Updated Feb. 28, 2015, 11:02 p.m.)
Review request for Build System, KDE Software on Mac OS X, KDE Frameworks,
and kdewin.
Repository: kdoctools
Description
---
The URI need to be encoded, because some valid characters for
filenames are not valid according RFC 2396.
Easy way to trigger the issue: when the path contains spaces,
as it happens on MacOSX builds.
See also https://git.reviewboard.kde.org/r/120649/ for the twin review on
kdelibs4support.
Diffs
-
cmake/uriencode.cmake PRE-CREATION
src/CMakeLists.txt 341ecf4
Diff: https://git.reviewboard.kde.org/r/120648/diff/
Testing
---
It compiles, but I can't properly test Mac and Windows scenarios
Thanks,
Luigi Toscano
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
