D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

[Méven Car]

meven added a comment.


  In D25010#632977 , @kossebau wrote:
  
  > In D25010#632835 , @dfaure wrote:
  >
  > > Yes, either prefixes or the C++11 way with enum class.
  >
  >
  > I propose to go with prefixes again, for consistency with currrent enum 
usages in KF/KIO APIs.
  
  
  I have a patch D28223 

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, dfaure, kossebau
Cc: mlaurent, dalbers, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

[Friedrich W. H. Kossebau]

kossebau added a comment.


  In D25010#632835 , @dfaure wrote:
  
  > Yes, either prefixes or the C++11 way with enum class.
  
  
  I propose to go with prefixes again, for consistency with currrent enum 
usages in KF/KIO APIs.

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, dfaure, kossebau
Cc: mlaurent, dalbers, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

[David Faure]

dfaure added a comment.


  Yes, either prefixes or the C++11 way with enum class.

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, dfaure, kossebau
Cc: mlaurent, dalbers, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

[Méven Car]

meven marked 2 inline comments as done.
meven added inline comments.

INLINE COMMENTS

> kossebau wrote in global.h:320
> This injects generic terms like `Basic`, `User`, `Time`, `Acl`, etc. into the 
> KIO namespace, with no futher hint that these belong to this very enum, 
> resulting in potential wrong usages (due to completion-based coding when 
> being convertable to int) or in potential conflicts with other future 
> additions.
> 
> Sadly no time to follow the review. Had this been discussed before? Ideally 
> those flags would get more explicit names, like `BasicDetail` (hm, what is 
> basic actually), `UserDetail`, etc.
> 
> Could not find naming recommendations for current Qt, but here some old one, 
> scroll to the section "Naming Enum Types and Values": 
> https://doc.qt.io/archives/qq/qq13-apis.html#theartofnaming

Yes it had been discussed and what you suggest was in a previous iteration : 
https://phabricator.kde.org/D25010?vs=69181=69385=ignore-most#change-FL2qoDJhfEB5

Somehow I removed it :
https://phabricator.kde.org/D25010?vs=70051=70088=ignore-most#change-FL2qoDJhfEB5

But I can't find why I did this, I guess it was a merging/synchronizing issue 
and lost those changes.
Have I already mentioned how much I dislike arcanist as I work on two machines, 
it is quite annoying.

Unless @dfaure you find a reason why I shouldn't do it, I will add those prefix 
back, since KF 5.69 is not yet out of the door, it is possible.

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, dfaure, kossebau
Cc: mlaurent, dalbers, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

[Friedrich W. H. Kossebau]

kossebau added inline comments.

INLINE COMMENTS

> global.h:320
> + */
> +enum StatDetail {
> +/// No field returned, useful to check if a file exists

This injects generic terms like `Basic`, `User`, `Time`, `Acl`, etc. into the 
KIO namespace, with no futher hint that these belong to this very enum, 
resulting in potential wrong usages (due to completion-based coding when being 
convertable to int) or in potential conflicts with other future additions.

Sadly no time to follow the review. Had this been discussed before? Ideally 
those flags would get more explicit names, like `BasicDetail` (hm, what is 
basic actually), `UserDetail`, etc.

Could not find naming recommendations for current Qt, but here some old one, 
scroll to the section "Naming Enum Types and Values": 
https://doc.qt.io/archives/qq/qq13-apis.html#theartofnaming

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, dfaure, kossebau
Cc: mlaurent, dalbers, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

[Méven Car]

meven marked 2 inline comments as done.
meven added inline comments.

INLINE COMMENTS

> kossebau wrote in statjob.h:203
> Ah, was already fixed.

5434ffd0c655b0996e881fec605dc988a672d85c 


REPOSITORY
  R241 KIO

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

To: meven, #frameworks, dfaure, kossebau
Cc: mlaurent, dalbers, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

[Friedrich W. H. Kossebau]

kossebau added a comment.


  In D25010#626677 , @mlaurent wrote:
  
  > Did you test to compile with flags == 5.68.0 ?
  >  I tested it with kdepim and I need to adapt it.
  >  -DKF_DISABLE_DEPRECATED_BEFORE_AND_AT=0x054400 is 5.68.0 ?
  
  
  You also want to adapt kdepim-runtime it seems -> failing as part of 
dependency build e.g. here: 
https://build.kde.org/view/Failing/job/Administration/job/Dependency%20Build%20Extragear%20kf5-qt5%20SUSEQt5.12/lastFailedBuild/console
 (I just  triggered product build, but missed that the latest "Applications" 
dep build might not yet have pulled in the new KIO with the respective 
deprecation).

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, dfaure, kossebau
Cc: mlaurent, dalbers, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

[Friedrich W. H. Kossebau]

kossebau added inline comments.

INLINE COMMENTS

> kossebau wrote in statjob.h:203
> This wants to be "5, 69" now, not "5, 68"

Ah, was already fixed.

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, dfaure, kossebau
Cc: mlaurent, dalbers, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

[Friedrich W. H. Kossebau]

kossebau added inline comments.

INLINE COMMENTS

> statjob.h:203
> +
> +#if KIOCORE_ENABLE_DEPRECATED_SINCE(5, 68)
>  /**

This wants to be "5, 69" now, not "5, 68"

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, dfaure, kossebau
Cc: mlaurent, dalbers, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

[Laurent Montel]

mlaurent added a comment.


  Did you test to compile with flags == 5.68.0 ?
  I tested it with kdepim and I need to adapt it.
  -DKF_DISABLE_DEPRECATED_BEFORE_AND_AT=0x054400 is 5.68.0 ?

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, dfaure, kossebau
Cc: mlaurent, dalbers, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

[David Faure]

dfaure added a comment.


  @mlaurent compilation is broken because a method got deprecated, and you set 
-DKF_DISABLE_DEPRECATED_BEFORE_AND_AT=0x06 so the use of that method no 
longer compiles.
  You can hardly blame other people for deprecating methods, it's a rather 
natural thing to do at this point...
  On the contrary, I strongly suggest to stop setting 
-DKF_DISABLE_DEPRECATED_BEFORE_AND_AT=0x06, it just breaks too much. It 
even breaks stable branches.
  
  I fixed that for kjumpingcube in 
https://commits.kde.org/kjumpingcube/5f238b424da1c57c56024a7e871579075bd06c73
  
  In order to port away from deprecated methods, I suggest a script that 
increases the KF_DISABLE_DEPRECATED_BEFORE_AND_AT, which you (or anyone who 
wants to help with that) can run and fix compilation before pushing. It's what 
I regularly do for KF5 itself.

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, dfaure, kossebau
Cc: mlaurent, dalbers, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

[Laurent Montel]

mlaurent added a comment.


  "/compile/kde5/framework/kde/kdegames/kjumpingcube/game.cpp:582:63: erreur: 
impossible de convertir « KIO::StatJob::DestinationSide » de « 
KIO::StatJob::StatSide » vers « KIO::JobFlags » {aka « QFlags »}
  
582 |  KIO::StatJob* statJob = KIO::stat(url, 
KIO::StatJob::DestinationSide, 0);
| 
~~^~~
|   |
|   
KIO::StatJob::StatSide
  
  /compile/kde5/framework/kde/kdegames/kjumpingcube/game.cpp: Dans la fonction 
membre « void Game::loadGame() »:
  /compile/kde5/framework/kde/kdegames/kjumpingcube/game.cpp:627:60: erreur: 
impossible de convertir « KIO::StatJob::SourceSide » de « 
KIO::StatJob::StatSide » vers « KIO::JobFlags » {aka « QFlags »}
  
627 |   KIO::StatJob* statJob = KIO::stat(url, 
KIO::StatJob::SourceSide, 0);
|  ~~^~
||
|
KIO::StatJob::StatSide
  
  "
  
  compile is broken in several package see below.

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, dfaure, kossebau
Cc: mlaurent, dalbers, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

[David Faure]

dfaure added a comment.


  Fixed in https://commits.kde.org/kio/440bec1b3524f168fc04898a09ac51457c79ed3b

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, dfaure, kossebau
Cc: dalbers, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

[Daniel Albers]

dalbers added a comment.


  This doesn't seem to compile:
  
[ 26%] Building CXX object 
src/ioslaves/file/CMakeFiles/kio_file.dir/file.cpp.o
cd /usr/src/kio-git/src/build/src/ioslaves/file && /usr/lib/ccache/bin/c++  
-DKCOMPLETION_DISABLE_DEPRECATED_BEFORE_AND_AT=0x054100 -DKCOREADDONS_LIB 
-DKF_DEPRECATED_WARNINGS_SINCE=0x06 
-DKF_DISABLE_DEPRECATED_BEFORE_AND_AT=0x054300 
-DKIOCORE_DEPRECATED_WARNINGS_SINCE=0x0 
-DKIOCORE_DISABLE_DEPRECATED_BEFORE_AND_AT=0x0 -DQT_CONCURRENT_LIB 
-DQT_CORE_LIB -DQT_DBUS_LIB -DQT_DEPRECATED_WARNINGS_SINCE=0x06 
-DQT_DISABLE_DEPRECATED_BEFORE=0x050d00 -DQT_NETWORK_LIB 
-DQT_NO_CAST_FROM_ASCII -DQT_NO_CAST_FROM_BYTEARRAY -DQT_NO_CAST_TO_ASCII 
-DQT_NO_DEBUG -DQT_NO_NARROWING_CONVERSIONS_IN_CONNECT 
-DQT_NO_SIGNALS_SLOTS_KEYWORDS -DQT_NO_URL_CAST_FROM_STRING 
-DQT_STRICT_ITERATORS -DQT_USE_QSTRINGBUILDER -DTRANSLATION_DOMAIN=\"kio5\" 
-D_GNU_SOURCE -D_LARGEFILE64_SOURCE -Dkio_file_EXPORTS 
-I/home/al/.cache/yay/kio-git/src/build/src/ioslaves/file 
-I/home/al/.cache/yay/kio-git/src/kio/src/ioslaves/file 
-I/home/al/.cache/yay/kio-git/src/build/src/ioslaves/file/kio_file_autogen/include
 -I/home/al/.cache/yay/kio-git/src/build/src/core/.. 
-I/home/al/.cache/yay/kio-git/src/kio/src/core/kssl 
-I/home/al/.cache/yay/kio-git/src/build/src/core 
-I/home/al/.cache/yay/kio-git/src/kio/src/core -isystem 
/usr/include/KF5/KCoreAddons -isystem /usr/include/KF5-isystem /usr/include/qt 
-isystem /usr/include/qt/QtCore -isystem /usr/lib/qt/mkspecs/linux-g++ -isystem 
/usr/include/KF5/KService -isystem /usr/include/KF5/KConfigCore -isystem 
/usr/include/qt/QtNetwork -isystem /usr/include/qt/QtConcurrent -isystem 
/usr/include/qt/QtDBus -isystem /usr/include/KF5/KI18n -isystem 
/usr/include/KF5/KAuth  -march=x86-64 -mtune=native -O2 -pipe -fno-plt -g 
-fvar-tracking-assignments 
-fdebug-prefix-map=/home/al/.cache/yay/kio-git/src=/usr/src/debug 
-fno-operator-names -fno-exceptions -Wall -Wextra -Wcast-align 
-Wchar-subscripts -Wformat-security -Wno-long-long -Wpointer-arith -Wundef 
-Wnon-virtual-dtor -Woverloaded-virtual -Werror=return-type -Wvla -Wdate-time 
-Wsuggest-override -Wlogical-op -pedantic -Wzero-as-null-pointer-constant -O3 
-DNDEBUG -fPIC -fvisibility=hidden -fvisibility-inlines-hidden -fPIC 
-std=gnu++11 -o CMakeFiles/kio_file.dir/file.cpp.o -c 
/home/al/.cache/yay/kio-git/src/kio/src/ioslaves/file/file.cpp
/usr/src/kio-git/src/kio/src/ioslaves/file/file.cpp: In member function 
'bool FileProtocol::createUDSEntry(const QString&, const QByteArray&, 
KIO::UDSEntry&, KIO::StatDetails)':
/usr/src/kio-git/src/kio/src/ioslaves/file/file.cpp:1041:38: error: 
'linkTargetBuffer' was not declared in this scope
1041 | targetPath = linkTargetBuffer;
|  ^~~~
make[2]: *** [src/ioslaves/file/CMakeFiles/kio_file.dir/build.make:76: 
src/ioslaves/file/CMakeFiles/kio_file.dir/file.cpp.o] Error 1

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, dfaure, kossebau
Cc: dalbers, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

[Méven Car]

This revision was automatically updated to reflect the committed changes.
Closed by commit R241:9537bca2b542: [StatJob] Use A QFlag to specify the 
details returned by StatJob (authored by meven).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25010?vs=77386=77460

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

AFFECTED FILES
  autotests/jobremotetest.cpp
  autotests/jobtest.cpp
  autotests/jobtest.h
  src/core/CMakeLists.txt
  src/core/copyjob.cpp
  src/core/deletejob.cpp
  src/core/directorysizejob.cpp
  src/core/global.h
  src/core/statjob.cpp
  src/core/statjob.h
  src/filewidgets/kdiroperator.cpp
  src/ioslaves/file/file.cpp
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp
  src/widgets/krun.cpp
  src/widgets/paste.cpp
  tests/kioslavetest.cpp
  tests/listjobtest.cpp

To: meven, #frameworks, dfaure, kossebau
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

[David Faure]

dfaure added a comment.


  Yep, that's what "Accepted" means ;)

REPOSITORY
  R241 KIO

BRANCH
  arcpatch-D25010

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

To: meven, #frameworks, dfaure, kossebau
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

[Méven Car]

meven added a comment.


  Ok to merge ?

REPOSITORY
  R241 KIO

BRANCH
  arcpatch-D25010

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

To: meven, #frameworks, dfaure, kossebau
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

[David Faure]

dfaure accepted this revision.

REPOSITORY
  R241 KIO

BRANCH
  arcpatch-D25010

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

To: meven, #frameworks, dfaure, kossebau
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

[Méven Car]

meven updated this revision to Diff 77386.
meven added a comment.


  Improve comment of StatJob *statDetails

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25010?vs=77385=77386

BRANCH
  arcpatch-D25010

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

AFFECTED FILES
  autotests/jobremotetest.cpp
  autotests/jobtest.cpp
  autotests/jobtest.h
  src/core/CMakeLists.txt
  src/core/copyjob.cpp
  src/core/deletejob.cpp
  src/core/directorysizejob.cpp
  src/core/global.h
  src/core/statjob.cpp
  src/core/statjob.h
  src/filewidgets/kdiroperator.cpp
  src/ioslaves/file/file.cpp
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp
  src/widgets/krun.cpp
  src/widgets/paste.cpp
  tests/kioslavetest.cpp
  tests/listjobtest.cpp

To: meven, #frameworks, dfaure, kossebau
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

[Méven Car]

meven updated this revision to Diff 77385.
meven added a comment.


  Fix typo, make StatJob::setDetails(KIO::StatDetail detail) build only when 
build_deprecated(5.69) is set

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25010?vs=77235=77385

BRANCH
  arcpatch-D25010

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

AFFECTED FILES
  autotests/jobremotetest.cpp
  autotests/jobtest.cpp
  autotests/jobtest.h
  src/core/CMakeLists.txt
  src/core/copyjob.cpp
  src/core/deletejob.cpp
  src/core/directorysizejob.cpp
  src/core/global.h
  src/core/statjob.cpp
  src/core/statjob.h
  src/filewidgets/kdiroperator.cpp
  src/ioslaves/file/file.cpp
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp
  src/widgets/krun.cpp
  src/widgets/paste.cpp
  tests/kioslavetest.cpp
  tests/listjobtest.cpp

To: meven, #frameworks, dfaure, kossebau
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

[David Faure]

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


  Huh. Sorry about the delays and the need to update version numbers once more.
  
  My last two comments are still not fixed. To see the issues, grep for 
"direcly" ('t' missing) and "Details int" (comma missing? or "int" should be 
removed?).
  Or just land as is and I'll fix them in a followup commit, it'll be faster...

REPOSITORY
  R241 KIO

BRANCH
  arcpatch-D25010

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

To: meven, #frameworks, dfaure, kossebau
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

[Méven Car]

meven updated this revision to Diff 77235.
meven added a comment.


  Get code ready for 5.69, I really HOPE that the last time I have to do this, 
remove a few qDebug(), add some test assertions, make test more reliable

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25010?vs=75740=77235

BRANCH
  arcpatch-D25010

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

AFFECTED FILES
  autotests/jobremotetest.cpp
  autotests/jobtest.cpp
  autotests/jobtest.h
  src/core/CMakeLists.txt
  src/core/copyjob.cpp
  src/core/deletejob.cpp
  src/core/directorysizejob.cpp
  src/core/global.h
  src/core/statjob.cpp
  src/core/statjob.h
  src/filewidgets/kdiroperator.cpp
  src/ioslaves/file/file.cpp
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp
  src/widgets/krun.cpp
  src/widgets/paste.cpp
  tests/kioslavetest.cpp
  tests/listjobtest.cpp

To: meven, #frameworks, dfaure, kossebau
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

[Méven Car]

meven updated this revision to Diff 75740.
meven added a comment.


  Add an assertion to JobTest::stat()^Ccomment out other tests debug output

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25010?vs=75567=75740

BRANCH
  arcpatch-D25010_2

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

AFFECTED FILES
  autotests/jobremotetest.cpp
  autotests/jobtest.cpp
  autotests/jobtest.h
  src/core/CMakeLists.txt
  src/core/copyjob.cpp
  src/core/deletejob.cpp
  src/core/directorysizejob.cpp
  src/core/global.h
  src/core/statjob.cpp
  src/core/statjob.h
  src/filewidgets/kdiroperator.cpp
  src/ioslaves/file/file.cpp
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp
  src/widgets/krun.cpp
  src/widgets/paste.cpp
  tests/kioslavetest.cpp
  tests/listjobtest.cpp

To: meven, #frameworks, dfaure, kossebau
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

[Méven Car]

meven updated this revision to Diff 75567.
meven marked an inline comment as done.
meven added a comment.


  Make setDetails(KIO::StatDetail detail) compile only when deprecated since 
5.68 is enabled

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25010?vs=75566=75567

BRANCH
  arcpatch-D25010

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

AFFECTED FILES
  autotests/jobremotetest.cpp
  autotests/jobtest.cpp
  autotests/jobtest.h
  src/core/CMakeLists.txt
  src/core/copyjob.cpp
  src/core/deletejob.cpp
  src/core/directorysizejob.cpp
  src/core/global.h
  src/core/statjob.cpp
  src/core/statjob.h
  src/filewidgets/kdiroperator.cpp
  src/ioslaves/file/file.cpp
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp
  src/widgets/krun.cpp
  src/widgets/paste.cpp
  tests/kioslavetest.cpp
  tests/listjobtest.cpp

To: meven, #frameworks, dfaure, kossebau
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

[Méven Car]

meven updated this revision to Diff 75566.
meven marked 3 inline comments as done.
meven added a comment.


  Add a test for setDetails(KIO::StatDetails), fix comment, add a 
KIOCORE_ENABLE_DEPRECATED_SINCE

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25010?vs=75304=75566

BRANCH
  arcpatch-D25010

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

AFFECTED FILES
  autotests/jobremotetest.cpp
  autotests/jobtest.cpp
  autotests/jobtest.h
  src/core/CMakeLists.txt
  src/core/copyjob.cpp
  src/core/deletejob.cpp
  src/core/directorysizejob.cpp
  src/core/global.h
  src/core/statjob.cpp
  src/core/statjob.h
  src/filewidgets/kdiroperator.cpp
  src/ioslaves/file/file.cpp
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp
  src/widgets/krun.cpp
  src/widgets/paste.cpp
  tests/kioslavetest.cpp
  tests/listjobtest.cpp

To: meven, #frameworks, dfaure, kossebau
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

[David Faure]

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

INLINE COMMENTS

> statjob.cpp:97
> +
> +void StatJob::setDetails(short int details)
> +{

When the declaration is in #if, the definition should be too (with 
s/ENABLED/BUILD/).

#if KIOCORE_BUILD_DEPRECATED_SINCE(5, 68)

> statjob.h:79
> +void setDetails(KIO::StatDetails details);
> +void setDetails(KIO::StatDetail detail);
> +

I guess this overload exists because of the setDetails(short int) overload?
(QFlags has an implicit constructor from Enum so usually we don't need a 
single-flag overload)

If so, I suppose we can get rid of it for Qt6.
Or when KIOCORE_ENABLE_DEPRECATED_SINCE(5, 68) isn't set.
So basically it could move into an #else section of the #if below?

> dfaure wrote in statjob.h:226
> Why does this say "int" after "StatDetails"?

This was marked as done, but I still see ", KIO::StatDetails int,"

> dfaure wrote in statjob.h:234
> Typo: direcly -> directly (happens again two lines down)

Still there

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, dfaure, kossebau
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

[Méven Car]

meven updated this revision to Diff 75304.
meven added a comment.
This revision is now accepted and ready to land.


  Fix implementation having a statDetails function to distinguish from the 
original function, fix and adapt test, in file_unix when using statx reduce the 
field retrieved for symlinks

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25010?vs=71864=75304

BRANCH
  arcpatch-D25010

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

AFFECTED FILES
  autotests/jobremotetest.cpp
  autotests/jobtest.cpp
  autotests/jobtest.h
  src/core/CMakeLists.txt
  src/core/copyjob.cpp
  src/core/deletejob.cpp
  src/core/directorysizejob.cpp
  src/core/global.h
  src/core/statjob.cpp
  src/core/statjob.h
  src/filewidgets/kdiroperator.cpp
  src/ioslaves/file/file.cpp
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp
  src/widgets/krun.cpp
  src/widgets/paste.cpp
  tests/kioslavetest.cpp
  tests/listjobtest.cpp

To: meven, #frameworks, dfaure, kossebau
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

[Méven Car]

meven planned changes to this revision.
meven added a comment.


  I have tried adding unit tests.
  It showed that the old:
  ` *stat(const QUrl , KIO::StatJob::StatSide side, short int details, 
JobFlags flags = DefaultFlags);`
  Takes over the new :
  `KIO::stat(const QUrl &, KIO::StatSide, KIO::StatDetails int, JobFlags)")`
  Since KIO::StatDetails data type is int and enums are transparently converted 
to integer.
  I will need to find a workarount or rename/change the signature of the new 
`KIO::stat` to make them apart

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, dfaure, kossebau
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

[Méven Car]

meven marked an inline comment as done.

REPOSITORY
  R241 KIO

BRANCH
  arcpatch-D25010_1

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

To: meven, #frameworks, dfaure, kossebau
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

[David Faure]

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

REPOSITORY
  R241 KIO

BRANCH
  arcpatch-D25010_1

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

To: meven, #frameworks, dfaure, kossebau
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

[Méven Car]

meven updated this revision to Diff 71864.
meven marked 2 inline comments as done.
meven added a comment.


  Replace use of KIOCORE_ENABLE_DEPRECATED_SINCE by 
KIOCORE_BUILD_DEPRECATED_SINCE in cpp files

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25010?vs=71703=71864

BRANCH
  arcpatch-D25010_1

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

AFFECTED FILES
  autotests/jobremotetest.cpp
  src/core/CMakeLists.txt
  src/core/copyjob.cpp
  src/core/deletejob.cpp
  src/core/directorysizejob.cpp
  src/core/global.h
  src/core/statjob.cpp
  src/core/statjob.h
  src/filewidgets/kdiroperator.cpp
  src/ioslaves/file/file.cpp
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp
  src/widgets/krun.cpp
  src/widgets/paste.cpp
  tests/kioslavetest.cpp
  tests/listjobtest.cpp

To: meven, #frameworks, dfaure, kossebau
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

[David Faure]

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

INLINE COMMENTS

> kossebau wrote in directorysizejob.cpp:137
> More correct would be KIOCORE_BUILD_DEPRECATED_SINCE(5, 65)., not ENABLE here 
> and in the other cpp files.

This still uses ENABLED instead of BUILD, see initial comment by kossebau.

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, dfaure, kossebau
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

[Méven Car]

meven updated this revision to Diff 71703.
meven added a comment.


  Fix Two references to KF 5.65

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25010?vs=71702=71703

BRANCH
  arcpatch-D25010_1

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

AFFECTED FILES
  autotests/jobremotetest.cpp
  src/core/copyjob.cpp
  src/core/deletejob.cpp
  src/core/directorysizejob.cpp
  src/core/global.h
  src/core/statjob.cpp
  src/core/statjob.h
  src/filewidgets/kdiroperator.cpp
  src/ioslaves/file/file.cpp
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp
  src/widgets/krun.cpp
  src/widgets/paste.cpp
  tests/kioslavetest.cpp
  tests/listjobtest.cpp

To: meven, #frameworks, dfaure, kossebau
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

[Méven Car]

meven updated this revision to Diff 71702.
meven added a comment.


  Fix Two references to KF 5,65

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25010?vs=71701=71702

BRANCH
  arcpatch-D25010_1

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

AFFECTED FILES
  autotests/jobremotetest.cpp
  src/core/copyjob.cpp
  src/core/deletejob.cpp
  src/core/directorysizejob.cpp
  src/core/global.h
  src/core/statjob.cpp
  src/core/statjob.h
  src/filewidgets/kdiroperator.cpp
  src/ioslaves/file/file.cpp
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp
  src/widgets/krun.cpp
  src/widgets/paste.cpp
  tests/kioslavetest.cpp
  tests/listjobtest.cpp

To: meven, #frameworks, dfaure, kossebau
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

[Méven Car]

meven updated this revision to Diff 71701.
meven added a comment.


  Rebase for kio 5.66

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25010?vs=70669=71701

BRANCH
  arcpatch-D25010_1

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

AFFECTED FILES
  autotests/jobremotetest.cpp
  src/core/copyjob.cpp
  src/core/deletejob.cpp
  src/core/directorysizejob.cpp
  src/core/global.h
  src/core/statjob.cpp
  src/core/statjob.h
  src/filewidgets/kdiroperator.cpp
  src/ioslaves/file/file.cpp
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp
  src/widgets/krun.cpp
  src/widgets/paste.cpp
  tests/kioslavetest.cpp
  tests/listjobtest.cpp

To: meven, #frameworks, dfaure, kossebau
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

[David Faure]

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

REPOSITORY
  R241 KIO

BRANCH
  arcpatch-D25010

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

To: meven, #frameworks, dfaure, kossebau
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

[Méven Car]

meven updated this revision to Diff 70669.
meven marked an inline comment as done.
meven added a comment.


  Fix: | instead of & to build a QFlags

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25010?vs=70543=70669

BRANCH
  arcpatch-D25010

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

AFFECTED FILES
  autotests/jobremotetest.cpp
  src/core/copyjob.cpp
  src/core/deletejob.cpp
  src/core/directorysizejob.cpp
  src/core/global.h
  src/core/statjob.cpp
  src/core/statjob.h
  src/filewidgets/kdiroperator.cpp
  src/ioslaves/file/file.cpp
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp
  src/widgets/krun.cpp
  src/widgets/paste.cpp
  tests/kioslavetest.cpp
  tests/listjobtest.cpp

To: meven, #frameworks, dfaure, kossebau
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

[David Faure]

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

INLINE COMMENTS

> directorysizejob.cpp:141
> +#endif
> +listJob->addMetaData(QStringLiteral("statDetails"), 
> QString::number(KIO::StatDetail::Basic & KIO::StatDetail::Inode));
>  q->connect(listJob, SIGNAL(entries(KIO::Job*,KIO::UDSEntryList)),

You want |, not & ...

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, dfaure, kossebau
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

[Méven Car]

meven updated this revision to Diff 70543.
meven added a comment.


  Do not forget to get UDS_DEVICE_ID UDS_INODE

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25010?vs=70522=70543

BRANCH
  arcpatch-D25010

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

AFFECTED FILES
  autotests/jobremotetest.cpp
  src/core/copyjob.cpp
  src/core/deletejob.cpp
  src/core/directorysizejob.cpp
  src/core/global.h
  src/core/statjob.cpp
  src/core/statjob.h
  src/filewidgets/kdiroperator.cpp
  src/ioslaves/file/file.cpp
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp
  src/widgets/krun.cpp
  src/widgets/paste.cpp
  tests/kioslavetest.cpp
  tests/listjobtest.cpp

To: meven, #frameworks, dfaure, kossebau
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

[David Faure]

dfaure added inline comments.

INLINE COMMENTS

> meven wrote in directorysizejob.cpp:137
> The previous code did (details of 3)

I'm confused by what you mean here... and I think I found more problems:

Indeed the old code used 3.
In your patch, you changed this to 2, in the legacy field for old kioslaves; 
any reason for that?
"3" adds UDS_DEVICE_ID/UDS_INODE which are used on lines 154/157 here. Surely 
we want to keep using 3.

And this means, in the new field statDetails, we want to specify KIO::Inode to 
get those fields.

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, dfaure, kossebau
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

[Méven Car]

meven marked 5 inline comments as done.

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, dfaure, kossebau
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

[Méven Car]

meven marked 2 inline comments as done.
meven added inline comments.

INLINE COMMENTS

> dfaure wrote in directorysizejob.cpp:137
> Now that I look at this again, we do NOT want to follow symlinks here.
> 
> This was a bug in the existing code.
> 
> In Dolphin we want to display the size of the target file (more useful 
> information than the size of the symlink itself), but when calculating 
> directory size, we do NOT want to follow the symlink, the target size does 
> not count.
> 
> These new flags allow to fix this long-standing bug.

The previous code did (details of 3)

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, dfaure, kossebau
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

[Méven Car]

meven updated this revision to Diff 70522.
meven added a comment.


  Rebasing

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25010?vs=70521=70522

BRANCH
  arcpatch-D25010_1

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

AFFECTED FILES
  autotests/jobremotetest.cpp
  src/core/copyjob.cpp
  src/core/deletejob.cpp
  src/core/directorysizejob.cpp
  src/core/global.h
  src/core/statjob.cpp
  src/core/statjob.h
  src/filewidgets/kdiroperator.cpp
  src/ioslaves/file/file.cpp
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp
  src/widgets/krun.cpp
  src/widgets/paste.cpp
  tests/kioslavetest.cpp
  tests/listjobtest.cpp

To: meven, #frameworks, dfaure, kossebau
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

[Méven Car]

meven updated this revision to Diff 70521.
meven added a comment.


  Lower the amount of data pulled by directory size job (avoids resolving 
symlinks notably)

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25010?vs=70088=70521

BRANCH
  arcpatch-D25010_1

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

AFFECTED FILES
  autotests/jobremotetest.cpp
  src/core/CMakeLists.txt
  src/core/copyjob.cpp
  src/core/deletejob.cpp
  src/core/directorysizejob.cpp
  src/core/global.h
  src/core/statjob.cpp
  src/core/statjob.h
  src/filewidgets/kdiroperator.cpp
  src/ioslaves/file/file.cpp
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp
  src/widgets/krun.cpp
  src/widgets/paste.cpp
  tests/kioslavetest.cpp
  tests/listjobtest.cpp

To: meven, #frameworks, dfaure, kossebau
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

[David Faure]

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


  In D25010#565502 , @meven wrote:
  
  > Funny thing is that statx includes always the name, down to syscall we can 
save at most STATX_SIZE , STATX_TYPE fields in NoDetails case.
  
  
  But this isn't only about configuring the statx call. If we are not 
interested in the name, we save the QFile::decodeName() call (8 bit to 16 bit 
conversion, triggering also a memory allocation).
  
  > I updated kdiroperator.cpp to use it line 748 since it only checks if a 
file exists.
  >  This code path is not run for local files anyway.
  
  Right -- at some point we should also use the details stuff in other ioslaves 
:-)
  
  > Another thing comes to mind
  >  If we allow not to fill KIO::UDSEntry::UDS_LINK_DEST but when 
KIO::ResolveSymlink is passed, we can save the second STAT by not passing 
AT_SYMLINK_NOFOLLOW to the first statx.
  
  Interesting, but given that statx isn't always available, I fear this will 
make the code quite complex?
  
  > So perhaps we want to expose a detail for this use case : that is add a 
IncludeLinkDest to KIO::StatDetail removing this field for StatDetail::Basic.
  >  I am not sure we have any use case currently though.
  
  I need to think more about this, gotta go.

INLINE COMMENTS

> kossebau wrote in directorysizejob.cpp:137
> More correct would be KIOCORE_BUILD_DEPRECATED_SINCE(5, 65)., not ENABLE here 
> and in the other cpp files.

Now that I look at this again, we do NOT want to follow symlinks here.

This was a bug in the existing code.

In Dolphin we want to display the size of the target file (more useful 
information than the size of the symlink itself), but when calculating 
directory size, we do NOT want to follow the symlink, the target size does not 
count.

These new flags allow to fix this long-standing bug.

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, dfaure, kossebau
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

[Méven Car]

meven added a comment.


  In D25010#565421 , @dfaure wrote:
  
  > Hmm, I meant Name is always useful when *listing*. But when *stating*, we 
don't always need to get the name back. The NoDetails comment is about a stat 
that really can just succeed/fail.
  
  
  I added
  
enum StatDetail {
/// No field returned, useful to check if a file exists
NoDetails = 0x0,
  
  Funny thing is that statx includes always the name, down to syscall we can 
save at most STATX_SIZE , STATX_TYPE fields in NoDetails case.
  I updated kdiroperator.cpp to use it line 748 since it only checks if a file 
exists.
  This code path is not run for local files anyway.
  
  > We can always add NoDetails later (with a value of 0), however, so I'll 
stop nitpicking here :-)
  
  I would not mind nitpicking/discussing a little more, this is easier to do 
now and since it is no small change.
  As long as we don't go off topic at least now I have a design standpoint 
validation.
  
  Another thing comes to mind
  If we allow not to fill KIO::UDSEntry::UDS_LINK_DEST but when 
KIO::ResolveSymlink is passed, we can save the second STAT by not passing 
AT_SYMLINK_NOFOLLOW to the first statx.
  So perhaps we want to expose a detail for this use case : that is add a 
IncludeLinkDest to KIO::StatDetail removing this field for StatDetail::Basic.
  I am not sure we have any use case currently though.

INLINE COMMENTS

> dfaure wrote in statjob.h:236
> missing the same #if as the above methods, no?
> In a "clean" build it should disappear.

It shares the same #if block with the stat function

REPOSITORY
  R241 KIO

BRANCH
  arcpatch-D25010

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

To: meven, #frameworks, dfaure, kossebau
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

[Méven Car]

meven updated this revision to Diff 70088.
meven added a comment.


  Add NoDetails to StatDetail, add StatJob::setDetails\(KIO::StatDetail 
detail\), small comment update

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25010?vs=70051=70088

BRANCH
  arcpatch-D25010

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

AFFECTED FILES
  autotests/jobremotetest.cpp
  src/core/CMakeLists.txt
  src/core/copyjob.cpp
  src/core/deletejob.cpp
  src/core/directorysizejob.cpp
  src/core/global.h
  src/core/statjob.cpp
  src/core/statjob.h
  src/filewidgets/kdiroperator.cpp
  src/ioslaves/file/file.cpp
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp
  src/widgets/krun.cpp
  src/widgets/paste.cpp
  tests/kioslavetest.cpp
  tests/listjobtest.cpp

To: meven, #frameworks, dfaure, kossebau
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

[David Faure]

dfaure added a comment.


  Isn't it simpler to commit as is, so we can just s/ENABLE/BUILD/ in *.cpp 
files once we add support for EXCLUDE_DEPRECATED_BEFORE_AND_AT? Faster than 
having to figure out where is the implementation of each  method to exclude.

REPOSITORY
  R241 KIO

BRANCH
  arcpatch-D25010_1

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

To: meven, #frameworks, dfaure, kossebau
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

[Friedrich W. H. Kossebau]

kossebau added a comment.


  Ah, EXCLUDE_DEPRECATED_BEFORE_AND_AT is not yet available with KIO (compare 
the TODO next to the ecm_generate_export_header call).
  So as result also the BUILD macros are not available. Using instead the 
ENABLE macros in the sources of the library instead does not make that much 
sense, they will be never triggerd to evaluate to FALSE.
  
  IMHO they should be left out simply in the sources.

REPOSITORY
  R241 KIO

BRANCH
  arcpatch-D25010_1

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

To: meven, #frameworks, dfaure, kossebau
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

[Friedrich W. H. Kossebau]

kossebau added inline comments.

INLINE COMMENTS

> directorysizejob.cpp:137
>  KIO::ListJob *listJob = KIO::listRecursive(url, KIO::HideProgressInfo);
> -listJob->addMetaData(QStringLiteral("details"), QStringLiteral("3"));
> +#if KIOCORE_ENABLE_DEPRECATED_SINCE(5, 65)
> +// TODO KF6: remove legacy details code path

More correct would be KIOCORE_BUILD_DEPRECATED_SINCE(5, 65)., not ENABLE here 
and in the other cpp files.

REPOSITORY
  R241 KIO

BRANCH
  arcpatch-D25010_1

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

To: meven, #frameworks, dfaure, kossebau
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

[David Faure]

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


  Hmm, I meant Name is always useful when *listing*. But when *stating*, we 
don't always need to get the name back. The NoDetails comment is about a stat 
that really can just succeed/fail.
  
  We can always add NoDetails later (with a value of 0), however, so I'll stop 
nitpicking here :-)

REPOSITORY
  R241 KIO

BRANCH
  arcpatch-D25010_1

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

To: meven, #frameworks, dfaure, kossebau
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

[Méven Car]

meven updated this revision to Diff 70051.
meven marked 8 inline comments as done.
meven added a comment.


  Typos, rephrase a comment, move FileProtocol::getStatDetails to file.cpp, add 
a TODO for in file_win.cpp

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25010?vs=69385=70051

BRANCH
  arcpatch-D25010_1

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

AFFECTED FILES
  autotests/jobremotetest.cpp
  src/core/CMakeLists.txt
  src/core/copyjob.cpp
  src/core/deletejob.cpp
  src/core/directorysizejob.cpp
  src/core/global.h
  src/core/statjob.cpp
  src/core/statjob.h
  src/filewidgets/kdiroperator.cpp
  src/ioslaves/file/file.cpp
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp
  src/ioslaves/file/file_win.cpp
  src/widgets/krun.cpp
  src/widgets/paste.cpp
  tests/kioslavetest.cpp
  tests/listjobtest.cpp

To: meven, #frameworks, dfaure, kossebau
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

[David Faure]

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


  About granularity: I think it's fine. I was about to say that the use case of 
a recursive listing to calculate directory size only needs type and size, but 
it actually needs name too (for dirs), in order to recurse into subdirs.
  
  So overall, I think Name is always useful, and should always be provided, 
whether or not the Basic flag is set.
  
  Access, type and size aren't always needed, but they come at zero cost 
(copying a few ints), while name implies string conversions (8bit->unicode). So 
I don't think it makes sense to make access, type and size optional.
  
  Hmm, with this reasoning, linkDest should be separated out. But that means 
going through all users of StatBasic and finding out if they look at 
UDS_LINK_DEST.

INLINE COMMENTS

> statjob.h:91
>   */
> +KIOCORE_DEPRECATED_VERSION(5, 65, "Use setDetails(KIO::statDetails)")
>  void setDetails(short int details);

Typo: s/stat/Stat/

> statjob.h:189
> + * @param details selects the level of details we want.
> + * You can fine grain the detail level you want.
> + * @param flags Can be HideProgressInfo here

Not sure what this specific sentence adds to the previous one. It's a 
parameter, obviously that means we can choose what to pass to it... Maybe more 
useful to mention that this is for performance reasons, less details implies 
more speed.

> statjob.h:226
>   */
> +KIOCORE_DEPRECATED_VERSION(5, 65, "Use KIO::stat(const QUrl &, 
> KIO::StatSide, KIO::StatDetails int, JobFlags)")
>  KIOCORE_EXPORT StatJob *stat(const QUrl , KIO::StatJob::StatSide side,

Why does this say "int" after "StatDetails"?

> statjob.h:234
> + * @since 5.65
> + * @deprecated since 5.65, use direcly KIO::StatDetails
> + */

Typo: direcly -> directly (happens again two lines down)

> statjob.h:236
> + */
> +KIOCORE_DEPRECATED_VERSION(5, 65, "Use direcly KIO::StatDetails")
> +KIOCORE_EXPORT KIO::StatDetails detailsToStatDetails(int details);

missing the same #if as the above methods, no?
In a "clean" build it should disappear.

> kdiroperator.cpp:748
>  KJobWidgets::setWindow(job, this);
> -job->setDetails(0); //We only want to know if it exists, 0 == 
> that.
> +job->setDetails(KIO::StatBasic); //We only want to know if it 
> exists
>  job->setSide(KIO::StatJob::DestinationSide);

This would actually be a use case for an even more basic level, "NoDetails"...

> file.h:123
>  QFile *mFile;
> +KIO::StatDetails getStatDetails();
>  };

Please move it up with the other private methods. This last section is member 
variables only.

Also, it could be marked as `const`.

> file_unix.cpp:542
>  
> -const QString sDetails = metaData(QStringLiteral("details"));
> -const int details = sDetails.isEmpty() ? 2 : sDetails.toInt();
> +const KIO::StatDetails details = getStatDetails();
>  //qDebug() << "= LIST " << url << "details=" << details << " 
> =";

file_win.cpp needs to be ported the same way.

> file_unix.cpp:830
>  
> +KIO::StatDetails FileProtocol::getStatDetails()
> +{

This should go to file.cpp so that it's available on Windows too, it's not Unix 
specific.

OK, right now it's not really implemented on Windows, but let's make it easy 
for the future developer who will implement it ;)

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, dfaure, kossebau
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

[Méven Car]

meven marked 5 inline comments as done.
meven added a comment.


  friendly ping
  
  Is https://phabricator.kde.org/D25010#inline-142132 satisfactory ?
  I think names are now explicit enough.
  I still wonder if the granularity level is enough.

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, dfaure, kossebau
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

[Méven Car]

meven added a comment.


  I feel the code is ready for review toward merging.

INLINE COMMENTS

> global.h:321
> +/// Filename, access, type, size, linkdest
> +StatBasic = 0x1,
> +/// uid, gid

Naming should be explicit enough now.
I still wonder about granularity if I should expose some more.

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, dfaure, kossebau
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

[Méven Car]

meven updated this revision to Diff 69385.
meven marked an inline comment as done.
meven added a comment.


  Add Stat prefix to enum values of KIO::StatDetail

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25010?vs=69261=69385

BRANCH
  arcpatch-D25010

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

AFFECTED FILES
  autotests/jobremotetest.cpp
  src/core/CMakeLists.txt
  src/core/copyjob.cpp
  src/core/deletejob.cpp
  src/core/directorysizejob.cpp
  src/core/global.h
  src/core/statjob.cpp
  src/core/statjob.h
  src/filewidgets/kdiroperator.cpp
  src/ioslaves/file/file.cpp
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp
  src/widgets/krun.cpp
  src/widgets/paste.cpp
  tests/kioslavetest.cpp
  tests/listjobtest.cpp

To: meven, #frameworks, dfaure, kossebau
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

[Méven Car]

meven updated this revision to Diff 69261.
meven added a comment.


  Move KIO::StatJob::StatDetail(s) to KIO::StatDetail(s), wrapped old details 
metadata code path with KIOCORE_ENABLE_DEPRECATED_SINCE

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25010?vs=69181=69261

BRANCH
  arcpatch-D25010

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

AFFECTED FILES
  autotests/jobremotetest.cpp
  src/core/CMakeLists.txt
  src/core/copyjob.cpp
  src/core/deletejob.cpp
  src/core/directorysizejob.cpp
  src/core/global.h
  src/core/statjob.cpp
  src/core/statjob.h
  src/filewidgets/kdiroperator.cpp
  src/ioslaves/file/file.cpp
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp
  src/widgets/krun.cpp
  src/widgets/paste.cpp
  tests/kioslavetest.cpp
  tests/listjobtest.cpp

To: meven, #frameworks, dfaure, kossebau
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

[David Faure]

dfaure added a comment.


  You could use KIOCORE_ENABLE_DEPRECATED_SINCE(5, 65) to already put in place 
the trigger for the compat code to disappear when KF6 comes around, like you 
did in file_unix.cpp.
  It will also make it easier for the future developer who cleans this up.
  
  I mean deletejob.cpp:401, directorysizejob.cpp:137, those places with a "TODO 
KF6".

INLINE COMMENTS

> meven wrote in listjobtest.cpp:44
> > Does KIO::DefaultDetails make sense beyond stat in the KIO namespace? How 
> > does it match other similar flags?
> 
> It was @kossebau concern as well.
> 
> I am open to suggestion to improve on that, maybe this enum should be in KIO 
> namespace ?
> In the meantime, it was what was made the most sense to me.

Well if it's called KIO::StatDefaultDetails (with the extra "Stat" compared to 
the question above), then it doesn't need to be in KIO::StatJob.

So yes, you could move the enum to the KIO namespace and define it in 
src/core/global.h
This will keep the two jobs a bit more separate (no need to add #include 
"statjob.h" in a few places)

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, dfaure, kossebau
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

[Méven Car]

meven updated this revision to Diff 69181.
meven added a comment.


  Allow retro-compatibility with old details both ways : old kio or old app, 
update comments to ready patch for KF 5.65

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25010?vs=69023=69181

BRANCH
  arcpatch-D25010

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

AFFECTED FILES
  autotests/jobremotetest.cpp
  src/core/copyjob.cpp
  src/core/deletejob.cpp
  src/core/directorysizejob.cpp
  src/core/statjob.cpp
  src/core/statjob.h
  src/filewidgets/kdiroperator.cpp
  src/ioslaves/file/file.cpp
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp
  src/widgets/krun.cpp
  src/widgets/paste.cpp
  tests/kioslavetest.cpp
  tests/listjobtest.cpp

To: meven, #frameworks, dfaure, kossebau
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

[Méven Car]

meven planned changes to this revision.
meven added a comment.


  I don't expect this to make to KF5.64, will update the @since and all.

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, dfaure, kossebau
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

[Méven Car]

meven edited the summary of this revision.

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, dfaure, kossebau
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

[Méven Car]

meven updated this revision to Diff 69023.
meven added a comment.


  Use StatDetail to reduce statx mask, support old details metadata in file 
ioslave, adding metadata statDetails for the new values

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25010?vs=69021=69023

BRANCH
  arcpatch-D25010

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

AFFECTED FILES
  autotests/jobremotetest.cpp
  src/core/copyjob.cpp
  src/core/deletejob.cpp
  src/core/directorysizejob.cpp
  src/core/statjob.cpp
  src/core/statjob.h
  src/filewidgets/kdiroperator.cpp
  src/ioslaves/file/file.cpp
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp
  src/widgets/krun.cpp
  src/widgets/paste.cpp
  tests/kioslavetest.cpp
  tests/listjobtest.cpp

To: meven, #frameworks, dfaure, kossebau
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

[Méven Car]

meven planned changes to this revision.
meven added a comment.


  In D25010#556237 , @dfaure wrote:
  
  > If you change the actual values being sent to kioslaves, then this breaks 
the "wire protocol". I.e. after upgrading KF5 on a live system, kioslaves 
(started by kdeinit5 forking the "old" libkiocore) will receive numbers from 
newly started apps (which link to the "new" libkiocore), that they won't 
understand. Of course a simple `kdeinit5` in a terminal would fix that, but 
most users don't know that. (One idea that has been floating around is to 
detect changes to libKIO.so and other dependencies and auto-restart kdeinit 
when these libs are changed... But, hmm, then already-running apps will send 
old values to newly started kioslaves, problem again).
  
  
  Thank you for reminding me of this concern.
  
  > One solution is to make the enum actually use the old values (0, 2, 3) by 
doing careful mapping. That's what I thought we would do, but I see you went 
for a more flexible approach where apps can pick the exact groups of fields 
they want.
  > 
  > So another solution would be to use a different metadata key and send the 
old key (with the old value, at least a close approximation of it) for 
compatibility purposes.
  
  That's the solution I would prefer : it would allow me to add a "TODO KF6 
remove to the old metadata key", breaking the wire format should be fine then, 
so we end up with the best state for KF6.
  
  Next items:
  
  - Add compatibility to older KIO version on the wire
  - match the flags to statx flags to avoid paying for unwanted fields down to 
the syscall.

INLINE COMMENTS

> statjob.h:53
> +/// Filename, access, type, size, linkdest
> +Basic = 0x1,
> +/// uid, gid

I am open to suggestion here :

1. Should it be more granular ?

2. Are names KIO-ish ?

@kossebau :

> On a first look, the names seems very short & generic to me, having some 
> other name element might avoid semantic collisions perhaps. No idea yet, not 
> looked further.

> dfaure wrote in file.cpp:896
> Did you know you can just write `if (details & KIO::StatJob::Basic)`?
> This is more C++-like, which is probably more future-proof if one day don't 
> use QFlags anymore for these types of things.
> 
> Or did you use testFlag() for a specific reason?

I used testFlag because i was documented, and I still have to familiar myself 
with C/C++.

> dfaure wrote in listjobtest.cpp:44
> It's a bit weird to use a StatJob enum in the ListJob class. But then again, 
> it is about the stat() done by listing So this is OK, I guess.

> Does KIO::DefaultDetails make sense beyond stat in the KIO namespace? How 
> does it match other similar flags?

It was @kossebau concern as well.

I am open to suggestion to improve on that, maybe this enum should be in KIO 
namespace ?
In the meantime, it was what was made the most sense to me.

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, dfaure, kossebau
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

[Méven Car]

meven updated this revision to Diff 69021.
meven marked 3 inline comments as done.
meven added a comment.


  Avoid using short and testFlag

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25010?vs=68975=69021

BRANCH
  arcpatch-D25010

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

AFFECTED FILES
  autotests/jobremotetest.cpp
  src/core/copyjob.cpp
  src/core/deletejob.cpp
  src/core/directorysizejob.cpp
  src/core/statjob.cpp
  src/core/statjob.h
  src/filewidgets/kdiroperator.cpp
  src/ioslaves/file/file.cpp
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp
  src/widgets/krun.cpp
  src/widgets/paste.cpp
  tests/kioslavetest.cpp
  tests/listjobtest.cpp

To: meven, #frameworks, dfaure, kossebau
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

[David Faure]

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


  If you change the actual values being sent to kioslaves, then this breaks the 
"wire protocol". I.e. after upgrading KF5 on a live system, kioslaves (started 
by kdeinit5 forking the "old" libkiocore) will receive numbers from newly 
started apps (which link to the "new" libkiocore), that they won't understand. 
Of course a simple `kdeinit5` in a terminal would fix that, but most users 
don't know that. (One idea that has been floating around is to detect changes 
to libKIO.so and other dependencies and auto-restart kdeinit when these libs 
are changed... But, hmm, then already-running apps will send old values to 
newly started kioslaves, problem again).
  
  One solution is to make the enum actually use the old values (0, 2, 3) by 
doing careful mapping. That's what I thought we would do, but I see you went 
for a more flexible approach where apps can pick the exact groups of fields 
they want.
  
  So another solution would be to use a different metadata key and send the old 
key (with the old value, at least a close approximation of it) for 
compatibility purposes.

INLINE COMMENTS

> file.cpp:895
>  assert(entry.count() == 0); // by contract :-)
> -switch (details) {
> -case 0:
> +short entries = 0;
> +if (details.testFlag(KIO::StatJob::Basic)) {

int is actually faster on most CPUs, and reserve() will convert it to an int 
anyway, so it might as well be an int from the beginning.

> file.cpp:896
> +short entries = 0;
> +if (details.testFlag(KIO::StatJob::Basic)) {
>  // filename, access, type, size, linkdest

Did you know you can just write `if (details & KIO::StatJob::Basic)`?
This is more C++-like, which is probably more future-proof if one day don't use 
QFlags anymore for these types of things.

Or did you use testFlag() for a specific reason?

> listjobtest.cpp:44
>  job->setUiDelegate(nullptr);
> -job->addMetaData(QStringLiteral("details"), QStringLiteral("2")); // 
> Default is 2 which means all details. 0 means just a few essential fields 
> (KIO::UDSEntry::UDS_NAME, KIO::UDSEntry::UDS_FILE_TYPE and 
> KIO::UDSEntry::UDS_LINK_DEST if it is a symbolic link. Not provided otherwise.
> +job->addMetaData(QStringLiteral("details"), 
> QString::number(KIO::StatJob::StatDefaultDetails));
>  

It's a bit weird to use a StatJob enum in the ListJob class. But then again, it 
is about the stat() done by listing So this is OK, I guess.

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, dfaure, kossebau
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by StatJob

[Méven Car]

meven retitled this revision from "[StatJob] Use A QFlag to specify the details 
returned by statJob" to "[StatJob] Use A QFlag to specify the details returned 
by StatJob".

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, dfaure, kossebau
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by statJob

[Méven Car]

meven edited the test plan for this revision.

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, dfaure, kossebau
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by statJob

[Méven Car]

meven marked 3 inline comments as done.

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, dfaure, kossebau
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by statJob

[Méven Car]

meven updated this revision to Diff 68975.
meven added a comment.


  Move KIO::StatDefaultDetails to KIO::StatJob::StatDefaultDetails

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25010?vs=68973=68975

BRANCH
  arcpatch-D25010

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

AFFECTED FILES
  autotests/jobremotetest.cpp
  src/core/copyjob.cpp
  src/core/deletejob.cpp
  src/core/directorysizejob.cpp
  src/core/statjob.cpp
  src/core/statjob.h
  src/filewidgets/kdiroperator.cpp
  src/ioslaves/file/file.cpp
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp
  src/widgets/krun.cpp
  src/widgets/paste.cpp
  tests/kioslavetest.cpp
  tests/listjobtest.cpp

To: meven, #frameworks, dfaure, kossebau
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by statJob

[Méven Car]

meven added a comment.


  In D25010#555890 , @kossebau wrote:
  
  > Now that is a quick turn-around, +1 for doing the work :)
  >
  > No time myself to look at this closely the next days, also not that much 
into KIO, but here some quick feedback with an API police hat on.
  
  
  Your comments are already of great value, given this needs some polishing.
  
  > Would be also good to have some patches which make use of the new API, so 
one could better judge the usefulness.
  
  The interest here is to have a modern API and better documented that is was 
and more features.
  In the meantime I have ported the rest of KIO to use it.

INLINE COMMENTS

> kossebau wrote in jobremotetest.cpp:70
> As reader of this code here alone, I wonder what KIO::StatJob::Basic means. 
> To understand what this code does, I would first have to look at the API dox, 
> not good.
> So possibly Basic should get a different name, at least contain "Detail" term 
> perhaps. "Basic" also needs context to have semantics, I could e.g. not tell 
> instantly what basic details are. So perhaps needs to be more expliciti here.

I guess so, "Basic" here lacks context.
But the documentation will be much faster to get than before with the 0 field.

> kossebau wrote in statjob.h:181
> I wonder if this should not be rather a member of StatJob, instead of being 
> on generic KIO namespace level.
> It feels unbalanced to have the enum being in the class, but a util flag set 
> not.

Great suggestion !
I wanted to do that but I just did not find a way to have StatDefaultDetails 
part of StatJob namespace given 
`Q_DECLARE_OPERATORS_FOR_FLAGS(StatJob::Details)` must be declared outside of 
StatJob class.

Adding it to the enum should just work, working on it.

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, dfaure, kossebau
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by statJob

[Friedrich W. H. Kossebau]

kossebau added inline comments.

INLINE COMMENTS

> kossebau wrote in statjob.h:181
> I wonder if this should not be rather a member of StatJob, instead of being 
> on generic KIO namespace level.
> It feels unbalanced to have the enum being in the class, but a util flag set 
> not.

Actually, this could be part of the StatJob::StatDetail enum, no? Having 
combinations of lfags in the enum itself (to be used as shortcuts) is usual 
practice and also should not conflict with Q_DECLARE_FLAGS, IIRC (would need to 
check).

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, dfaure, kossebau
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by statJob

[Méven Car]

meven updated this revision to Diff 68973.
meven added a comment.


  Fix jobs details argument passing and fix setDetails(int)

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25010?vs=68971=68973

BRANCH
  arcpatch-D25010

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

AFFECTED FILES
  autotests/jobremotetest.cpp
  src/core/copyjob.cpp
  src/core/deletejob.cpp
  src/core/directorysizejob.cpp
  src/core/statjob.cpp
  src/core/statjob.h
  src/filewidgets/kdiroperator.cpp
  src/ioslaves/file/file.cpp
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp
  src/widgets/krun.cpp
  src/widgets/paste.cpp
  tests/kioslavetest.cpp
  tests/listjobtest.cpp

To: meven, #frameworks, dfaure, kossebau
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by statJob

[Friedrich W. H. Kossebau]

kossebau added inline comments.

INLINE COMMENTS

> jobremotetest.cpp:70
>  {
> -KIO::Job *job = KIO::stat(url, KIO::StatJob::DestinationSide, 0, 
> KIO::HideProgressInfo);
> +KIO::Job *job = KIO::stat(url, KIO::StatJob::DestinationSide, 
> KIO::StatJob::Basic, KIO::HideProgressInfo);
>  job->setUiDelegate(nullptr);

As reader of this code here alone, I wonder what KIO::StatJob::Basic means. To 
understand what this code does, I would first have to look at the API dox, not 
good.
So possibly Basic should get a different name, at least contain "Detail" term 
perhaps. "Basic" also needs context to have semantics, I could e.g. not tell 
instantly what basic details are. So perhaps needs to be more expliciti here.

> copyjob.cpp:365
>  const QUrl dest = m_asMethod ? m_dest.adjusted(QUrl::RemoveFilename) : 
> m_dest;
> -KIO::Job *job = KIO::stat(dest, StatJob::DestinationSide, 2, 
> KIO::HideProgressInfo);
> +KIO::Job *job = KIO::stat(dest, StatJob::DestinationSide, 
> KIO::StatDefaultDetails, KIO::HideProgressInfo);
>  qCDebug(KIO_COPYJOB_DEBUG) << "CopyJob: stating the dest" << m_dest;

Fear the same as said for Basic is true with Default. I would prefer explicit 
flags here as code reader.

> statjob.h:181
> +/// @since 5.64
> +constexpr static StatJob::StatDetails StatDefaultDetails = 
> StatJob::StatDetail::Basic | StatJob::StatDetail::User | 
> StatJob::StatDetail::Time | StatJob::StatDetail::Acl | 
> StatJob::StatDetail::ResolveSymlink;
> +

I wonder if this should not be rather a member of StatJob, instead of being on 
generic KIO namespace level.
It feels unbalanced to have the enum being in the class, but a util flag set 
not.

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, dfaure, kossebau
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by statJob

[Méven Car]

meven updated this revision to Diff 68971.
meven marked an inline comment as done.
meven added a comment.


  Wrap setDetails with #if KIOCORE_ENABLE_DEPRECATED_SINCE, use the new API 
throughout the rest of KIO

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25010?vs=68969=68971

BRANCH
  arcpatch-D25010

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

AFFECTED FILES
  autotests/jobremotetest.cpp
  src/core/copyjob.cpp
  src/core/deletejob.cpp
  src/core/statjob.cpp
  src/core/statjob.h
  src/filewidgets/kdiroperator.cpp
  src/ioslaves/file/file.cpp
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp
  src/widgets/krun.cpp
  src/widgets/paste.cpp
  tests/kioslavetest.cpp

To: meven, #frameworks, dfaure, kossebau
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by statJob

[Friedrich W. H. Kossebau]

kossebau added inline comments.

INLINE COMMENTS

> statjob.h:96
> +
>  /**
>   * Selects the level of @p details we want.

All deprecated API should be also wrapped in visibility controls., so same 
#if/#endif also here.

Basic structure:

  if deprecated api should be visible to compiler & Co (e.g. docs generator)
  API docs
  warning attribute if should be emitted 
  method declaration
  endif

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, dfaure, kossebau
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by statJob

[Méven Car]

meven updated this revision to Diff 68969.
meven marked 8 inline comments as done.
meven added a comment.


  Rename StatJob::Detail to StatJob::StatDetail, KIO::DefaultDetails to 
KIO::StatDefaultDetails, add deprecation doc and macro

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25010?vs=68893=68969

BRANCH
  arcpatch-D25010

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

AFFECTED FILES
  src/core/statjob.cpp
  src/core/statjob.h
  src/ioslaves/file/file.cpp
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp

To: meven, #frameworks, dfaure, kossebau
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by statJob

[Friedrich W. H. Kossebau]

kossebau added a comment.


  Now that is a quick turn-around, +1 for doing the work :)
  
  No time myself to look at this closely the next days, also not that much into 
KIO, but here some quick feedback with an API police hat on.
  
  Would be also good to have some patches which make use of the new API, so one 
could better judge the usefulness.

INLINE COMMENTS

> statjob.h:50
>  
> +enum Detail {
> +/// Filename, access, type, size, linkdest

Is there other KIO API which has similar enums, where some consistency would be 
good to have?

On a first look, the names seems very short & generic to me, having some other 
name element might avoid semantic collisions perhaps. No idea yet, not looked 
further.

> statjob.h:50
>  
> +enum Detail {
> +/// Filename, access, type, size, linkdest

Please add @since comment

> statjob.h:91
> + * Selects the level of @p details we want.
> + */
> +void setDetails(StatJob::Details details);

Please add @since

> statjob.h:103
>   */
>  void setDetails(short int details);
>  

Not deprecated now?

> statjob.h:174
> +
> +constexpr static StatJob::Details DefaultDetails = StatJob::Detail::Basic | 
> StatJob::Detail::User | StatJob::Detail::Time | StatJob::Detail::Acl | 
> StatJob::Detail::ResolveSymlink;
> +

Does KIO::DefaultDetails make sense beyond stat in the KIO namespace? How does 
it match other similar flags?
Perhaps should be more bound to "stat" by the name.
Please also add documentation, incl. "@since"

> statjob.h:206
> + * @return the job handling the operation.
> + */
> +KIOCORE_EXPORT StatJob *stat(const QUrl , KIO::StatJob::StatSide side,

Please add @since

> statjob.h:209
> + StatJob::Details details, JobFlags flags = 
> DefaultFlags);
>  /**
>   * Find all details for one file or directory.

Please wrap the deprecated API call (incl. API dox comment) with visibilty 
controlling #if/#endif., so here

  #if KIOCORE_ENABLE_DEPRECATED_SINCE(5, 64)

> statjob.h:234
>   * @param flags Can be HideProgressInfo here
>   * @return the job handling the operation.
>   */

Please add "@deprecated Since 5.64. Use KIO::stat(const QUrl &, 
KIO::StatJob::StatSide, StatJob::Details int, JobFlags)"

For all the recommended details to add when deprecating API (also with the new 
deprecation macros), please see
https://community.kde.org/Policies/Library_Code_Policy#Deprecation_of_API

> statjob.h:239
>   short int details, JobFlags flags = 
> DefaultFlags);
>  
>  #if KIOCORE_ENABLE_DEPRECATED_SINCE(4, 0)

#endif

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, dfaure, kossebau
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by statJob

[Méven Car]

meven updated this revision to Diff 68893.
meven added a comment.


  Fix wrong comparison

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25010?vs=68892=68893

BRANCH
  stat-qflags

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

AFFECTED FILES
  src/core/statjob.cpp
  src/core/statjob.h
  src/ioslaves/file/file.cpp
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp

To: meven, #frameworks, dfaure, kossebau
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by statJob

[Méven Car]

meven retitled this revision from "Use A QFlag to specify the details returned 
by statJob" to "[StatJob] Use A QFlag to specify the details returned by 
statJob".
meven added reviewers: Frameworks, dfaure, kossebau.

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, dfaure, kossebau
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25010: [StatJob] Use A QFlag to specify the details returned by statJob

[Méven Car]

meven updated this revision to Diff 68892.
meven added a comment.


  Amend commit message

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25010?vs=68890=68892

BRANCH
  stat-qflags

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

AFFECTED FILES
  src/core/statjob.cpp
  src/core/statjob.h
  src/ioslaves/file/file.cpp
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp

To: meven, #frameworks, dfaure, kossebau
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns