D5865: Add missing KDE_ENABLE_NAMED_OPERATORS function

2017-05-29 Thread Thiago Macieira 
thiago added a comment.


  In https://phabricator.kde.org/D5865#112747, @rjvbb wrote:
  
  > In https://phabricator.kde.org/D5865#112743, @thiago wrote:
  >
  > > Fix the source code.
  > >
  > > If certain third-party libraries can't compile under MSVC 2015 (or even 
2017!) with default compiler options, they don't deserve to be used. Blacklist 
them.
  >
  >
  > That may work for Qt but is not an acceptable attitude for ECM, IMHO - and 
MSVC is problematic enough to build KF5 code for other reasons not to jump 
through hoops for it. You cannot expect FOSS projects to avoid Boost because it 
uses less common feature of the standard, big ole bad M$ cannot be bothered to 
write a proper compiler. I'd be all for blacklisting such compilers, if 
anything had to be blacklisted.
  
  
  This is in a kde.org codereview of a macro with KDE in the name. I think it's 
acceptable. Don't waste time with code that doesn't try to be cross-platform. 
Just let it fail to compile.
  
  And submit bug reports to those libraries. It's very easy for them to fix.
  
  > The projects I'm aware of that use "incriminated" boost code do provide 
MSWin builds but using a properly standards-compliant compiler.
  > 
  > My proposal is thus to drop MSVC support in the new macro, printing a 
warning should be enough.
  
  That's a fine solution. If you're going to print a warning and you're serious 
about cross-platformness support, I suggest printing the warning for ALL 
compilers: "Warning: using C++ operator names is not compatible with MSVC prior 
to version 2017 (19.1). You should reconsider using this macro if your code is 
meant to be cross-platform".

REPOSITORY
  R240 Extra CMake Modules

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

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

D5865: Add missing KDE_ENABLE_NAMED_OPERATORS function

2017-05-29 Thread René J . V . Bertin 
rjvbb added a comment.


  In https://phabricator.kde.org/D5865#112743, @thiago wrote:
  
  > Fix the source code.
  >
  > If certain third-party libraries can't compile under MSVC 2015 (or even 
2017!) with default compiler options, they don't deserve to be used. Blacklist 
them.
  
  
  That may work for Qt but is not an acceptable attitude for ECM, IMHO - and 
MSVC is problematic enough to build KF5 code for other reasons not to jump 
through hoops for it. You cannot expect FOSS projects to avoid Boost because it 
uses less common feature of the standard, big ole bad M$ cannot be bothered to 
write a proper compiler. I'd be all for blacklisting such compilers, if 
anything had to be blacklisted.
  The projects I'm aware of that use "incriminated" boost code do provide MSWin 
builds but using a properly standards-compliant compiler.
  
  My proposal is thus to drop MSVC support in the new macro, printing a warning 
should be enough.

REPOSITORY
  R240 Extra CMake Modules

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

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

D5966: Fix race-condition in KRandom-seeding.

2017-05-29 Thread Albert Astals Cid 
aacid added inline comments.

INLINE COMMENTS

> dfaure wrote in krandomtest.cpp:168
> variable size array, which is not in the standard.
> https://stackoverflow.com/questions/1887097/why-arent-variable-length-arrays-part-of-the-c-standard
> 
> Make size static to fix it.

They are in C++11, no?

REPOSITORY
  R244 KCoreAddons

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

To: tfry, dfaure
Cc: aacid, mpyne, tfry, rjvbb, #frameworks

D5865: Add missing KDE_ENABLE_NAMED_OPERATORS function

2017-05-29 Thread Thiago Macieira 
thiago added a comment.


  In https://phabricator.kde.org/D5865#112737, @rjvbb wrote:
  
  > In https://phabricator.kde.org/D5865#112697, @thiago wrote:
  >
  > > Why are we spending time in named operators? It's much easier to fix the 
source code not to use them and then the problem goes away.
  >
  >
  > This is about adding a convenience macro for projects that have 
dependencies using named operators, like certain Boost modules.
  
  
  Fix the source code.
  
  If certain third-party libraries can't compile under MSVC 2015 (or even 
2017!) with default compiler options, they don't deserve to be used. Blacklist 
them.

REPOSITORY
  R240 Extra CMake Modules

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

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

D5865: Add missing KDE_ENABLE_NAMED_OPERATORS function

2017-05-29 Thread René J . V . Bertin 
rjvbb added a comment.


  In https://phabricator.kde.org/D5865#112697, @thiago wrote:
  
  > Why are we spending time in named operators? It's much easier to fix the 
source code not to use them and then the problem goes away.
  
  
  This is about adding a convenience macro for projects that have dependencies 
using named operators, like certain Boost modules.

INLINE COMMENTS

> KDECompilerSettings.cmake:216-218
>  if (CMAKE_C_COMPILER_ID STREQUAL "GNU" OR
>  CMAKE_C_COMPILER_ID MATCHES "Clang" OR
>  (CMAKE_C_COMPILER_ID STREQUAL "Intel" AND NOT WIN32))

Shouldn't this be checking for CMAKE_CXX_COMPILER_ID?

REPOSITORY
  R240 Extra CMake Modules

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

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

D5966: Fix race-condition in KRandom-seeding.

2017-05-29 Thread David Faure 
dfaure added inline comments.

INLINE COMMENTS

> krandomtest.cpp:168
> +const int size = 5;
> +KRandomTestThread* threads[size];
> +for (int i=0; i < size; ++i) {

variable size array, which is not in the standard.
https://stackoverflow.com/questions/1887097/why-arent-variable-length-arrays-part-of-the-c-standard

Make size static to fix it.

> krandomtest.cpp:178
> +}
> +// each thread should have returned a unique result
> +QVERIFY(results.size() == size);

The unittest doesn't check that at all. I guess it can't, reliably, but then 
what is the comment for?

> krandomtest.cpp:179
> +// each thread should have returned a unique result
> +QVERIFY(results.size() == size);
> +for (int i=0; i < size; ++i) {

QCOMPARE

REPOSITORY
  R244 KCoreAddons

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

To: tfry, dfaure
Cc: mpyne, tfry, rjvbb, #frameworks

D6017: Don't leak MimeData object

2017-05-29 Thread David Edmundson 
davidedmundson created this revision.
Restricted Application added projects: Plasma, Frameworks.
Restricted Application added subscribers: Frameworks, plasma-devel.

REVISION SUMMARY
  A DeclarativeDropArea creates a new DeclarativeDragDropEvent on every
  enter/move/leave event.
  
  The getter method for the mimeData property creates a new MimeData
  QObject wrapper, which then leaks.
  
  Use of the mimeData object outside of the event shouldn't be expected to
  work, and a quick grep couldn't find any usage.
  
  BUG: 380270

TEST PLAN
  Dragged some things

REPOSITORY
  R296 KDeclarative

BRANCH
  master

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

AFFECTED FILES
  src/qmlcontrols/draganddrop/DeclarativeDragDropEvent.cpp
  src/qmlcontrols/draganddrop/DeclarativeDragDropEvent.h

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

D5865: Add missing KDE_ENABLE_NAMED_OPERATORS function

2017-05-29 Thread Thiago Macieira 
thiago added a comment.


  In https://phabricator.kde.org/D5865#112679, @rjvbb wrote:
  
  > > That option only exists in MSVC 2017.
  >
  > Should we have deduce that from the docs I cite in the CMake comments? I'm 
willing to believe that I misread as far as support for named operators is 
concerned, but they do mention 2015 Update 3 specifically as the appearance of 
`/permissive-`.
  
  
  Documentation to the rescue:
  
  - MSVC 2015: https://msdn.microsoft.com/en-us/library/fwkeyyhe.aspx
  - MSVC 2017: 
https://docs.microsoft.com/en-us/cpp/build/reference/permissive-standards-conformance
  
  >> But that has nothing to do with the named keywords. And the problem is not 
the compiler, it's Microsoft's Windows SDK headers.
  > 
  > In what sense? Do they use reserved keywords or is the named operator 
support implemented in header files with macros or who knows what?
  
  Yes, and more. For example, MSVC 2013 had -Zs:strictStrings, but we couldn't 
turn it on because the headers would then fail to compile. This improved in 
MSVC 2015, so we turned it on.
  
  > And just to be clear, there is thus no reliable way to obtain support for 
named operators in MSVC, not even with v. 2017 (and foreseeable newer versions)?
  
  Why are we spending time in named operators? It's much easier to fix the 
source code not to use them and then the problem goes away.

REPOSITORY
  R240 Extra CMake Modules

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

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

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

2017-05-29 Thread Tobias C. Berner 
tcberner added a comment.


  Ok, I think I found the issue in QStorageInfo -- it calls getmntinfo() with 
`flags=0` [1] . Instead one of the valid arguments 1-4 [2].
  
  [1] 
https://github.com/qt/qtbase/blob/dev/src/corelib/io/qstorageinfo_unix.cpp#L199
  [2] 
https://svnweb.freebsd.org/base/head/sys/sys/mount.h?revision=318736=markup#l441

REPOSITORY
  R293 Baloo

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

To: tcberner, #freebsd
Cc: kfunk, #frameworks

D5966: Fix race-condition in KRandom-seeding.

2017-05-29 Thread René J . V . Bertin 
rjvbb added a comment.


  I have no official reference for Qt requiring C++11, but I'm pretty sure I've 
seen the remark made on a mailing list or one of their code review tickets. I 
would have guessed that was with 5.7 but it can just as well be 5.8 and later. 
A GCC >= 4.8 *probably* corresponds to "we need complete enough C++11".
  
  Supporting MSVC is a PITA: https://phabricator.kde.org/D5865

REPOSITORY
  R244 KCoreAddons

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

To: tfry, dfaure
Cc: mpyne, tfry, rjvbb, #frameworks

D5865: Add missing KDE_ENABLE_NAMED_OPERATORS function

2017-05-29 Thread René J . V . Bertin 
rjvbb added a comment.


  > That option only exists in MSVC 2017.
  
  Should we have deduce that from the docs I cite in the CMake comments? I'm 
willing to believe that I misread as far as support for named operators is 
concerned, but they do mention 2015 Update 3 specifically as the appearance of 
`/permissive-`.
  
  > But that has nothing to do with the named keywords. And the problem is not 
the compiler, it's Microsoft's Windows SDK headers.
  
  In what sense? Do they use reserved keywords or is the named operator support 
implemented in header files with macros or who knows what?
  
  And just to be clear, there is thus no reliable way to obtain support for 
named operators in MSVC, not even with v. 2017 (and foreseeable newer versions)?

REPOSITORY
  R240 Extra CMake Modules

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

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

D4911: add Baloo DBus signals for moved or removed files

2017-05-29 Thread Matthieu Gallien 
mgallien updated this revision to Diff 14953.
mgallien added a comment.


  add a new method to register applications wanting to watch moved files and
  define (via xml) the interface to implement for applications

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D4911?vs=12137=14953

BRANCH
  master

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

AFFECTED FILES
  autotests/unit/file/CMakeLists.txt
  autotests/unit/file/filewatchtest.cpp
  autotests/unit/file/metadatamovertest.cpp
  autotests/unit/file/metadatamovertest.h
  src/CMakeLists.txt
  src/engine/transaction.cpp
  src/engine/transaction.h
  src/file/CMakeLists.txt
  src/file/filewatch.cpp
  src/file/filewatch.h
  src/file/mainhub.cpp
  src/file/mainhub.h
  src/file/metadatamover.cpp
  src/file/metadatamover.h
  src/file/org.kde.BalooWatcherApplication.xml

To: mgallien, vhanda, dfaure
Cc: cullmann, apol, #frameworks

KDE CI: Frameworks kcoreaddons kf5-qt5 XenialQt5.7 - Build # 12 - Still Unstable!

2017-05-29 Thread no-reply 
BUILD UNSTABLE
 Build URL
https://build-sandbox.kde.org/job/Frameworks%20kcoreaddons%20kf5-qt5%20XenialQt5.7/12/
 Project:
Frameworks kcoreaddons kf5-qt5 XenialQt5.7
 Date of build:
Mon, 29 May 2017 18:22:08 +
 Build duration:
3 min 40 sec and counting
   JUnit Tests
  Name: (root) Failed: 4 test(s), Passed: 21 test(s), Skipped: 0 test(s), Total: 25 test(s)Failed: TestSuite.kdirwatch_fam_unittestFailed: TestSuite.kdirwatch_inotify_unittestFailed: TestSuite.kdirwatch_qfswatch_unittestFailed: TestSuite.kdirwatch_stat_unittest
   Cobertura Report
  
   Project Coverage Summary
  
   Name
  PackagesFilesClassesLinesConditionalsCobertura Coverage Report100%
(10/10)85%
(79/93)85%
(79/93)72%
(5934/8240)42%
(10641/25227)Coverage Breakdown by Package
Name
   FilesClassesLinesConditionalsautotests83%
(33/40)83%
(33/40)96%
(2567/2683)48%
(6526/13503)src.desktoptojson100%
(3/3)100%
(3/3)79%
(84/106)38%
(146/384)src.lib67%
(2/3)67%
(2/3)56%
(302/536)22%
(192/893)src.lib.caching100%
(2/2)100%
(2/2)45%
(352/787)18%
(195/1086)src.lib.io90%
(9/10)90%
(9/10)53%
(744/1397)26%
(837/3214)src.lib.jobs71%
(5/7)71%
(5/7)52%
(159/304)39%
(57/146)src.lib.plugin100%
(8/8)100%
(8/8)86%
(646/748)44%
(1035/2326)src.lib.randomness100%
(2/2)100%
(2/2)67%
(66/99)58%
(45/78)src.lib.text63%
(5/8)63%
(5/8)46%
(349/764)40%
(794/1981)src.lib.util100%
(10/10)100%
(10/10)81%
(665/816)50%
(814/1616)

build.log
Description: Binary data

D5966: Fix race-condition in KRandom-seeding.

2017-05-29 Thread Thomas Friedrichsmeier 
This revision was automatically updated to reflect the committed changes.
Closed by commit R244:26a262180155: Ensure proper per thread seeding in 
KRandom. (authored by tfry).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D5966?vs=14863=14952#toc

REPOSITORY
  R244 KCoreAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D5966?vs=14863=14952

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

AFFECTED FILES
  autotests/krandomtest.cpp
  src/lib/randomness/krandom.cpp

To: tfry, dfaure
Cc: mpyne, tfry, rjvbb, #frameworks

D5966: Fix race-condition in KRandom-seeding.

2017-05-29 Thread Michael Pyne 
mpyne added a comment.


  The changeset is fine to go in (once the duplicated semicolon is fixed).
  
  As for C++11, I'm not aware that Qt requires C++11 as a blanket policy.  
Instead they require specific compilers, and then we can use the C++11 features 
supported by that compiler.
  
  According to the Qt documentation 
, Qt 5.8 is 
the first Qt release to require at least GCC 4.8 across the board, on 
configurations using GCC.  I believe that was the first GCC where 
`thread_local` is fully supported.  Note that we do try to support MS Windows 
though, and MSVC as distributed with VS 2013 is the minimum requirement for 
both Qt 5.8 and current Qt development.  But MSVC doesn't fully support 
`thread_local` until VS 2015 
.  So 
it will probably be at least another couple of years before we can rely on that 
feature to be available.

INLINE COMMENTS

> krandom.cpp:46
>  unsigned int seed;
> -init = true;
> +initialized_threads.setLocalData(true);;
>  QFile urandom(QStringLiteral("/dev/urandom"));

There are two semicolons on this line, only one is needed. :)

REPOSITORY
  R244 KCoreAddons

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

To: tfry, dfaure
Cc: mpyne, tfry, rjvbb, #frameworks

D6016: Don't warn when attempting to remove a MainWindow container

2017-05-29 Thread Elvis Angelaccio 
elvisangelaccio added a comment.


  hmm, this is what I did in ark: 
https://phabricator.kde.org/R36:e932b113b2d9ee7b792c23057ea2e15f8ea4f9c5

REPOSITORY
  R263 KXmlGui

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

To: elvisangelaccio, dfaure
Cc: #frameworks

D6016: Don't warn when attempting to remove a MainWindow container

2017-05-29 Thread David Faure 
dfaure added a comment.


  Containers are menus and toolbars, how can we possibly get here with a 
mainwindow as container? The fix doesn't looks horrible but I'm curious as to 
why this never happened before, what does ark do that nobody else ever did? Got 
a bt?

REPOSITORY
  R263 KXmlGui

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

To: elvisangelaccio, dfaure
Cc: #frameworks

D6016: Don't warn when attempting to remove a MainWindow container

2017-05-29 Thread Elvis Angelaccio 
elvisangelaccio created this revision.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: Frameworks.

REVISION SUMMARY
  MainWindow is a valid element that can contain other elements. We
  shouldn't print a warning in this case.

TEST PLAN
  latest ark git master (which has a  child of ) no longer 
prints:
  
Unhandled container to remove :  MainWindow
  
  when the app is closed.

REPOSITORY
  R263 KXmlGui

BRANCH
  master

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

AFFECTED FILES
  src/kxmlguibuilder.cpp

To: elvisangelaccio, dfaure
Cc: #frameworks

D5983: Check error status after every PolKitAuthority usage

2017-05-29 Thread Sergey Kalinichev 
skalinichev added a comment.


  In https://phabricator.kde.org/D5983#112386, @aacid wrote:
  
  > Have you had these errors happen to you?
  >
  > How can one reproduce them?
  >
  > Or is it more of a "this should be the right code but i don't know how to 
trigger it" case?
  
  
  Yes, for me it happens all the time when I try to adjust screen brightness 
after switching between VT's.
  
  Here is the error that I get with my patch:
  KAuth::Polkit1Backend::actionStatus: Encountered error while checking action 
status, error code: 2 "GDBus.Error:org.freedesktop.PolicyKit1.Error.Failed: 
Action org.kde.powerdevil.discretegpuhelper.hasdualgpu is not registered"
  
  Sure one has to fix this error in the first place, but it's always a good 
thing to error-proof the code.

REPOSITORY
  R283 KAuth

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

To: skalinichev
Cc: aacid, #frameworks

D5865: Add missing KDE_ENABLE_NAMED_OPERATORS function

2017-05-29 Thread Thiago Macieira 
thiago added a comment.


  In https://phabricator.kde.org/D5865#112549, @rjvbb wrote:
  
  > If a compiler is problematic in general it may not be worth it trying to 
account for it in a macro, trying to coerce it into doing something it cannot 
handle. I don't think that's a reason not to implement the macro at all though. 
Even if we cannot figure out from what version onwards the /fpermissive- option 
works it could still be useful for cross-platform code that really needs named 
operator support to let the macro print a warning (or raise an error) when it 
is built with MSVC.
  
  
  That option only exists in MSVC 2017. Qt 5.9.1 will use it for its own build. 
So far, so good.
  
  But that has nothing to do with the named keywords. And the problem is not 
the compiler, it's Microsoft's Windows SDK headers.

REPOSITORY
  R240 Extra CMake Modules

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

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

D6014: Small improvements in IconItem

2017-05-29 Thread Aleix Pol Gonzalez 
This revision was automatically updated to reflect the committed changes.
Closed by commit R242:3b303d736317: Small improvements in IconItem (authored by 
apol).

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D6014?vs=14943=14949

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

AFFECTED FILES
  src/declarativeimports/core/iconitem.cpp
  src/plasma/svg.cpp

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

D6014: Small improvements in IconItem

2017-05-29 Thread Marco Martin 
mart accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  master

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

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

D6014: Small improvements in IconItem

2017-05-29 Thread Aleix Pol Gonzalez 
apol updated this revision to Diff 14943.
apol added a comment.


  Few additions

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D6014?vs=14939=14943

BRANCH
  master

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

AFFECTED FILES
  src/declarativeimports/core/iconitem.cpp
  src/plasma/svg.cpp

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

D5611: [RenameDialog] Enforce plain text format

2017-05-29 Thread Kai Uwe Broulik 
This revision was automatically updated to reflect the committed changes.
Closed by commit R241:6b7115ce7016: [RenameDialog] Enforce plain text format 
(authored by broulik).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D5611?vs=13859=14941

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

AFFECTED FILES
  src/widgets/renamedialog.cpp

To: broulik, kde-frameworks-devel, apol
Cc: #frameworks

D6014: Small improvements in IconItem

2017-05-29 Thread Aleix Pol Gonzalez 
apol created this revision.
Restricted Application added projects: Plasma, Frameworks.
Restricted Application added a subscriber: plasma-devel.

REVISION SUMMARY
  Don't construct a QUrl for every source strings. Check it's a file url
  first (which is the only kind of url we support at the moment)
  When extracting the path, use toLocalFile, so it works on platforms where
  path!=toLocalPath (e.g. windows)
  Don't split a string and get a chunk, but just extract the chunk we need
  with section. Reduces allocations that are automatically discarded.
  Narrow iconPath scope

TEST PLAN
  Tests still pass, plasmashell still works

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  master

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

AFFECTED FILES
  src/declarativeimports/core/iconitem.cpp
  src/plasma/svg.cpp

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

D5865: Add missing KDE_ENABLE_NAMED_OPERATORS function

2017-05-29 Thread René J . V . Bertin 
rjvbb marked an inline comment as done.
rjvbb added a comment.


  So, how do we proceed?
  
  When I asked other devs what compiler they used to build their MSWin bundles 
they all (both) said they can't use MSVC anyway because it's too buggy.
  
  If a compiler is problematic in general it may not be worth it trying to 
account for it in a macro, trying to coerce it into doing something it cannot 
handle. I don't think that's a reason not to implement the macro at all though. 
Even if we cannot figure out from what version onwards the /fpermissive- option 
works it could still be useful for cross-platform code that really needs named 
operator support to let the macro print a warning (or raise an error) when it 
is built with MSVC.

INLINE COMMENTS

> kfunk wrote in KDECompilerSettings.cmake:239
> Doesn't work on MSVC 2015 Update 3:
> 
>   cl : Command line warning D9002 : ignoring unknown option '/permissive-'
> 
> My Version:
> 
>   Microsoft (R) C/C++ Optimizing Compiler Version 19.00.24215.1 for x64

Have you been (or are you) able to check from what version the option is 
accepted?

REPOSITORY
  R240 Extra CMake Modules

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

To: rjvbb, #frameworks, #build_system, cgilles, kfunk
Cc: kfunk

D6002: Identify PIE binaries (application/x-sharedlib) as executable files

2017-05-29 Thread Fabian Vogt 
fvogt added a comment.


  In https://phabricator.kde.org/D6002#112526, @dfaure wrote:
  
  > Then isExecutable() is wrong API, it misses the check for the executable 
bit. But that's non trivial to fix (check if callers would have the info, 
deprecate method and add replacement which takes more info as input...).
  
  
  Well, for `application/x-shellscript` it was broken before this in the same 
way, I'd say: Without the executable bit it's simply data.

REPOSITORY
  R241 KIO

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

To: fvogt, dfaure, aacid
Cc: asturmlechner, #frameworks

D6002: Identify PIE binaries (application/x-sharedlib) as executable files

2017-05-29 Thread David Faure 
dfaure added a comment.


  Then isExecutable() is wrong API, it misses the check for the executable bit. 
But that's non trivial to fix (check if callers would have the info, deprecate 
method and add replacement which takes more info as input...).

REPOSITORY
  R241 KIO

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

To: fvogt, dfaure, aacid
Cc: asturmlechner, #frameworks

D6002: Identify PIE binaries (application/x-sharedlib) as executable files

2017-05-29 Thread Fabian Vogt 
This revision was automatically updated to reflect the committed changes.
Closed by commit R241:4122b52fee54: Identify PIE binaries 
(application/x-sharedlib) as executable files (authored by fvogt).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D6002?vs=14928=14933

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

AFFECTED FILES
  src/widgets/krun.cpp
  src/widgets/krun.h

To: fvogt, dfaure, aacid
Cc: asturmlechner, #frameworks

D6002: Identify PIE binaries (application/x-sharedlib) as executable files

2017-05-29 Thread David Faure 
dfaure accepted this revision.

REPOSITORY
  R241 KIO

BRANCH
  master

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

To: fvogt, dfaure, aacid
Cc: asturmlechner, #frameworks

D6008: Give a parent to KMoreToolsMenuFactory menus

2017-05-29 Thread Elvis Angelaccio 
elvisangelaccio added a comment.


  Note: we could also deprecate the current createMenuFromGroupingNames() in 
favor of a new createMenuFromGroupingNames() that takes an addition QWidget* 
argument. Not sure which way is the best.

REPOSITORY
  R304 KNewStuff

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

To: elvisangelaccio, mart
Cc: #frameworks

D6008: Give a parent to KMoreToolsMenuFactory menus

2017-05-29 Thread Elvis Angelaccio 
elvisangelaccio created this revision.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: Frameworks.

REVISION SUMMARY
  A QMenu without a parent will be wrongly positioned on Wayland.

TEST PLAN
  KMoreToolsMenuFactory menu in Dolphin status bar now works on wayland

REPOSITORY
  R304 KNewStuff

BRANCH
  master

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

AFFECTED FILES
  src/kmoretools/kmoretoolsmenufactory.cpp
  src/kmoretools/kmoretoolsmenufactory.h

To: elvisangelaccio, mart
Cc: #frameworks

D5865: Add missing KDE_ENABLE_NAMED_OPERATORS function

2017-05-29 Thread Kevin Funk 
kfunk requested changes to this revision.
kfunk added a comment.
This revision now requires changes to proceed.


  Can't go in as-is, as it breaks compilation on MSVC 2015.
  
  But interesting to see that `/Za` is actually problematic. Which proves my 
point: Just don't use named operators in code which claims to be cross-platform.

INLINE COMMENTS

> KDECompilerSettings.cmake:230
> +if (${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 19.0.24210)
> +set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /Za")
> +elseif()

Doesn't work, breaks compilation inside WinSDK 10:

  FAILED: src/CMakeFiles/KF5Parts.dir/readwritepart.cpp.obj
  C:\PROGRA~2\MICROS~2.0\VC\bin\amd64\cl.exe   /nologo /TP -DKCOREADDONS_LIB 
-DKF5Parts_EXPORTS -DQT_CORE_LIB -DQT_DBUS_LIB -DQT_GUI_LIB -DQT_NETWORK_LIB 
-DQT_NO_CAST_TO_ASCII -DQT_NO_DEBUG -DQT_NO_SIG
  NALS_SLOTS_KEYWORDS -DQT_NO_URL_CAST_FROM_STRING -DQT_USE_FAST_OPERATOR_PLUS 
-DQT_USE_QSTRINGBUILDER -DQT_WIDGETS_LIB -DQT_XML_LIB 
-DTRANSLATION_DOMAIN=\"kparts5\" -DUNICODE -DWIN32_LEAN_AND_MEAN -DW
  INVER=0x0600 -D_CRT_NONSTDC_NO_DEPRECATE -D_CRT_SECURE_NO_DEPRECATE 
-D_CRT_SECURE_NO_WARNINGS -D_SCL_SECURE_NO_WARNINGS -D_UNICODE 
-D_USE_MATH_DEFINES -D_WIN32_IE=0x0600 -D_WIN32_WINNT=0x0600 -Isrc -
  IZ:\kderoot\download\git\kparts\src -I. -IZ:\kderoot\include\KF5\KIOWidgets 
-IZ:\kderoot\include\KF5 -IZ:\kderoot\include\KF5\KIOCore 
-IZ:\kderoot\include\KF5\KCoreAddons -IZ:\kderoot\include\qt5 -IZ
  :\kderoot\include\qt5\QtCore -IZ:\kderoot\.\mkspecs\win32-msvc2015 
-IZ:\kderoot\include\KF5\KService -IZ:\kderoot\include\KF5\KConfigCore 
-IZ:\kderoot\include\KF5\KJobWidgets -IZ:\kderoot\include\qt5
  \QtWidgets -IZ:\kderoot\include\qt5\QtGui -IZ:\kderoot\include\qt5\QtNetwork 
-IZ:\kderoot\include\KF5\KCompletion -IZ:\kderoot\include\KF5\KWidgetsAddons 
-IZ:\kderoot\include\KF5\KXmlGui -IZ:\kderoot
  \include\qt5\QtDBus -IZ:\kderoot\include\qt5\QtXml 
-IZ:\kderoot\include\KF5\KConfigWidgets -IZ:\kderoot\include\KF5\KCodecs 
-IZ:\kderoot\include\KF5\KConfigGui -IZ:\kderoot\include\KF5\KAuth -IZ:\kde
  root\include\KF5\KTextWidgets -IZ:\kderoot\include\KF5\SonnetUi 
-IZ:\kderoot\include\KF5\KI18n -IZ:\kderoot\include\KF5\KIconThemes /DWIN32 
/D_WINDOWS /W3 /GR /EHsc /wd4250 /wd4251 /wd4396 /wd4661 /Z
  a /MD /Zi /O2 /Ob1 /DNDEBUG /showIncludes 
/Fosrc\CMakeFiles\KF5Parts.dir\readwritepart.cpp.obj 
/Fdsrc\CMakeFiles\KF5Parts.dir\ /FS -c 
Z:\kderoot\download\git\kparts\src\readwritepart.cpp
  C:\Program Files (x86)\Windows 
Kits\10\include\10.0.14393.0\um\winnt.h(12128): error C2467: illegal 
declaration of anonymous 'struct'

Known issue: 
https://stackoverflow.com/questions/5489326/za-compiler-directive-does-not-compile-system-headers-in-vs2010

Apparently Microsoft doesn't even test their own headers against compiling with 
`/Za`

> KDECompilerSettings.cmake:231
> +set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /Za")
> +elseif()
> +# /permissive- became the preferred switch to obtain 
> standards-compliant

Typo: `else()` then this branch is actually executed.

> KDECompilerSettings.cmake:239
> +# https://sourceforge.net/p/predef/wiki/Compilers/
> +set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /permissive-")
> +endif()

Doesn't work on MSVC 2015 Update 3:

  cl : Command line warning D9002 : ignoring unknown option '/permissive-'

My Version:

  Microsoft (R) C/C++ Optimizing Compiler Version 19.00.24215.1 for x64

REPOSITORY
  R240 Extra CMake Modules

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

To: rjvbb, #frameworks, #build_system, cgilles, kfunk
Cc: kfunk

D6002: Identify PIE binaries (application/x-sharedlib) as executable files

2017-05-29 Thread Fabian Vogt 
fvogt added a comment.


  In https://phabricator.kde.org/D6002#112407, @dfaure wrote:
  
  > This is a workaround for https://bugs.freedesktop.org/show_bug.cgi?id=97226 
but indeed, there seems to be no other way.
  >
  > Maybe add a link to that bug report in one of the uses of x-sharedlib in 
your patch?
  
  
  Not really a workaround - it's the proper solution. There is no difference 
between shared libraries and PIE executables,
  if a shared lib has executable permissions, you can run it, whether it makes 
sense or not.
  As that is how the kernel behaves it's also how KIO should behave.

REPOSITORY
  R241 KIO

BRANCH
  master

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

To: fvogt, dfaure, aacid
Cc: asturmlechner, #frameworks

D6002: Identify PIE binaries (application/x-sharedlib) as executable files

2017-05-29 Thread Fabian Vogt 
fvogt updated this revision to Diff 14928.
fvogt added a comment.


  Add a comment referencing the fdo bug report

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D6002?vs=14914=14928

BRANCH
  master

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

AFFECTED FILES
  src/widgets/krun.cpp
  src/widgets/krun.h

To: fvogt, dfaure, aacid
Cc: asturmlechner, #frameworks

D6002: Identify PIE binaries (application/x-sharedlib) as executable files

2017-05-29 Thread David Faure 
dfaure accepted this revision.
dfaure added a comment.
This revision is now accepted and ready to land.


  This is a workaround for https://bugs.freedesktop.org/show_bug.cgi?id=97226 
but indeed, there seems to be no other way.
  
  Maybe add a link to that bug report in one of the uses of x-sharedlib in your 
patch?

REPOSITORY
  R241 KIO

BRANCH
  master

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

To: fvogt, dfaure, aacid
Cc: asturmlechner, #frameworks