from:"Kevin Funk"

D26394: ECMGeneratePriFile: Fix static configurations

2020-02-07 Thread Kevin Funk

This revision was automatically updated to reflect the committed changes.
Closed by commit R240:fafbc8cec665: ECMGeneratePriFile: Fix static 
configurations (authored by kfunk).

REPOSITORY
  R240 Extra CMake Modules

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26394?vs=72687=75155

REVISION DETAIL
  https://phabricator.kde.org/D26394

AFFECTED FILES
  modules/ECMGeneratePriFile.cmake

To: kfunk, dfaure, winterz, vkrause, apol
Cc: kde-frameworks-devel, kde-buildsystem, LeGast00n, cblack, GB_2, bencreasy, 
michaelh, ngraham, bruns

D26364: SlaveBase::dispatchLoop: Fix timeout calculation

2020-01-09 Thread Kevin Funk

This revision was automatically updated to reflect the committed changes.
Closed by commit R241:27271405f408: SlaveBase::dispatchLoop: Fix timeout 
calculation (authored by Julien Goodwin jgood...@studio442.com.au, 
committed by kfunk).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26364?vs=72610=73129

REVISION DETAIL
  https://phabricator.kde.org/D26364

AFFECTED FILES
  src/core/slavebase.cpp

To: kfunk, chinmoyr, davidedmundson, dfaure, broulik
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D26394: ECMGeneratePriFile: Fix static configurations

2020-01-03 Thread Kevin Funk

kfunk added reviewers: winterz, vkrause.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D26394

To: kfunk, dfaure, winterz, vkrause
Cc: kde-frameworks-devel, kde-buildsystem, LeGast00n, GB_2, bencreasy, 
michaelh, ngraham, bruns

D26394: ECMGeneratePriFile: Fix static configurations

2020-01-03 Thread Kevin Funk

kfunk created this revision.
kfunk added a reviewer: dfaure.
Herald added projects: Frameworks, Build System.
Herald added subscribers: kde-buildsystem, kde-frameworks-devel.
kfunk requested review of this revision.

REVISION SUMMARY
  Populate module_config with staticlib. This is needed for Qt 5.12, as
  Makefiles contain the full path to the library instead of just the base
  name. QMake needs to be aware of the build type. This issue was found in
  KDStateMachineEditor's .pri files.
  
  Before this patch the linker tried to link against .so files even for
  static libraries.
  
  Note: Probably not very relevenat to KDE Frameworks (since it's all
  about shared libraries, but I'd like to keep the original
  ECMGeneratePriFile version up-to-date)
  
  Compare:
  
% cat kdsme-qmake-test.pro
QT += KDSMEDebugInterfaceSource

!qtHaveModule(KDSMEDebugInterfaceSource): warning("Library not found")

SOURCES += main.cpp

% qmake --version
QMake version 3.1
Using Qt version 5.9.8 in /home/kfunk/devel/build/qt5.9/qtbase/lib
% qmake .
% make
...
g++ -Wl,-rpath,/home/kfunk/devel/build/qt5.9/qtbase/lib ...  -L.../lib 
-lkdstatemachineeditor_debuginterfacesource ...

% make clean

% env-qt5.12
% qmake --version
QMake version 3.1
Using Qt version 5.12.5 in /home/kfunk/devel/build/qt5.12/qtbase/lib

% qmake .
% make
...
g++ -Wl,-rpath,/home/kfunk/devel/build/qt5.12/qtbase/lib ... 
.../lib/libkdstatemachineeditor_debuginterfacesource.a ...

REPOSITORY
  R240 Extra CMake Modules

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D26394

AFFECTED FILES
  modules/ECMGeneratePriFile.cmake

To: kfunk, dfaure
Cc: kde-frameworks-devel, kde-buildsystem, LeGast00n, GB_2, bencreasy, 
michaelh, ngraham, bruns

D26364: SlaveBase::dispatchLoop: Fix timeout calculation

2020-01-02 Thread Kevin Funk

kfunk added a reviewer: broulik.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D26364

To: kfunk, chinmoyr, davidedmundson, dfaure, broulik
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26364: SlaveBase::dispatchLoop: Fix timeout calculation

2020-01-02 Thread Kevin Funk

kfunk added reviewers: chinmoyr, davidedmundson, dfaure.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D26364

To: kfunk, chinmoyr, davidedmundson, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26364: SlaveBase::dispatchLoop: Fix timeout calculation

2020-01-02 Thread Kevin Funk

kfunk created this revision.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
kfunk requested review of this revision.

REVISION SUMMARY
  Old version of the code:
  
ms = qMax(d->nextTimeout.elapsed() - d->nextTimeoutMsecs, 1);
  
  ... will mean the sleep is for as long as the timer has run *minus* the
  intended duration, so if nextTimeoutMsecs is ever set and the timer just
  started this becomes very negative, and 1ms is the result.
  
  Inverting the subtraction:
  
ms = qMax(d->nextTimeoutMsecs - d->nextTimeout.elapsed(), 1);
  
  Means sleeping for the remaining time, and so far my CPU seems much
  happier, with my KIO-HTTP using apps looking fine.
  
  BUG: 392768

REPOSITORY
  R241 KIO

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D26364

AFFECTED FILES
  src/core/slavebase.cpp

To: kfunk
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D17325: Fix leak in kemoticons

2018-12-03 Thread Kevin Funk

kfunk updated this revision to Diff 46787.
kfunk added a comment.


  Use QSharedPointer

REPOSITORY
  R301 KEmoticons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17325?vs=46765=46787

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D17325

AFFECTED FILES
  src/core/kemoticonstheme.cpp

To: kfunk
Cc: anthonyfieroni, kde-frameworks-devel, michaelh, ngraham, bruns

D17325: Fix leak in kemoticons

2018-12-02 Thread Kevin Funk

kfunk added a comment.


  Not entirely sure about whether this is the right fix for this. Can someone 
check why this was commented before?
  
  The leak is fixed with this patch. Unit tests still work, too.
  
  And it kind of makes sense to have the `KEmoticonsTheme` class own its 
provider.

REPOSITORY
  R301 KEmoticons

REVISION DETAIL
  https://phabricator.kde.org/D17325

To: kfunk
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17325: Fix leak in kemoticons

2018-12-02 Thread Kevin Funk

kfunk created this revision.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
kfunk requested review of this revision.

REVISION SUMMARY
  ASAN trace:
  Indirect leak of 64 byte(s) in 2 object(s) allocated from:
  
#0 0x52a000 in operator new(unsigned long) 
(/home/kfunk/devel/install/kf5/bin/kmail+0x52a000)
#1 0x7fbac897e192 in 
QList::node_construct(QList::Node*,
 KEmoticonsProvider::Emoticon const&) 
/home/kfunk/devel/build/qt5.11/qtbase/include/QtCore/../../../../../src/qt5.11/qtbase/src/corelib/tools/qlist.h:435:65
#2 0x7fbac897d184 in 
QList::append(KEmoticonsProvider::Emoticon 
const&) 
/home/kfunk/devel/build/qt5.11/qtbase/include/QtCore/../../../../../src/qt5.11/qtbase/src/corelib/tools/qlist.h:584:13
#3 0x7fbac897c59c in KEmoticonsProvider::addIndexItem(QString const&, 
QStringList const&) 
/home/kfunk/devel/src/kf5/kemoticons/src/core/kemoticonsprovider.cpp:162:39
#4 0x7fba8a15f7b5 in KdeEmoticons::loadTheme(QString const&) 
/home/kfunk/devel/src/kf5/kemoticons/src/providers/kde/kde_emoticons.cpp:183:13
#5 0x7fbac896ceb7 in KEmoticonsPrivate::loadTheme(QString 
const&)::$_6::operator()(QString const&, QString const&, KEmoticonsProvider*) 
const /home/kfunk/devel/src/kf5/kemoticons/src/core/kemoticons.cpp:126:19
#6 0x7fbac896cca1 in KEmoticonsPrivate::loadTheme(QString const&) 
/home/kfunk/devel/src/kf5/kemoticons/src/core/kemoticons.cpp:151:24
#7 0x7fbac896d24f in KEmoticons::theme(QString const&) const 
/home/kfunk/devel/src/kf5/kemoticons/src/core/kemoticons.cpp:181:15
#8 0x7fbac896d1ab in KEmoticons::theme() const 
/home/kfunk/devel/src/kf5/kemoticons/src/core/kemoticons.cpp:171:12
#9 0x7fba8a2e25bb in KTextToHTMLEmoticons::parseEmoticons(QString const&, 
bool, QStringList const&) 
/home/kfunk/devel/src/kf5/kemoticons/src/integrationplugin/ktexttohtml.cpp:43:24
...

REPOSITORY
  R301 KEmoticons

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D17325

AFFECTED FILES
  src/core/kemoticonstheme.cpp

To: kfunk
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17031: Fix a few memory leaksASAN: Fix leak in AppletQuickItem

2018-11-20 Thread Kevin Funk

kfunk added a comment.


  Yep, those are in fact separate commits.

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D17031

To: kfunk, mart
Cc: mart, kde-frameworks-devel, michaelh, ngraham, bruns

D17031: Fix a few memory leaksASAN: Fix leak in AppletQuickItem

2018-11-20 Thread Kevin Funk

This revision was automatically updated to reflect the committed changes.
Closed by commit R242:421e1c01266e: ASAN: Fix memory leak in CalendarPlugin 
(authored by kfunk).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D17031?vs=45849=45891#toc

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17031?vs=45849=45891

REVISION DETAIL
  https://phabricator.kde.org/D17031

AFFECTED FILES
  src/declarativeimports/calendar/calendarplugin.cpp

To: kfunk, mart
Cc: mart, kde-frameworks-devel, michaelh, ngraham, bruns

D17031: Fix a few memory leaksASAN: Fix leak in AppletQuickItem

2018-11-19 Thread Kevin Funk

kfunk created this revision.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
kfunk requested review of this revision.

REVISION SUMMARY
  Direct leak of 3960 byte(s) in 15 object(s) allocated from:
  
#0 0x544cc0 in operator new(unsigned long) 
(/home/kfunk/devel/install/kf5/bin/plasmashell+0x544cc0)
#1 0x7f0dd8a3e2bd in 
PlasmaQuick::AppletQuickItem::AppletQuickItem(Plasma::Applet*, QQuickItem*) 
/home/kfunk/devel/src/kf5/plasma-framework/src/plasmaquick/appletquickitem.cpp:472:9
#2 0x7f0dcb7b365b in 
AppletInterface::AppletInterface(DeclarativeAppletScript*, QList 
const&, QQuickItem*) 
/home/kfunk/devel/src/kf5/plasma-framework/src/scriptengines/qml/plasmoid/appletinterface.cpp:50:7
#3 0x7f0dcb7aede1 in DeclarativeAppletScript::init() 
/home/kfunk/devel/src/kf5/plasma-framework/src/scriptengines/qml/plasmoid/declarativeappletscript.cpp:87:27
...
  
  ASAN: Fix memory leak in Corona
  
  KPackagePrivate::internalPackage already existed, the re-assignment to a
  new value causes a memory leak.
  
  Trace:
  Indirect leak of 112 byte(s) in 1 object(s) allocated from:
  
#0 0x544cc0 in operator new(unsigned long) 
(/home/kfunk/devel/install/kf5/bin/plasmashell+0x544cc0)
#1 0x7f905829163f in 
KPackage::Package::Package(KPackage::PackageStructure*) 
/home/kfunk/devel/src/kf5/kpackage/src/kpackage/package.cpp:51:9
#2 0x7f9058fad786 in Plasma::Package::Package(Plasma::PackageStructure*) 
/home/kfunk/devel/src/kf5/plasma-framework/src/plasma/package.cpp:66:34
#3 0x7f9058f14dee in Plasma::Corona::package() const 
/home/kfunk/devel/src/kf5/plasma-framework/src/plasma/corona.cpp:78:13
#4 0x5d9eb9 in ShellCorona::ShellCorona(QObject*) 
/home/kfunk/devel/src/kf5/plasma-workspace/shell/shellcorona.cpp:132:70
#5 0x65c31d in ShellManager::loadHandlers() 
/home/kfunk/devel/src/kf5/plasma-workspace/shell/shellmanager.cpp:93:21
  
  ASAN: Fix memory leak in DataSource
  
  Trace:
  Direct leak of 216 byte(s) in 27 object(s) allocated from:
  
#0 0x544cc0 in operator new(unsigned long) 
(/home/kfunk/devel/install/kf5/bin/plasmashell+0x544cc0)
#1 0x7fb2e9cd77d9 in Plasma::DataSource::setEngine(QString const&) 
/home/kfunk/devel/src/kf5/plasma-framework/src/declarativeimports/core/datasource.cpp:93:28
#2 0x7fb2e9d5c8f1 in Plasma::DataSource::qt_static_metacall(QObject*, 
QMetaObject::Call, int, void**) 
/home/kfunk/devel/build/kf5/plasma-framework/src/declarativeimports/core/corebindingsplugin_autogen/EWIEGA46WW/moc_datasource.cpp:330:21
  
  ASAN: Fix memory leak in CalendarPlugin
  
  Trace:
  Indirect leak of 24 byte(s) in 1 object(s) allocated from:
  
#0 0x508a17 in __interceptor_malloc 
(/home/kfunk/devel/install/kf5/bin/plasmashell+0x508a17)
#1 0x7fcf92aa6230 in QHashData::allocateNode(int) 
/home/kfunk/devel/src/qt5.11/qtbase/src/corelib/tools/qhash.cpp:479:79
#2 0x7fcf7a851d10 in QHash::detach() 
/home/kfunk/devel/build/qt5.11/qtbase/include/QtCore/../../../../../src/qt5.11/qtbase/src/corelib/tools/qhash.h:275:51
#3 0x7fcf7a8519c9 in QHash::insert(int const&, QByteArray 
const&) 
/home/kfunk/devel/build/qt5.11/qtbase/include/QtCore/../../../../../src/qt5.11/qtbase/src/corelib/tools/qhash.h:769:5
#4 0x7fcf7a859736 in 
EventPluginsModel::EventPluginsModel(EventPluginsManager*) 
/home/kfunk/devel/src/kf5/plasma-framework/src/declarativeimports/calendar/eventpluginsmanager.cpp:42:17
#5 0x7fcf7a854c55 in EventPluginsManager::EventPluginsManager(QObject*) 
/home/kfunk/devel/src/kf5/plasma-framework/src/declarativeimports/calendar/eventpluginsmanager.cpp:185:19
...

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D17031

AFFECTED FILES
  src/declarativeimports/calendar/calendarplugin.cpp
  src/declarativeimports/core/datasource.cpp
  src/declarativeimports/core/datasource.h
  src/plasma/corona.cpp
  src/plasmaquick/appletquickitem.cpp

To: kfunk
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D16894: [ECM] use a macro to add compiler flags conditionally

2018-11-16 Thread Kevin Funk

kfunk requested changes to this revision.
kfunk added a comment.
This revision now requires changes to proceed.


  I don't like the hiding of the if-branches as argument to macro. We shouldn't 
to this as it makes the code harder to understand.
  
  The general issue with the existing code here is: Statements like 
`CMAKE_CXX_COMPILER_ID MATCHES "Clang" AND NOT CMAKE_CXX_COMPILER_VERSION 
VERSION_LESS 3.5` do not make sense. If we check the compiler version, then we 
need to differentiate between Clang and AppleClang (cf. something like 
https://github.com/Microsoft/LightGBM/blob/master/CMakeLists.txt#L20).
  
  Thus these places need to be turned into:
  
...
if(CMAKE_CXX_COMPILER_ID STREQUAL "Clang" AND CMAKE_CXX_COMPILER_VERSION 
VERSION_LESS "3.8")
  ...
endif()
elseif(CMAKE_CXX_COMPILER_ID STREQUAL "AppleClang" AND 
CMAKE_CXX_COMPILER_VERSION VERSION_LESS "8.1.0")
  ...
endif()
  
  If you're unsure in which version of AppleClang a certain feature was 
introduced then:
  
...
elseif(CMAKE_CXX_COMPILER_ID STREQUAL "AppleClang" AND 
CMAKE_CXX_COMPILER_VERSION VERSION_LESS "8.1.0")
  add_compile_flag_if_supported(...)
endif()
  
  Please check out the functions provided in: 
kdevelop.git:cmake/modules/KDevelopMacrosInternal.cmake
  
#   add_compile_flag_if_supported( [CXX_ONLY])
#   add_target_compile_flag_if_supported( 
 )
  
  They are more clean than your implementation, I think it would make sense to 
actually add them to KDECompilerSettings.cmake and prefix them with `kde_`.

INLINE COMMENTS

> KDECompilerSettings.cmake:202
> +# is expected to support _FLAG.
> +if (${ARGN})
> +if(APPLE AND CMAKE_CXX_COMPILER_ID MATCHES "Clang")

This "let's abuse ${ARGN} as code to be evaluated later" is pretty ugly. The 
previous version (with the if-statements at  the caller side) is way more clean 
and understandable.

> KDECompilerSettings.cmake:203
> +if (${ARGN})
> +if(APPLE AND CMAKE_CXX_COMPILER_ID MATCHES "Clang")
> +# Clang on APPLE needs verification because of Apple's

I don't really understand why this branch is needed. The macro name name 
suggests it does the compile check on all compilers. Again very misleading.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D16894

To: rjvbb, #build_system, kfunk
Cc: kfunk, apol, kde-frameworks-devel, kde-buildsystem, #build_system, 
michaelh, ngraham, bruns

D15002: Allow to install syntax files instead of having them in a resource

2018-10-23 Thread Kevin Funk

kfunk added inline comments.

INLINE COMMENTS

> CMakeLists.txt:3
>  macro(generate_php_syntax_definition targetFile srcFile)
> -add_custom_command(
> -OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/${targetFile}
> -COMMAND ${PERL_EXECUTABLE} 
> ${CMAKE_CURRENT_SOURCE_DIR}/generators/generate-php.pl < 
> ${CMAKE_CURRENT_SOURCE_DIR}/syntax/${srcFile} > 
> ${CMAKE_CURRENT_BINARY_DIR}/${targetFile}
> -DEPENDS ${CMAKE_CURRENT_SOURCE_DIR}/generators/generate-php.pl 
> ${CMAKE_CURRENT_SOURCE_DIR}/syntax/${srcFile}
> -)
> +execute_process(COMMAND ${CMAKE_COMMAND} -E make_directory 
> ${CMAKE_CURRENT_BINARY_DIR}/syntax)
> +execute_process(COMMAND ${PERL_EXECUTABLE} 
> ${CMAKE_CURRENT_SOURCE_DIR}/generators/generate-php.pl

Note: The Ninja generator doesn't like this:

  CMake Warning (dev):
Policy CMP0058 is not set: Ninja requires custom command byproducts to be
explicit.  Run "cmake --help-policy CMP0058" for policy details.  Use the
cmake_policy command to set the policy and suppress this warning.
  
This project specifies custom command DEPENDS on files in the build tree
that are not specified as the OUTPUT or BYPRODUCTS of any
add_custom_command or add_custom_target:
  
 data/syntax/css-php.xml
 data/syntax/html-php.xml
 data/syntax/javascript-php.xml
  
For compatibility with versions of CMake that did not have the BYPRODUCTS
option, CMake is generating phony rules for such files to convince 'ninja'
to build.
  
Project authors should add the missing BYPRODUCTS or OUTPUT options to the
custom commands that produce these files.
  This warning is for project developers.  Use -Wno-dev to suppress it.

There must be a way to implement this feature and keep using 
`add_custom_command`...? Using `execute_process()` is usually a bad sign (tm). 
Note that these commands will be executed each CMake run(!)

REPOSITORY
  R216 Syntax Highlighting

REVISION DETAIL
  https://phabricator.kde.org/D15002

To: cullmann, vkrause, dhaumann
Cc: kfunk, dhaumann, kwrite-devel, kde-frameworks-devel, michaelh, ngraham, 
bruns, demsking, cullmann, sars

D14987: hunspell: Restore build with hunspell <=v1.5.0

2018-08-22 Thread Kevin Funk

This revision was automatically updated to reflect the committed changes.
Closed by commit R246:a18b3810a18d: hunspell: Restore build with hunspell 
=v1.5.0 (authored by kfunk).

REPOSITORY
  R246 Sonnet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14987?vs=40190=40200

REVISION DETAIL
  https://phabricator.kde.org/D14987

AFFECTED FILES
  src/plugins/hunspell/CMakeLists.txt
  src/plugins/hunspell/config-hunspell.h.cmake
  src/plugins/hunspell/hunspelldict.cpp

To: kfunk, rjvbb, dfaure, mlaurent
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D14987: hunspell: Restore build with hunspell <=v1.5.0

2018-08-22 Thread Kevin Funk

kfunk added inline comments.

INLINE COMMENTS

> dfaure wrote in hunspelldict.cpp:130
> lst.reserve(nbWord);

Note: That's just old/copied code. Unlikely to be called on recent distros 
anyway.

REPOSITORY
  R246 Sonnet

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D14987

To: kfunk, rjvbb, dfaure, mlaurent
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D14987: hunspell: Restore build with hunspell <=v1.5.0

2018-08-22 Thread Kevin Funk

kfunk added reviewers: rjvbb, dfaure, mlaurent.

REPOSITORY
  R246 Sonnet

REVISION DETAIL
  https://phabricator.kde.org/D14987

To: kfunk, rjvbb, dfaure, mlaurent
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D14987: hunspell: Restore build with hunspell <=v1.5.0

2018-08-22 Thread Kevin Funk

kfunk created this revision.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
kfunk requested review of this revision.

REVISION SUMMARY
  Commit 0a96acf251baa5c9dd042d093ab2bf8fcee10502 
 
broke compatibility with
  hunspell versions earlier than v1.5.1. v1.5.1 is was released Nov 2016,
  thus only two years old.
  
  This patch restores the build with hunspell versions provided on more
  conservative platforms (e.g. CentOS 6.8, which provides hunspell v1.2.x)

REPOSITORY
  R246 Sonnet

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D14987

AFFECTED FILES
  src/plugins/hunspell/CMakeLists.txt
  src/plugins/hunspell/config-hunspell.h.cmake
  src/plugins/hunspell/hunspelldict.cpp

To: kfunk
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D11193: Sonnet : use current hunspell API

2018-08-21 Thread Kevin Funk

kfunk added a comment.


  Please see concerns on 
https://phabricator.kde.org/R246:0a96acf251baa5c9dd042d093ab2bf8fcee10502

REPOSITORY
  R246 Sonnet

REVISION DETAIL
  https://phabricator.kde.org/D11193

To: rjvbb, #frameworks, dfaure, mlaurent, vkrause
Cc: kfunk, kde-frameworks-devel, michaelh, ngraham, bruns

D9408: extractors: Hide warnings from system headers

2018-04-27 Thread Kevin Funk

This revision was automatically updated to reflect the committed changes.
Closed by commit R286:b680861aa2ed: extractors: Hide warnings from system 
headers (authored by kfunk).

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D9408?vs=33189=33191

REVISION DETAIL
  https://phabricator.kde.org/D9408

AFFECTED FILES
  src/extractors/CMakeLists.txt

To: kfunk, mgallien
Cc: #frameworks, ashaposhnikov, michaelh, astippich, spoorun, bruns

D9408: extractors: Hide warnings from system headers

2018-04-26 Thread Kevin Funk

kfunk updated this revision to Diff 33189.
kfunk added a comment.


  Add PRIVATE

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D9408?vs=33121=33189

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D9408

AFFECTED FILES
  src/extractors/CMakeLists.txt

To: kfunk, mgallien
Cc: #frameworks, ashaposhnikov, michaelh, astippich, spoorun, bruns

Re: kfilemetadata compile failure

2018-04-26 Thread Kevin Funk

On Wednesday, 25 April 2018 14:34:58 CEST Jonathan Riddell wrote:
> kfilemetadata does not compile in KDE neon from git master currently
> 
> /workspace/build/src/extractors/ffmpegextractor.cpp:97:15: error:
> ‘AVCodecParameters’ does not name a type
> 12:27:35  const AVCodecParameters* codec = stream->codecpar;
> 
> https://build.neon.kde.org/job/xenial_unstable_kde_kfilemetadata_bin_amd64/1
> 45/console

Raised a concern on the resp. Phab Diff (to ping the responsible people):
  https://phabricator.kde.org/R286:037208a787e0c2412ab616ff1573c323a2346d2d

Regards,
Kevin
 
> It may be expecting a newer version of ffmpeg
> 
> Jonathan


-- 
Kevin Funk | kf...@kde.org | http://kfunk.org

signature.asc
Description: This is a digitally signed message part.

D9408: extractors: Hide warnings from system headers

2018-04-26 Thread Kevin Funk

kfunk added a reviewer: mgallien.

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D9408

To: kfunk, mgallien
Cc: #frameworks, ashaposhnikov, michaelh, astippich, spoorun, bruns

D9408: extractors: Hide warnings from system headers

2018-04-26 Thread Kevin Funk

kfunk updated this revision to Diff 33121.
kfunk added a comment.
Restricted Application added a project: Baloo.


  Rebased

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D9408?vs=24101=33121

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D9408

AFFECTED FILES
  src/extractors/CMakeLists.txt

To: kfunk
Cc: #frameworks, ashaposhnikov, michaelh, astippich, spoorun, bruns

D12112: Fix compiler warning under Clang

2018-04-11 Thread Kevin Funk

This revision was automatically updated to reflect the committed changes.
Closed by commit R244:53f44072c70a: Fix compiler warning under Clang (authored 
by kfunk).

REPOSITORY
  R244 KCoreAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12112?vs=31888=31914

REVISION DETAIL
  https://phabricator.kde.org/D12112

AFFECTED FILES
  src/lib/caching/kshareddatacache_p.h

To: kfunk, apol
Cc: apol, #frameworks, michaelh, ngraham, bruns

D12112: Fix compiler warning under Clang

2018-04-11 Thread Kevin Funk

kfunk updated this revision to Diff 31888.
kfunk added a comment.


  Address concerns. I knew why I put this up for review. /me grabs another 
coffee...

REPOSITORY
  R244 KCoreAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12112?vs=31878=31888

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D12112

AFFECTED FILES
  src/lib/caching/kshareddatacache_p.h

To: kfunk, apol
Cc: apol, #frameworks, michaelh, ngraham, bruns

D12112: Fix compiler warning under Clang

2018-04-11 Thread Kevin Funk

kfunk created this revision.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: Frameworks.
kfunk requested review of this revision.

REVISION SUMMARY
  Issue:
  In file included from .../kcoreaddons/src/lib/caching/kshareddatacache.cpp:25:
  .../kcoreaddons/src/lib/caching/kshareddatacache_p.h:169:47: warning: unknown 
attribute 'artificial' ignored [-Wunknown-attributes]
  
__attribute__((always_inline, gnu_inline, artificial))
  ^

REPOSITORY
  R244 KCoreAddons

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D12112

AFFECTED FILES
  src/lib/caching/kshareddatacache_p.h

To: kfunk
Cc: #frameworks, michaelh, ngraham, bruns

D11610: clang-tidy: modernize-use-default-member-init run

2018-04-11 Thread Kevin Funk

This revision was automatically updated to reflect the committed changes.
Closed by commit R39:40d801599549: clang-tidy: 
modernize-use-default-member-init run (authored by kfunk).

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11610?vs=30305=31852

REVISION DETAIL
  https://phabricator.kde.org/D11610

AFFECTED FILES
  autotests/src/plaintextsearch_test.cpp
  autotests/src/plaintextsearch_test.h
  autotests/src/scriptdocument_test.cpp
  autotests/src/scriptdocument_test.h
  autotests/src/vimode/emulatedcommandbar.cpp
  autotests/src/vimode/emulatedcommandbar.h
  src/buffer/katetexthistory.h
  src/buffer/katetextline.cpp
  src/buffer/katetextline.h
  src/completion/katecompletionmodel.cpp
  src/completion/katecompletionmodel.h
  src/completion/katecompletionwidget.h
  src/dialogs/kateconfigpage.cpp
  src/dialogs/kateconfigpage.h
  src/document/katedocument.cpp
  src/document/katedocument.h
  src/include/ktexteditor/configinterface.h
  src/include/ktexteditor/cursor.h
  src/include/ktexteditor/markinterface.h
  src/include/ktexteditor/modificationinterface.h
  src/include/ktexteditor/movinginterface.h
  src/include/ktexteditor/movingrangefeedback.h
  src/include/ktexteditor/sessionconfiginterface.h
  src/include/ktexteditor/texthintinterface.h
  src/mode/katemodemanager.h
  src/printing/printpainter.cpp
  src/render/katerenderrange.cpp
  src/render/katerenderrange.h
  src/render/katetextlayout.cpp
  src/render/katetextlayout.h
  src/schema/katecolortreewidget.h
  src/script/kateindentscript.h
  src/script/katescript.h
  src/spellcheck/prefixstore.cpp
  src/spellcheck/prefixstore.h
  src/spellcheck/spellcheckbar.cpp
  src/syntax/katehighlight.h
  src/syntax/katesyntaxmanager.cpp
  src/syntax/katesyntaxmanager.h
  src/utils/codecompletionmodel.cpp
  src/utils/configinterface.cpp
  src/utils/kateconfig.cpp
  src/utils/kateconfig.h
  src/utils/ktexteditor.cpp
  src/utils/movinginterface.cpp
  src/utils/movingrangefeedback.cpp
  src/vimode/modes/modebase.h

To: kfunk, dhaumann
Cc: dhaumann, #frameworks, michaelh, kevinapavew, ngraham, bruns, demsking, 
cullmann, sars

D11948: [KFileWidget] Hardcode example user name

2018-04-10 Thread Kevin Funk

kfunk accepted this revision.
kfunk added a comment.
This revision is now accepted and ready to land.


  Makes sense to me. Go for it.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D11948

To: broulik, #frameworks, kfunk
Cc: kfunk, elvisangelaccio, michaelh, ngraham, bruns

D5784: Add support for FreeBSD in FSUtils::getDirectoryFileSystem().

2018-04-09 Thread Kevin Funk

kfunk resigned from this revision.

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D5784

To: tcberner, #freebsd, poboiko, bruns
Cc: bruns, adridg, kfunk, #frameworks, ashaposhnikov, michaelh, astippich, 
spoorun, ngraham, alexeymin

Re: Asking for recommendations on how to add a feature to KTextEditor

2018-04-09 Thread Kevin Funk

On Monday, 9 April 2018 13:13:46 CEST Aleix Pol wrote:
> On Mon, Apr 9, 2018 at 12:49 PM, Michal Srb <michal...@gmail.com> wrote:
> > Hi,
> > 
> > I would like to ask for hints/recommendations on how to implement a
> > feature for KTextEditor.
> > 
> > My ultimate goal is to make a plugin for KDevelop that will show some
> > additional information in the code itself. For example names of
> > function arguments at call site. Something like this in intellij:
> > https://d3nmt5vlzunoa1.cloudfront.net/idea/files/2016/09/Screen-Shot-2016-> 
> > > 09-27-at-10.29.15.png

Heya Michal,

That indeed would be super helpful to have, for a variety of use-cases.

+1 on the idea for the feature!

Can't help a lot with implementing this correctly in KTextEditor, though. The 
Kate people should chime in there :)

Cheers,
Kevin

> > For this I would need a way to render something in the text, affecting
> > the layout of the line. It would be nice to be able to place any
> > general widget or image into the line, but even plain text would be
> > sufficient. It must behave differently than the "real" text around it,
> > such that it can not be edited, cursor skips it, copying does not copy
> > it, etc.
> > 
> > So far I thought of adding something like `AnnotationViewInterface`,
> > which would allow to add this kind of immutable text on line+column
> > position. Then in `KateRenderer` it could take these into
> > consideration when layouting the line and render it in. It would be
> > visible in the text, but never become a real part of the text.
> > 
> > Would you recommend some other approach, or is there perhaps a way to
> > do it without modifying KTextEditor?
> 
> Including kwrite-devel where many kate developers live. :)
> Looking forward to seeing your feature implemented in KDevelop! :D
> 
> Aleix


-- 
Kevin Funk | kf...@kde.org | http://kfunk.org

signature.asc
Description: This is a digitally signed message part.

D10739: Remove deprecated cmake code

2018-03-25 Thread Kevin Funk

kfunk accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R246 Sonnet

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D10739

To: vonreth, kfunk, dfaure, vkrause, mlaurent
Cc: apol, #frameworks, michaelh, ngraham

D5865: Add missing KDE_ENABLE_NAMED_OPERATORS function

2018-03-25 Thread Kevin Funk

kfunk requested changes to this revision.
This revision now requires changes to proceed.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D5865

To: rjvbb, #frameworks, #build_system, cgilles, kfunk
Cc: thiago, kfunk, michaelh, ngraham

D11610: clang-tidy: modernize-use-default-member-init run

2018-03-25 Thread Kevin Funk

kfunk added a comment.


  I'll wait until v5.45 got branched off. No need to hurry with this.

REPOSITORY
  R39 KTextEditor

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D11610

To: kfunk, dhaumann
Cc: dhaumann, #frameworks, michaelh, kevinapavew, ngraham, demsking, cullmann, 
sars

D11610: clang-tidy: modernize-use-default-member-init run

2018-03-23 Thread Kevin Funk

kfunk edited the summary of this revision.
kfunk edited the test plan for this revision.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D11610

To: kfunk
Cc: #frameworks, michaelh, kevinapavew, ngraham, demsking, cullmann, sars, 
dhaumann

D11610: clang-tidy: modernize-use-default-member-init run

2018-03-23 Thread Kevin Funk

kfunk created this revision.
Restricted Application added projects: Kate, Frameworks.
Restricted Application added a subscriber: Frameworks.
kfunk requested review of this revision.

REPOSITORY
  R39 KTextEditor

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D11610

AFFECTED FILES
  autotests/src/plaintextsearch_test.cpp
  autotests/src/plaintextsearch_test.h
  autotests/src/scriptdocument_test.cpp
  autotests/src/scriptdocument_test.h
  autotests/src/vimode/emulatedcommandbar.cpp
  autotests/src/vimode/emulatedcommandbar.h
  src/buffer/katetexthistory.h
  src/buffer/katetextline.cpp
  src/buffer/katetextline.h
  src/completion/katecompletionmodel.cpp
  src/completion/katecompletionmodel.h
  src/completion/katecompletionwidget.h
  src/dialogs/kateconfigpage.cpp
  src/dialogs/kateconfigpage.h
  src/document/katedocument.cpp
  src/document/katedocument.h
  src/include/ktexteditor/configinterface.h
  src/include/ktexteditor/cursor.h
  src/include/ktexteditor/markinterface.h
  src/include/ktexteditor/modificationinterface.h
  src/include/ktexteditor/movinginterface.h
  src/include/ktexteditor/movingrangefeedback.h
  src/include/ktexteditor/sessionconfiginterface.h
  src/include/ktexteditor/texthintinterface.h
  src/mode/katemodemanager.h
  src/printing/printpainter.cpp
  src/render/katerenderrange.cpp
  src/render/katerenderrange.h
  src/render/katetextlayout.cpp
  src/render/katetextlayout.h
  src/schema/katecolortreewidget.h
  src/script/kateindentscript.h
  src/script/katescript.h
  src/spellcheck/prefixstore.cpp
  src/spellcheck/prefixstore.h
  src/spellcheck/spellcheckbar.cpp
  src/syntax/katehighlight.h
  src/syntax/katesyntaxmanager.cpp
  src/syntax/katesyntaxmanager.h
  src/utils/codecompletionmodel.cpp
  src/utils/configinterface.cpp
  src/utils/kateconfig.cpp
  src/utils/kateconfig.h
  src/utils/ktexteditor.cpp
  src/utils/movinginterface.cpp
  src/utils/movingrangefeedback.cpp
  src/vimode/modes/modebase.h

To: kfunk
Cc: #frameworks, michaelh, kevinapavew, ngraham, demsking, cullmann, sars, 
dhaumann

D11278: [KateCompletionWidget] Create configuration interface on demand

2018-03-13 Thread Kevin Funk

kfunk accepted this revision.
kfunk added a comment.
This revision is now accepted and ready to land.

  In D11278#224484 , @kfunk wrote:

  > Looks like you don't need the member at all? Otherwise late-init wouldn't 
work this way.
  >
  > `m_configWidget` seems only used in `showConfig()`. Let's remove the member 
altogether?

  Ah, disregard my comment.  If `showConfig()` is invoked multiple times then 
my approach would be slower.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D11278

To: broulik, #ktexteditor, dhaumann, kfunk
Cc: kfunk, #frameworks, michaelh, kevinapavew, ngraham, demsking, cullmann, 
sars, dhaumann

D11278: [KateCompletionWidget] Create configuration interface on demand

2018-03-13 Thread Kevin Funk

kfunk added a comment.


  Looks like you don't need the member at all? Otherwise late-init wouldn't 
work this way.
  
  `m_configWidget` seems only used in `showConfig()`. Let's remove the member 
altogether?

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D11278

To: broulik, #ktexteditor, dhaumann
Cc: kfunk, #frameworks, michaelh, kevinapavew, ngraham, demsking, cullmann, 
sars, dhaumann

Re: Converting KDE4 projects to KF5

2018-02-21 Thread Kevin Funk

On Wednesday, 21 February 2018 14:42:20 CET Robin Atwood wrote:
> I have several old projects that really needed to be updated to KF5. Is
> there a handy guide to doing the conversion?

Heya Robin,

note: wrong mailing list. This is the mailing list dealing with KDevelop, the 
IDE :)

I've CC'd the correct mailing list now.

Anyway, for porting your applications to KF5, check out:
  http://www.proli.net/2014/06/21/porting-your-project-to-qt5kf5/

I'd especially pay attention to:
  http://community.kde.org/Frameworks/Porting_Notes

And I'd strongly recommend you to use the porting scripts here:
  git://anongit.kde.org/kde-dev-scripts, subdirectory kf5

Happy porting!

Regards,
Kevin

> Thanks
> Robin


-- 
Kevin Funk | kf...@kde.org | http://kfunk.org

signature.asc
Description: This is a digitally signed message part.

Re: KDirListerCache destroy comment in kcoredirlister.cpp

2018-01-29 Thread Kevin Funk

On Monday, 29 January 2018 11:32:56 CET René J.V. Bertin wrote:
> Hi,
> 
> Researching an issue I'm seeing (hang/crash on exit) I found the following
> vestigial out-commented code the KCoreDirListerCache ctor in
> kcoredirlister.cpp:

FYI: Note to the mailing list, before more precious developer time is being 
spent on this here:

There are two more follow-up posts, just a few more hours younger, related to 
this issue on qt-interest:
  http://lists.qt-project.org/pipermail/interest/2018-January/029182.html
  (rather unrelated, but is also hinting at said problem...)
  http://lists.qt-project.org/pipermail/interest/2018-January/029194.html
  (asks about help for debugging crashes during destruction of QRegExpEngine)

Regards,
Kevin

> // Probably not needed in KF5 anymore:
> // The use of KUrl::url() in ~DirItem (sendSignal) crashes if the static
> for QRegExpEngine got deleted already, // so we need to destroy the
> KCoreDirListerCache before that.
> //qAddPostRoutine(kDirListerCache.destroy);
> 
> WHat's clear is that this no longer works: there is no `destroy` method in
> whatever kDirListerCache's real class is.
> 
> Git blame is of little help here; can anyone remember why the explicit
> pro-active delete was deemed unnecessary, and what the exact symptoms were
> when this was not done in KDE4?
> 
> FWIW, the crash (or hang) happens during QRegExpEngine take-down when I have
> used a KDE file dialog in a "pure Qt" application via a mechanism built on
> code taken from the plasma integration plugin. It does not happen in KF5
> applications like kate or kdialog. As far as I know those applications
> obtain the KDE file dialog from the platform plugin too. The difference
> must be in the order in which certain things are deleted during the global
> destruction phase, but which and how to restore the proper order. (The
> Audacious audioplayer had a similar issue on exit in its Qt5 version, which
> I traced to the QApplication instance being deleted via atexit; a lucky
> shot in the dark that makes a lot of sense in retrospect.)
> 
> Thanks for any pointers,
> R.


-- 
Kevin Funk | kf...@kde.org | http://kfunk.org

signature.asc
Description: This is a digitally signed message part.

D9766: Use at least the requested width for the argument hint tree

2018-01-18 Thread Kevin Funk

kfunk added a comment.


  Whoops. Sorry for accidentally reopening it to begin with.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D9766

To: mwolff, #kdevelop, #kate
Cc: dhaumann, kfunk, #frameworks, michaelh, kevinapavew, ngraham, demsking, 
cullmann, sars

D9766: Use at least the requested width for the argument hint tree

2018-01-16 Thread Kevin Funk

kfunk reopened this revision.
kfunk added a comment.


  LGTM.
  
  Just a had a quick look at the appearance of the popup before vs. after the 
patch when doing code completion inside the a function call parameter list. 
Looks more consisten now, thanks!

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D9766

To: mwolff, #kdevelop, #kate
Cc: dhaumann, kfunk, #frameworks, michaelh, kevinapavew, ngraham, demsking, 
cullmann, sars

D9760: Fix kded dbus name in solid-networking howto

2018-01-09 Thread Kevin Funk

kfunk accepted this revision.

REPOSITORY
  R239 KDELibs4Support

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D9760

To: habacker, kfunk
Cc: kfunk, #frameworks

D9760: Fix kded dbus name in solid-networking howto

2018-01-09 Thread Kevin Funk

kfunk accepted this revision.
kfunk added a comment.
This revision is now accepted and ready to land.


  //src/solid-networkstatus/DESIGN// also still references `org.kde.kded`. 
Please update, too.
  
  Rest LGTM

REPOSITORY
  R239 KDELibs4Support

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D9760

To: habacker, kfunk
Cc: kfunk, #frameworks

D9750: ExpandingWidgetModel: find the right-most column based on location

2018-01-09 Thread Kevin Funk

kfunk added a comment.


  LGTM, but I'll let someone else approve.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D9750

To: mwolff, #kate
Cc: kfunk, #frameworks, michaelh, kevinapavew, ngraham, demsking, cullmann, 
sars, dhaumann

D9749: Simplify code: return early to reduce indentation depth

2018-01-09 Thread Kevin Funk

kfunk accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D9749

To: mwolff, #kate, kfunk
Cc: #frameworks, michaelh, kevinapavew, ngraham, demsking, cullmann, sars, 
dhaumann

D9423: Fix 'Exec line in kiod service file must not have any path prefix on Windows'

2017-12-20 Thread Kevin Funk

kfunk added a comment.


  In https://phabricator.kde.org/D9423#181460, @habacker wrote:
  
  > In https://phabricator.kde.org/D9423#181442, @kfunk wrote:
  >
  > > And I agree. There shouldn't be a need to use two different input files. 
That's the whole point of `configure_file(...)`: the interpolation of values 
happens *inside* the input files.
  >
  >
  > This would be possible by using in the service file something like this
  >
  > [D-BUS Service]
  >  Name=org.kde.kiod5
  >  Exec=@SOME_PREFIX@kiod5
  
  
  Sounds good.
  
  In the top-level CMakeLists.txt maybe:
  
if(WIN32)
 set(DBUS_LIBEXECDIR)
else()
 set(DBUS_LIBEXECDIR ${KDE_INSTALL_FULL_LIBEXECDIR}/kf5/)
endif()
  
  ... and then use that everywhere needed.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D9423

To: habacker, dfaure, bcooksley, kfunk
Cc: kfunk, broulik, #frameworks

D9423: Fix 'Exec line in kiod service file must not have any path prefix on Windows'

2017-12-20 Thread Kevin Funk

kfunk added a comment.


  @dfaure Opinions?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D9423

To: habacker, dfaure, bcooksley, kfunk
Cc: kfunk, broulik, #frameworks

D9424: Fix 'Exec line in kglobalaccel5 service file must not have any path prefix on Windows'

2017-12-20 Thread Kevin Funk

kfunk added a comment.


  Please let's discuss on https://phabricator.kde.org/D9423 exclusively. This 
is basically fixing the same issue in a different repository.

REPOSITORY
  R268 KGlobalAccel

REVISION DETAIL
  https://phabricator.kde.org/D9424

To: habacker, dfaure, bcooksley, apol
Cc: kfunk, vonreth, #windows, apol, #frameworks

D9423: Fix 'Exec line in kiod service file must not have any path prefix on Windows'

2017-12-20 Thread Kevin Funk

kfunk requested changes to this revision.
kfunk added a comment.
This revision now requires changes to proceed.


  And I agree. There shouldn't be a need to use two different input files. 
That's the whole point of `configure_file(...)` the interpolation of values 
happens *inside* the input files.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D9423

To: habacker, dfaure, bcooksley, kfunk
Cc: kfunk, broulik, #frameworks

D9423: Fix 'Exec line in kiod service file must not have any path prefix on Windows'

2017-12-20 Thread Kevin Funk

kfunk added a comment.


  Could you please squash the commits and just keep this review for all the 
changes in KIO? Doesn't make sense to have different Phab Diffs for that (makes 
it harder to review, harder to revert in case, etc.).

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D9423

To: habacker, dfaure, bcooksley
Cc: kfunk, broulik, #frameworks

D9398: Fix TagLib detection and build on Windows

2017-12-19 Thread Kevin Funk

This revision was automatically updated to reflect the committed changes.
Closed by commit R320:5e6c6fe0c7a1: Fix TagLib detection and build on Windows 
(authored by kfunk).

REPOSITORY
  R320 KIO Extras

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D9398?vs=24075=24104

REVISION DETAIL
  https://phabricator.kde.org/D9398

AFFECTED FILES
  cmake/FindTaglib.cmake
  thumbnail/audiocreator.cpp

To: leinir, #frameworks, kfunk
Cc: vonreth

D9408: extractors: Hide warnings from system headers

2017-12-19 Thread Kevin Funk

kfunk created this revision.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: Frameworks.

REVISION SUMMARY
  Also prefer kde_target_enable_exceptions() over the function enabling
  exceptions globally.

REPOSITORY
  R286 KFileMetaData

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D9408

AFFECTED FILES
  src/extractors/CMakeLists.txt

To: kfunk
Cc: #frameworks

D9398: Fix TagLib detection and build on Windows

2017-12-18 Thread Kevin Funk

kfunk accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D9398

To: leinir, #frameworks, kfunk
Cc: vonreth

D9299: Introduce INSTALL_PREFIX_SCRIPT to easily set up prefixes

2017-12-15 Thread Kevin Funk

kfunk added a comment.


  Sorry for being late, but I didn't have time to follow up on this one.
  
  My concerns:
  
  - If you have that option `ON` and all frameworks install into the same 
prefix, `prefix.sh` will be overwritten. => Bad.
  - I still don't fully see how to adopt the use case apol mentioned: Say each 
Framework installs into its own unique prefix. => Ok.
- But how are you supposed to source each `prefix.sh`? There must be script 
to do this? Where's the documentation then?

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D9299

To: apol, #frameworks, sitter
Cc: kfunk, bcooksley, ngraham, sitter, cgiboudeaux, #build_system

D9321: Fix another CMake CMP0071 warning

2017-12-14 Thread Kevin Funk

This revision was automatically updated to reflect the committed changes.
Closed by commit R216:fb80c0549aed: Fix another CMake CMP0071 warning (authored 
by kfunk).

REPOSITORY
  R216 Syntax Highlighting

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D9321?vs=23887=23896

REVISION DETAIL
  https://phabricator.kde.org/D9321

AFFECTED FILES
  data/CMakeLists.txt

To: kfunk, mlaurent, dfaure
Cc: #frameworks

D9321: Fix another CMake CMP0071 warning

2017-12-14 Thread Kevin Funk

kfunk added reviewers: mlaurent, dfaure.

REPOSITORY
  R216 Syntax Highlighting

REVISION DETAIL
  https://phabricator.kde.org/D9321

To: kfunk, mlaurent, dfaure
Cc: #frameworks

D9321: Fix another CMake CMP0071 warning

2017-12-14 Thread Kevin Funk

kfunk created this revision.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: Frameworks.

REVISION SUMMARY
CMake Warning (dev) in data/CMakeLists.txt:
  Policy CMP0071 is not set: Let AUTOMOC and AUTOUIC process GENERATED 
files.
  Run "cmake --help-policy CMP0071" for policy details.  Use the 
cmake_policy
  command to set the policy and suppress this warning.

  For compatibility, CMake is excluding the GENERATED source file(s):


"/home/kfunk/devel/src/kf5/syntax-highlighting/build/data/qrc_syntax-data.cpp"

REPOSITORY
  R216 Syntax Highlighting

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D9321

AFFECTED FILES
  data/CMakeLists.txt

To: kfunk
Cc: #frameworks

D9277: Remove cmake warning about generating moc file

2017-12-13 Thread Kevin Funk

kfunk accepted this revision.
kfunk added a comment.
This revision is now accepted and ready to land.


  Yes. Makes sense to me.

INLINE COMMENTS

> KF5ConfigMacros.cmake:98
>  
> +   set_property(SOURCE ${_header_FILE} PROPERTY SKIP_AUTOMOC TRUE)  # 
> don't run automoc on this file
> +   set_property(SOURCE ${_src_FILE} PROPERTY SKIP_AUTOMOC TRUE)  # don't 
> run automoc on this file

Use `set_source_files_properties(...)`  instead, cf. 
https://phabricator.kde.org/R249:6e3b708435cc7747ea83a3afea4647c2d48b

REPOSITORY
  R237 KConfig

BRANCH
  remove_cmake_warning

REVISION DETAIL
  https://phabricator.kde.org/D9277

To: mlaurent, kfunk, dfaure
Cc: mpyne, apol, aacid, #frameworks

D9299: Introduce INSTALL_PREFIX_SCRIPT to easily set up prefixes

2017-12-13 Thread Kevin Funk

kfunk added a comment.


  In https://phabricator.kde.org/D9299#179036, @cgiboudeaux wrote:
  
  > In https://phabricator.kde.org/D9299#179035, @cgiboudeaux wrote:
  >
  > > In https://phabricator.kde.org/D9299#179032, @kfunk wrote:
  > >
  > > > If we'd name this file somewhat less generic then it could be even 
installed by default, no?
  > > >
  > > > I had the scheme of  the QNX setup script in my mind: 
https://github.com/acklinr/qnx660/blob/master/qnx660-env.sh
  > > >
  > > > Thus: Maybe rename prefix.sh to say 'ecm-env.sh' and install the file 
by default?
  > >
  > >
  > > Files overwritting each others is a no-go for packagers. +1 for a less 
generic name however.
  >
  >
  > Adding files in /usr directly, will also be rejected.
  >
  > Now that I had a closer look: KDEInstallDirs only defines some paths, I 
don't think this change shall be part of this file. What about moving it to its 
own file and sourcing it instead ?
  
  
  I agree. KDEInstallDirs.cmake seems to be wrong location for this 
functionality.
  
  What I'm envisioning is a ecm-env.sh-like script which gets installed into 
`$PREFIX/bin` as soon as you install ECM.
  
  Pseudo-cmake code:
  
configure_file(${CMAKE_SOURCE_DIR}/ecm-env.sh 
${CMAKE_BINARY_DIR}/ecm-env.sh)
install(FILES ${CMAKE_BINARY_DIR}/ecm-env.sh DESTINATION ${BIN_INSTALL_DIR})

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D9299

To: apol, #frameworks, sitter
Cc: kfunk, bcooksley, ngraham, sitter, cgiboudeaux, #build_system

D9299: Introduce INSTALL_PREFIX_SCRIPT to easily set up prefixes

2017-12-13 Thread Kevin Funk

kfunk added inline comments.

INLINE COMMENTS

> sitter wrote in KDEInstallDirs.cmake:699
> From a style perspective, I'd suggest having the prefix.sh live somewhere in 
> the installed ECM tree and get copied, rather than maintained as a glorified 
> heredoc in the cmake code. That's just a suggestion though.

Definitely, +1.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D9299

To: apol, #frameworks, sitter
Cc: kfunk, bcooksley, ngraham, sitter, cgiboudeaux, #build_system

D9299: Introduce INSTALL_PREFIX_SCRIPT to easily set up prefixes

2017-12-13 Thread Kevin Funk

kfunk added a comment.


  If we'd name this file somewhat less generic then it could be even installed 
by default, no?
  
  I had the scheme of  the QNX setup script in my mind: 
https://github.com/acklinr/qnx660/blob/master/qnx660-env.sh
  
  Thus: Maybe rename prefix.sh to say 'ecm-env.sh' and install the file by 
default?

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D9299

To: apol, #frameworks, sitter
Cc: kfunk, bcooksley, ngraham, sitter, cgiboudeaux, #build_system

D9182: return nullptr -> return {} for QFlags

2017-12-05 Thread Kevin Funk

kfunk accepted this revision.
kfunk added a comment.
This revision is now accepted and ready to land.


  For the record, see comment https://phabricator.kde.org/D3987#174960

REPOSITORY
  R287 KImageFormats

REVISION DETAIL
  https://phabricator.kde.org/D9182

To: mkoller, kfunk, kossebau
Cc: #frameworks

D9118: ki18n cmake macros: Mark UIC-generated .h files to skip AUTOMOC by default

2017-12-04 Thread Kevin Funk

kfunk accepted this revision.
kfunk added a comment.
This revision is now accepted and ready to land.


  This shouldn't create any troubles with earlier CMake versions.
  
  Also note that we set the same properties unconditionally in newer Qt5 CMake 
macros: https://codereview.qt-project.org/#/c/207327

INLINE COMMENTS

> KF5I18NMacros.cmake:56
>)
> +  set_property(SOURCE ${_header} PROPERTY SKIP_AUTOMOC ON)
>list(APPEND ${_sources} ${_header})

`set_source_files_properties(${_header} ...)` is probably the more canonical 
way to do this.

REPOSITORY
  R249 KI18n

REVISION DETAIL
  https://phabricator.kde.org/D9118

To: mpyne, #frameworks, #build_system, #localization, kfunk
Cc: aacid

D3987: Use nullptr in all Frameworks (just diff in KIO shown here)

2017-12-04 Thread Kevin Funk

kfunk added a comment.


  In https://phabricator.kde.org/D3987#174960, @mkoller wrote:
  
  > I just found out that this change introduced wrong replacements.
  
  
  It did not, the changes are correct, though they're probably not what 
everyone expects.
  
  Please read through the comments in this Diff, this issue has been discussed 
in length already.
  
  tl;dr: Proposed resolution of this "problem": I'd replace `return nullptr` in 
your cases with `return {}` or just leave it as-is.
  
  > The changes in frameworks/kimageformats/src/imageformats like the following 
are incorrect, since the return type is not a pointer but an enum flags type, 
so it should still be 0, not nullptr.
  > 
  > @@ -337,10 +337,10 @@ QImageIOPlugin::Capabilities 
EPSPlugin::capabilities(QIODevice *device, const QB
  > 
  >   return Capabilities(CanRead | CanWrite);
  >   }
  >   if (!format.isEmpty()) {
  > 
  > - return 0; +return nullptr; } if (!device->isOpen()) {
  > - return 0; +return nullptr; }
  > 
  >   Capabilities cap;

REPOSITORY
  R280 Prison

REVISION DETAIL
  https://phabricator.kde.org/D3987

To: kfunk, #frameworks, dfaure, kossebau
Cc: mkoller, skelly, kossebau, dfaure, graesslin

D8979: ecm_add_test: Use proper path sep on Windows

2017-11-24 Thread Kevin Funk

This revision was automatically updated to reflect the committed changes.
Closed by commit R240:64915e0291cd: ecm_add_test: Use proper path sep on 
Windows (authored by kfunk).

REPOSITORY
  R240 Extra CMake Modules

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8979?vs=22863=22869

REVISION DETAIL
  https://phabricator.kde.org/D8979

AFFECTED FILES
  modules/ECMAddTests.cmake

To: kfunk, dfaure, cgiboudeaux, bcooksley
Cc: #windows, #frameworks, #build_system

D8979: ecm_add_test: Use proper path sep on Windows

2017-11-23 Thread Kevin Funk

kfunk added a reviewer: cgiboudeaux.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D8979

To: kfunk, dfaure, cgiboudeaux
Cc: #frameworks, #build_system

D8979: ecm_add_test: Use proper path sep on Windows

2017-11-23 Thread Kevin Funk

kfunk added a reviewer: bcooksley.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D8979

To: kfunk, dfaure, cgiboudeaux, bcooksley
Cc: #windows, #frameworks, #build_system

D8979: ecm_add_test: Use proper path sep on Windows

2017-11-23 Thread Kevin Funk

kfunk added a subscriber: Windows.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D8979

To: kfunk, dfaure, cgiboudeaux
Cc: #windows, #frameworks, #build_system

D8979: ecm_add_test: Use proper path sep on Windows

2017-11-23 Thread Kevin Funk

kfunk created this revision.
kfunk added a reviewer: dfaure.
Restricted Application added projects: Frameworks, Build System.
Restricted Application added subscribers: Build System, Frameworks.

REVISION SUMMARY
  We need to separate paths in environment variables
  with a semincolon (;) on Windows.

REPOSITORY
  R240 Extra CMake Modules

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D8979

AFFECTED FILES
  modules/ECMAddTests.cmake

To: kfunk, dfaure
Cc: #frameworks, #build_system

D8976: Remove redundant map look-ups

2017-11-23 Thread Kevin Funk

kfunk accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R282 NetworkManagerQt

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D8976

To: apol, #frameworks, jgrulich, kfunk

D8923: Offer QWindow API for KJobWidgets:: decorators

2017-11-21 Thread Kevin Funk

kfunk added inline comments.

INLINE COMMENTS

> apol wrote in kjobwidgets.cpp:67
> Note I didn't change any code, I just moved it around.
> 
> Removing the cast still seems to work, so I'll do that.

> Note I didn't change any code, I just moved it around.

Whoops, sorry. Didn't notice.

REPOSITORY
  R288 KJobWidgets

REVISION DETAIL
  https://phabricator.kde.org/D8923

To: apol, #frameworks
Cc: kfunk

D8923: Offer QWindow API for KJobWidgets:: decorators

2017-11-21 Thread Kevin Funk

kfunk added inline comments.

INLINE COMMENTS

> kjobwidgets.cpp:67
> +if (window) {
> +job->setProperty("window-id", 
> QVariant::fromValue(qptrdiff(window->winId(;
> +}

`WId` is a `quintptr`: `src/gui/kernel/qwindowdefs.h:99:typedef 
QT_PREPEND_NAMESPACE(quintptr) WId;`

Besides: `QVariant::fromValue(window->winId())` should work just fine, or?

REPOSITORY
  R288 KJobWidgets

REVISION DETAIL
  https://phabricator.kde.org/D8923

To: apol, #frameworks
Cc: kfunk

Re: How is symbol visibility set in KF5 and KDE?

2017-11-15 Thread Kevin Funk

On Wednesday, 15 November 2017 15:54:17 CET Shaheed Haque wrote:
> Hi all,
> 
> I just realised that the Python binding effort is not setting the default
> visibility for symbols using the -fvisibility=xxx option when processing
> the header files [1]. Of course I can see the export macros set by the
> likes of attica_exports.h, but I don't see where the compiler default is
> set. Can somebody kindly point that out?

It's configured in extra-cmake-modules.git:

kde-modules/KDECompilerSettings.cmake
223:set(CMAKE_C_VISIBILITY_PRESET hidden)
224:set(CMAKE_CXX_VISIBILITY_PRESET hidden)
225:set(CMAKE_VISIBILITY_INLINES_HIDDEN 1)

Regards,
Kevin

> Thanks, Shaheed
> 
> [1] I'm also a bit mystified by the fact that I am deliberately querying
> CMake for the COMPILE_FLAGS to use, but I have not seen -fvisibility
> anywhere...


-- 
Kevin Funk | kf...@kde.org | http://kfunk.org

signature.asc
Description: This is a digitally signed message part.

D8810: Do not look for kioslave binary in applicationDirPath (#386859)

2017-11-14 Thread Kevin Funk

kfunk requested changes to this revision.
kfunk added a comment.
This revision now requires changes to proceed.


  Just chiming in here, without having a final solution for your problem: This 
search path has been added for a reason -- to find a kioslave.exe next to the 
application binary in application bundles on macOS/Windows.
  
  See:
  https://git.reviewboard.kde.org/r/125778/
  
  CC'ing Christoph.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8810

To: kkofler, #frameworks, kfunk
Cc: kfunk

D8810: Do not look for kioslave binary in applicationDirPath (#386859)

2017-11-14 Thread Kevin Funk

kfunk added a reviewer: cullmann.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8810

To: kkofler, #frameworks, kfunk, cullmann
Cc: kfunk

D8577: Fix: Missing dependencies for ktexeditor autotests

2017-11-06 Thread Kevin Funk

kfunk added a comment.


  In https://phabricator.kde.org/D8577#164815, @cullmann wrote:
  
  > Could you just remove the problematic includes and try to compile without 
the dependency, IMHO I see no use of QtScript stuff.
  
  
  Done with: 
https://phabricator.kde.org/R39:631b1447c97f1d81de2e81d2556474f0bc3109dd

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D8577

To: hgoebel, #ktexteditor, dfaure
Cc: kfunk, cullmann, dhaumann, #frameworks, kevinapavew, demsking, sars

D8544: KTextEditor : avoiding QML crashes

2017-11-02 Thread Kevin Funk

kfunk added inline comments.

INLINE COMMENTS

> kateglobal.cpp:104
> +// disable the QML JIT compiler as a protection against an unknown bug
> +// in Qt's V4 engine which can provoke a crash in certain of our scripts.
> +qputenv("QV4_FORCE_INTERPRETER", QByteArrayLiteral("1"));

Please add a reference to the bug reports (Qt JIRA, KDE Bugzilla) here.

> kateglobal.cpp:106
> +qputenv("QV4_FORCE_INTERPRETER", QByteArrayLiteral("1"));
> +qCDebug(LOG_KTE) << "QV4_FORCE_INTERPRETER set to" << 
> qgetenv("QV4_FORCE_INTERPRETER");
> +#endif

Minor: "... set to 1" would be sufficient. You're setting it yourself right 
before, so you can presume it is set (=> no need for `qgetenv(...)`.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D8544

To: rjvbb, #frameworks
Cc: kfunk, dhaumann, cullmann, kde-frameworks-devel, kevinapavew, demsking, 
head7, sars

D8437: KWidgetsAddons : more compact password dialog

2017-10-27 Thread Kevin Funk

kfunk accepted this revision.
kfunk added a comment.
This revision is now accepted and ready to land.


  Looks like it :)

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D8437

To: rjvbb, #frameworks, kfunk, ngraham
Cc: cfeck, ngraham

D8437: KWidgetsAddons : more compact password dialog

2017-10-24 Thread Kevin Funk

kfunk requested changes to this revision.
This revision now requires changes to proceed.

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D8437

To: rjvbb, #frameworks, kfunk
Cc: cfeck, ngraham

D7401: Solid/Mac : fleshing out the skeleton IOKit backend (WIP)

2017-10-24 Thread Kevin Funk

kfunk requested changes to this revision.
kfunk added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> iokitdevice.cpp:307
> +default:
> +vendor = QString();
> +break;

Why not `return` here as well? For consistency.

You can end the function with

  Q_UNREACHABLE();
  return {};

Very common pattern.

> iokitdevice.cpp:463
>  {
> -return (type == Solid::DeviceInterface::GenericInterface
> -|| type == d->type);
> +bool ret = false;
> +switch (type) {

Returning inside the case statements would make this code clearer as well.

> iokitdeviceinterface.h:51
>  IOKitDevice *m_device;
> +IOKitDevice *copy;
>  };

`m_` prefix missing. Would call it `m_deviceCopy`.

> rjvbb wrote in iokitstorage.cpp:36
> Did you check that these are indeed pointers? ;)

Yes. And they are, no?

> rjvbb wrote in iokitstorage.cpp:75
> I don't get it, sorry, can you explain in more words how you'd want to see 
> this changed? If I remove this extra ctor, I can no longer call 
> `IOKitStorage(this).vendor()` in `IOKitDevice::vendor()` without extra code 
> that's also going to add noise.
> 
> I get a warning when I remove the const attribute from 
> `IOKitDevice::vendor()`, which suggests that I'd no longer be reimplementing 
> a virtual method but adding a method instead.
> 
> All these "extra" ctors hand off the pointer to a "const this" to 
> `DeviceInterface` which finally makes a deep copy. I find that cleaner than 
> creating a deep copy of `this` everywhere needed and ensuring it gets 
> deallocated (even via QPointers).
> Unusual doesn't mean wrong (we're on Mac here, after all ^^)

Okay, well, leave it like that.

I'm running out of time to properly explain how I'd envision this code to be 
like in a perfect world.

> iokitstorage.cpp:73
> +
> +const IOKitDevice *m_device;
> +DASessionRef daSession;

Inconsistent member naming. Some with `m_` prefix, some without. Choose one 
style. Private classes' members usually live without the `m_` prefix, but we 
don't mind them being around (in KDE land)

> iokitstorage.h:43
> +
> +const QString vendor();
> +const QString product();

Remove `const` from return type, but make the method `const`.

> iokitstorageaccess.cpp:56
> +
> +const IOKitDevice *m_device;
> +DASessionRef daSession;

Dito, inconsistent member naming.

> iokitvolume.cpp:70
> +
> +const IOKitDevice *m_device;
> +DASessionRef daSession;

Dito, inconsistent member naming.

And given that I've noticed that three times now, this again leads to the 
conclusion this is very repetitive code amongst . 
`{IOKitStorageAccess,IOKitVolume,IOKitStorage}::Private`.

Maybe there should be helper class for accessing a `CFDictionary` instead which 
all these classes use?

I'm not trying to piss you off René, but this is slightly sloppy coding which 
easy for the initial writer to do, but will bite us any time in the future when 
someone unfamiliar with the code needs to do fixes to this code and potentially 
fixes only one copy of these snippets. Please think about your architecture 
more carefully.

> iokitvolume.h:45
> +
> +virtual bool isIgnored() const Q_DECL_OVERRIDE;
> +virtual Solid::StorageVolume::UsageType usage() const Q_DECL_OVERRIDE;

No `virtual` needed if there's already a `Q_DECL_OVERRIDE` or `override`.

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D7401

To: rjvbb, #frameworks, kfunk
Cc: kfunk, anthonyfieroni, cgilles, kde-mac

D7401: Solid/Mac : fleshing out the skeleton IOKit backend (WIP)

2017-10-23 Thread Kevin Funk

kfunk added inline comments.

INLINE COMMENTS

> iokitopticaldrive.cpp:27
> +
> +#ifdef EJECT_USING_DISKARBITRATION
> +// for QCFType:

Where's that defined via the build system?

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D7401

To: rjvbb, #frameworks, kfunk
Cc: kfunk, anthonyfieroni, cgilles, kde-mac

D7401: Solid/Mac : fleshing out the skeleton IOKit backend (WIP)

2017-10-23 Thread Kevin Funk

kfunk requested changes to this revision.
kfunk added a comment.
This revision now requires changes to proceed.


  This definitely needs another revision. Please fix the outstanding issues.

INLINE COMMENTS

> iokitblock.h:43
> +
> +virtual int deviceMajor() const;
> +virtual int deviceMinor() const;

Here and below: Missing `Q_DECL_OVERRIDE`

> iokitdevice.cpp:151
> +size_t size = 0;
> +if (sysctlbyname("hw.model", NULL, , NULL, 0) == 0 && size > 0) {
> +model = new char [size];

Use `nullptr` everywhere

> iokitdevice.cpp:154
> +if (sysctlbyname("hw.model", model, , NULL, 0) == 0) {
> +qModel= QLatin1String(model);
> +}

Style: Use `qModel = ...`

> iokitdevice.cpp:156
> +}
> +delete model;
> +}

`new`/`delete` mismatch. Use `delete[]`

> iokitdevice.cpp:167
> +{
> +type << Solid::DeviceInterface::Unknown;
> +}

Initialize at class member decl

> iokitdevice.cpp:171
> +{
> +if (parent) {
> +delete parent;

That's *very* odd style. Why does a private class delete its public 
counterpart? I've never seen this.

> iokitdevice.cpp:197
> +type = typesFromEntry(entry, properties, mainType);
> +// if (udi.contains(QStringLiteral("IOBD")) || 
> udi.contains(QStringLiteral("BD PX"))) {
> +// qWarning() << "Solid: BlueRay entry" << entry << "mainType=" << 
> mainType << "typeList:" << type

Don't leave commented code around. Either enable this code paths properly via 
categorized logging or remove it altogether.

> iokitopticaldrive.cpp:211
> +}
> +delete ioDVDServices;
> +d = new Private(device, devCharMap);

Create on `ioDVDServices` on the stack to begin with?

> iokitopticaldrive.cpp:333
> +{
> +QList speeds;
> +return speeds;

`return {}`

> iokitopticaldrive.cpp:354
> +return true;
> +} else {
> +emit ejectDone(Solid::ErrorType::OperationFailed, QVariant(), 
> m_device->udi());

Coding style: Would turn around this two branches (and remove the else part) to 
make the intended meaning more clear:

Pseudo code:

  if (errorCase) {
return ...;
  }
  
  // normal case, code flow continues
  return ...;

That's usually the common style

> iokitprocessor.cpp:84
> +
> +const QString Processor::vendor()
> +{

Remove `const`

> iokitprocessor.cpp:87
> +QString qVendor = QString();
> +char *vendor = NULL;
> +size_t size = 0;

Way too much error prone `new`/`delete` logic on naked char arrays. Every 
invocation here is wrong (causing undefined behavior) due to the 
`new[]`/delete` mismatch mentioned above.

Factor that out reading into a `QString` into a separate function, cf. 
https://github.com/trueos/lumina/blob/master/src-qt5/core/libLumina/LuminaOS-DragonFly.cpp#L29
 and use it everywhere instead.

> iokitprocessor.cpp:99
>  
> +const QString Processor::product()
> +{

Remove `const`

> iokitstorage.cpp:36
> +{
> +daRef = 0;
> +daDict = 0;

Here and one line below: Could be initialized at decl again.

> iokitstorage.cpp:36
> +{
> +daRef = 0;
> +daDict = 0;

`nullptr`

> iokitstorage.cpp:51
> +CFRelease(daDict);
> +daDict = 0;
> +}

`nullptr`, etc. pp.

> iokitstorage.cpp:75
> +
> +IOKitStorage::IOKitStorage(const IOKitDevice *device)
> +: Block(device)

Please just remove the constructors taking a `const IOKitDevice *device` and 
adapt uses (just use `IOKitDevice *device` everywhere). It's unusual for 
pointer types to be `const` in such contexts. It just adds noise.

> iokitstorageaccess.cpp:41
> +} else {
> +daRef = 0;
> +}

Here and below: `nullptr`

> iokitstorageaccess.cpp:91
> +{
> +// mount points can change (but can they between invocations of 
> filePath()?)
> +if (d->daRef) {

Early-return would reduce the indentation level here [1].

  if (errorCase)
// return early
return {};
  }
  
  // normal case
  // ...

[1] 
https://softwareengineering.stackexchange.com/questions/18454/should-i-return-from-a-function-early-or-use-an-if-statement

> iokitstorageaccess.cpp:132
> +return false;
> +} else {
> +// TODO?

Coding style: Eliminate `else` branch, just `return in function-level scope.

> iokitstorageaccess.cpp:142
> +return false;
> +} else {
> +// TODO?

Dito

> iokitvolume.cpp:42
> +} else {
> +daRef = 0;
> +}

Here and below: `nullptr`

> iokitvolume.cpp:121
> +if (dict) {
> +// qWarning() << Q_FUNC_INFO;
> +// CFShow(dict);

Remove commented code.

> iokitvolume.cpp:177
> +if (d->daRef) {
> +CFDictionaryRef dict = DADiskCopyDescription(d->daRef);
> +if (dict) {

That's a lot of copy-pasted code amongst `fsType`, `label`, `vendor`, `product, 
`description`. I'm sure that can be done better.

> iokitvolume.h:45
> +
> +virtual bool isIgnored() const;
> +virtual

Re: What removes protocoltojson with make install in kio?

2017-10-22 Thread Kevin Funk

On Sunday, 22 October 2017 17:29:17 CEST Mark Gaiser wrote:
> Hi,
> 
> My cmake line:
> cmake ../../kio -DCMAKE_BUILD_TYPE=RelWithDebInfo
> -DCMAKE_INSTALL_PREFIX=~/GitProjects/builds/kio -DBUILD_TESTING=OFF
> -DBUILD_QCH=ON -DKDE_INSTALL_LIBDIR=lib -DKDE_INSTALL_LIBEXECDIR=lib
> 
> make runs just fine
> 
> After that my bin folder looks like this [1], the relevant part (ls -l
> bin/):
> -rwxr-xr-x 1 mark users   645616 okt 22 17:21 protocoltojson
> 
> But running make install ends with:
>  -- Up-to-date:
> /home/mark/GitProjects/builds/kio/include/KF5/KIOCore/KSSLSettings
> -- Up-to-date:
> /home/mark/GitProjects/builds/kio/include/KF5/KIOCore/ksslsettings.h
> -- Up-to-date:
> /home/mark/GitProjects/builds/kio/include/KF5/KIOCore/kiocore_export.h
> -- Up-to-date:
> /home/mark/GitProjects/builds/kio/etc/xdg/accept-languages.codes
> -- Up-to-date:
> /home/mark/GitProjects/builds/kio/mkspecs/modules/qt_KIOCore.pri
> CMake Error at src/protocoltojson/cmake_install.cmake:42 (file):
>   file INSTALL cannot find
>   "/home/mark/GitProjects/builds/kio/bin/protocoltojson".
> Call Stack (most recent call first):
>   src/cmake_install.cmake:46 (include)
>   cmake_install.cmake:72 (include)
> 
> 
> make: *** [Makefile:74: install] Error 1

Is /home/mark/GitProjects/builds/kio/ both your build & install folder?

If yes, don't do this, that won't work. Keep separate directories -- install 
into a different prefix.

Regards,
Kevin

> and protocoltojson is indeed gone from the bin folder.
> Running find . -iname "protocoltojson"
> only gives me the build folder for it, the binary is actually gone...
> 
> Am i doing anything wrong?
> 
> Cheers,
> Mark
> 
> 
> [1] https://paste.kde.org/ptfhyntme


-- 
Kevin Funk | kf...@kde.org | http://kfunk.org

signature.asc
Description: This is a digitally signed message part.

Re: Adding build info attribute to KAboutData to improve bug report data?

2017-10-16 Thread Kevin Funk

On Monday, 16 October 2017 07:10:29 CEST Luca Beltrame wrote:
> Il giorno Sun, 08 Oct 2017 19:45:00 +0200
> 
> Kevin Funk <kf...@kde.org> ha scritto:
> > Heya,
> > 
> > The KDevelop team is currently discussing a patch which adds
> > 
> > git-describe like information to the version string in KAboutData:
> >   https://phabricator.kde.org/D8158
> >   (don't bother reading through the comments...)
> 
> Is it added at compile time? I didn't read through the diff yet, but
> how would you define a "development" build?

> This is to make sure that no such things are put in released versions,
> which would cause problems with reproducible builds.

See: 
  https://phabricator.kde.org/D8158

We'd like to add version information such as:
  "5.1.80-24-g6e3cfe2421"

If Git's HEAD is checked out at a tag (i.e. v5.1.2), then I wouldn't set that 
attribute at all.

Though even "5.1.80-24-g6e3cfe2421" should be fine when striving for 
reproducible builds. The Git SHA1 doesn't change after all, if you haven't 
modified the Git history.

I wouldn't put the 'build time' or similar things there, to begin with (which 
would cause problems with reproducible builds, obviously).

tl;dr: It's up to the individual project how to set that build information 
attribute. Probably makes sense to add little hints about reproducible builds 
to that attribute's API documentation.

Regards,
Kevin

-- 
Kevin Funk | kf...@kde.org | http://kfunk.org

signature.asc
Description: This is a digitally signed message part.

D8314: Fix deprecation warning

2017-10-16 Thread Kevin Funk

This revision was automatically updated to reflect the committed changes.
Closed by commit R39:68d42865076e: Fix deprecation warning (authored by kfunk).

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8314?vs=20813=20841

REVISION DETAIL
  https://phabricator.kde.org/D8314

AFFECTED FILES
  src/dialogs/katedialogs.cpp

To: kfunk, apol
Cc: #frameworks, kevinapavew, cullmann, sars, dhaumann

D8315: CMake cleanup: Don't hardcode -std=c++0x

2017-10-16 Thread Kevin Funk

This revision was automatically updated to reflect the committed changes.
Closed by commit R279:265b9f9f3120: CMake cleanup: Dont hardcode 
-std=c++0x (authored by kfunk).

REPOSITORY
  R279 ThreadWeaver

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8315?vs=20814=20840

REVISION DETAIL
  https://phabricator.kde.org/D8315

AFFECTED FILES
  CMakeLists.txt
  benchmarks/CMakeLists.txt
  examples/CMakeLists.txt

To: kfunk, apol
Cc: #frameworks

FYI: Added .arcconfig files to all frameworks

2017-10-15 Thread Kevin Funk

Heya,

just pushed an .arcconfig file to all framework repositories. There were still 
lots of frameworks without them.

Example commit:
  https://cgit.kde.org/qqc2-desktop-style.git/commit/?
id=146e370704938e6cab6278a5e203f3ef2cf93946

Regards,
Kevin

-- 
Kevin Funk | kf...@kde.org | http://kfunk.org

signature.asc
Description: This is a digitally signed message part.

D8315: CMake cleanup: Don't hardcode -std=c++0x

2017-10-15 Thread Kevin Funk

kfunk created this revision.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: Frameworks.

REVISION SUMMARY
  This is both set by ECM & Qt5 CMake config files

REPOSITORY
  R279 ThreadWeaver

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D8315

AFFECTED FILES
  CMakeLists.txt
  benchmarks/CMakeLists.txt
  examples/CMakeLists.txt

To: kfunk
Cc: #frameworks

D8314: Fix deprecation warning

2017-10-15 Thread Kevin Funk

kfunk created this revision.
Restricted Application added projects: Kate, Frameworks.
Restricted Application added a subscriber: Frameworks.

REVISION SUMMARY
  Warning:
  
/home/kfunk/devel/src/kf5/ktexteditor/src/dialogs/katedialogs.cpp:1455:11:
warning: 'runUrl' is deprecated [-Wdeprecated-declarations]
KRun::runUrl(url, QStringLiteral("text/x-patch"), nullptr, true);
  ^
/home/kfunk/devel/install/kf5/include/KF5/KIOWidgets/krun.h:302:17:
note: 'runUrl' has been explicitly marked deprecated here
static bool KIOWIDGETS_DEPRECATED runUrl(const QUrl , const QString 
, QWidget *window,
^

REPOSITORY
  R39 KTextEditor

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D8314

AFFECTED FILES
  src/dialogs/katedialogs.cpp

To: kfunk
Cc: #frameworks, kevinapavew, cullmann, sars, dhaumann

Re: Adding build info attribute to KAboutData to improve bug report data?

2017-10-15 Thread Kevin Funk

On Sunday, 8 October 2017 19:45:00 CEST Kevin Funk wrote:
> Heya,
> 
> The KDevelop team is currently discussing a patch which adds git-describe
> like information to the version string in KAboutData:
>   https://phabricator.kde.org/D8158
>   (don't bother reading through the comments...)
> 
> Problem:
> 
> When KAboutData's version is set to something like "5.1.80-24-g262fb4cb9f"
> and when attempting to report a bug via KDevelop -> Help -> Report Bug,
> bugs.kde.org will no longer pre-select the correct version in the version
> list. Even worse: The version information is lost completely if the user
> does not copy it from the dialog.
> 
> Idea:
> Let's add a 'build info' attribute to KAboutData which may contain custom
> data filled by a project to provide more information about a specific
> build.
> 
> Then, for instance in KDevelop's main.cpp:
> ```
> KAboutData data(...);
> data.setBuildInfo("Version: " KDEVELOP_FULL_VERSION_STRING);
> ```
> ... with KDEVELOP_FULL_VERSION_STRING being "5.1.80-24-g262fb4cb9f"

@KCoreAddons maintainers: Do you think that makes sense? In that case I'd 
create a patch which adds such API to KAboutData.

Regards,
Kevin

> And then during the bug report process, just amend the url used to direct
> the user to bugs.kde.org:
>   https://bugs.kde.org/enter_bug.cgi?
> format=guided=kdevelop=5.1.80=Build%20information:
> %0AVersion:%205.1.80-24-g262fb4cb9f
> 
> Would require changes in kcoreaddons (KAboutData) and kxmlgui (KBugReport)
> afaics.
> 
> Comments? There are a couple of other KDE projects (thinking of krita, kexi
> -- which also use Git info) which could benefit from this.
> 
> This is also extensible, one could add more content to the build info
> attribute which would be sent over to bugs.kde.org.
> 
> Regards,
> Kevin


-- 
Kevin Funk | kf...@kde.org | http://kfunk.org

signature.asc
Description: This is a digitally signed message part.

Re: concise instructions how to use sub...@bugs.kde.org?

2017-10-15 Thread Kevin Funk

On Sunday, 15 October 2017 12:07:57 CEST René J.V. Bertin wrote:
> Hi,
> 
> I'm looking for some concise instructions how to submit reports to BKO via
> the bug reporting address (something more straightforward than
> retro-engineering DrKonqi ;)).
> 
> I've come across something (www.plcrashreporter.org) that could replace
> DrKonqi on Mac, and which might eventually be implemented instead of the
> code in whatever framework invokes DrKonqi.

Why don't you finish / bump people on
  https://phabricator.kde.org/D4929

... which  would be the more straight-forward and sustainable variant? I 
thought this is working in general?

Why do we need diverge again (i.e. introducing a tool where no-one is familiar 
with) instead of completing existing work?

Thanks,
Kevin

> Thank,
> René

-- 
Kevin Funk | kf...@kde.org | http://kfunk.org

signature.asc
Description: This is a digitally signed message part.

D8296: Use Ctrl+, as the standard shortcut for "Configure "

2017-10-15 Thread Kevin Funk

kfunk added a comment.


  In KDevelop, this is also a really important shortcut for navigating through 
the source. `Ctrl+,` for jumping to definition, `Ctrl+.` for jumping to 
declaration. /me wonders if one really needs a shortcut for configuring 
shortcuts at all.
  
  
https://lxr.kde.org/search?_filestring=&_string=Qt%3A%3ACTRL.*Qt%3A%3AKey_Comma 
lists the usages (=> Digikam, KDevelop, KMail)

REPOSITORY
  R237 KConfig

REVISION DETAIL
  https://phabricator.kde.org/D8296

To: ngraham, #frameworks, #vdg
Cc: kfunk, marten, graesslin, broulik, #frameworks

D8264: Fix build of breeze-icons on the Android CI

2017-10-12 Thread Kevin Funk

kfunk accepted this revision.
kfunk added a comment.
This revision is now accepted and ready to land.


  That comment could be a bit more verbose and better placed (i.e. move it next 
to `set(BINARY_ICONS_RESOURCE_OPTION_DEFAULT OFF)` and make it something along 
"When cross-compiling qrcAlias would be compiled against Embedded Qt, thus 
disable ... -- TODO: should compile ..."
  
  But okay :)

REPOSITORY
  R266 Breeze Icons

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D8264

To: apol, #frameworks, #plasma, kfunk
Cc: kfunk, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D8264: Fix build of breeze-icons on the Android CI

2017-10-12 Thread Kevin Funk

kfunk requested changes to this revision.
kfunk added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> CMakeLists.txt:23
> +if (CMAKE_CROSSCOMPILING)
> +set(GENERATE_ICONS_DEFAULT OFF)
> +else()

Better: `GENERATE_ICONS_DEFAULT` -> `BINARY_ICONS_RESOURCE_OPTION_DEFAULT` 
(consistent naming)

Also add some comment why it's disabled when cross-compiling.

REPOSITORY
  R266 Breeze Icons

REVISION DETAIL
  https://phabricator.kde.org/D8264

To: apol, #frameworks, #plasma, kfunk
Cc: kfunk, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D8209: Use https for KDE urls

2017-10-08 Thread Kevin Funk

This revision was automatically updated to reflect the committed changes.
Closed by commit R263:1ee609564a8f: Use https for KDE urls (authored by kfunk).

REPOSITORY
  R263 KXmlGui

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8209?vs=20493=20504

REVISION DETAIL
  https://phabricator.kde.org/D8209

AFFECTED FILES
  make_kdepackages_updated.py
  src/kaboutapplicationdialog.cpp
  src/kaboutkdedialog_p.cpp
  src/kbugreport.cpp
  src/kxmlgui.xsd
  src/kxmlguiwindow.h
  tests/kbugreporttest.cpp

To: kfunk, apol
Cc: #frameworks

D8207: Use https for KDE urls

2017-10-08 Thread Kevin Funk

kfunk added a comment.


  In https://phabricator.kde.org/D8207#153510, @mpyne wrote:
  
  > I support the idea, but I think there's a few more spots where we can 
convert http:// to https://.  I ran `ag --only-matching --no-heading 
'http:.*k.*\.org'` in the kcoreaddons source directory, with the current 
version of this patch already applied, and got the following output:
  >
  >   src/lib/kaboutdata.h:811:http://;. "http://kate.kde.org
  
  
  Bad match -- this is just an apidocs example how to format the url. I left 
this out.
  
  I've now fixed all the http://kde.org urls as well. Didn't touch the others 
though -- those are out of our control.
  
  > Of these, the XML might be an issue (though I doubt it), and I'm not sure 
if koffice.org is still current.  But the autotest sample .desktop files can be 
changed, which will impact the kpluginmetadatatest also.
  > 
  > In the meantime I've verified that the current patch passes the testsuite 
on my machine as well.
  
  +1, same here.
  
  Pushed now.

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D8207

To: kfunk, apol
Cc: apol, mpyne, #frameworks

D8207: Use https for KDE urls

2017-10-08 Thread Kevin Funk

This revision was automatically updated to reflect the committed changes.
Closed by commit R244:3cc8b8f5ad26: Use https for KDE urls (authored by kfunk).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D8207?vs=20491=20502#toc

REPOSITORY
  R244 KCoreAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8207?vs=20491=20502

REVISION DETAIL
  https://phabricator.kde.org/D8207

AFFECTED FILES
  autotests/data/fakeplugin.desktop
  autotests/data/hiddenplugin.desktop
  autotests/data/twostepsparsetest.desktop
  autotests/kaboutdatatest.cpp
  autotests/kmacroexpandertest.cpp
  autotests/kpluginmetadatatest.cpp
  autotests/kstringhandlertest.cpp
  autotests/ktexttohtmltest.cpp
  autotests/kurlmimedatatest.cpp
  src/lib/kaboutdata.cpp
  src/lib/plugin/kpluginmetadata.h

To: kfunk, apol
Cc: apol, mpyne, #frameworks

D8209: Use https for KDE urls

2017-10-08 Thread Kevin Funk

kfunk created this revision.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: Frameworks.

REPOSITORY
  R263 KXmlGui

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D8209

AFFECTED FILES
  make_kdepackages_updated.py
  src/kaboutapplicationdialog.cpp
  src/kaboutkdedialog_p.cpp
  src/kbugreport.cpp
  src/kxmlgui.xsd
  src/kxmlguiwindow.h
  tests/kbugreporttest.cpp

To: kfunk
Cc: #frameworks

1 2 3 4 5 6 >

1 - 100 of 511 matches

Mail list logo