D17816: Support for xattrs on kio copy/move

[Anthony Fieroni]

anthonyfieroni added inline comments.

INLINE COMMENTS

> copyxattrjob.cpp:98-111
> + const int bsrc_fd = open(m_bsrc, 0);
> + if (bsrc_fd < 0)
> + {
> +q->setErrorText(QLatin1String("failed to obtain file descriptor of 
> source during xattr copy"));
> +q->emitResult();
> + }
> +const QByteArray destination = QFile::encodeName(m_dest.toLocalFile());

indentation

> copyxattrjob.cpp:156-158
> +vallen = fgetxattr(bsrc_fd, key.constData(), value.data(), valuelen, 
> 0, 0);
> +#elif HAVE_SYS_EXTATTR
> +vallen = extattr_get_file(m_bsrc, EXTATTR_NAMESPACE_USER, 
> key.constData(), value.data(), valuelen);

valuelen there is no vallen

REPOSITORY
  R241 KIO

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

To: tmarshall, dfaure, chinmoyr, bruns, #frameworks, cochise
Cc: anthonyfieroni, tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, 
funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, spoorun, 
nicolasfella, kde-frameworks-devel, LeGast00n, GB_2, michaelh

KDE CI: Frameworks » kcoreaddons » kf5-qt5 FreeBSDQt5.13 - Build # 81 - Still Unstable!

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks/job/kcoreaddons/job/kf5-qt5%20FreeBSDQt5.13/81/
 Project:
kf5-qt5 FreeBSDQt5.13
 Date of build:
Wed, 25 Dec 2019 01:52:04 +
 Build duration:
2 min 28 sec and counting
   JUnit Tests
  Name: projectroot Failed: 1 test(s), Passed: 26 test(s), Skipped: 0 test(s), Total: 27 test(s)Failed: projectroot.autotests.kdirwatch_inotify_unittest

D17816: Support for xattrs on kio copy/move

[Cochise César]

cochise added a comment.


  Many thanks to getting this live again, @tmarshall . A year ago my vacations 
ended and I can't get this ready to ship.
  
  > Everything works for me if I replace the exec call with a start call. 
Currently the result of the exec call is thrown away as the job itself does the 
error handling and there isn't much to do if the xattrs can't be copied for 
whatever reason.
  
  If I recall correctly, `start()` and `exec()` works to me on manual tests, 
but `start()` broke the automated tests. As the maintainer vetoed a sync 
solution, but the async one cant pass the tests, the patch stalled.
  
  If all tests are passing to you with a async call, I think it's shipabble.

REPOSITORY
  R241 KIO

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

To: tmarshall, dfaure, chinmoyr, bruns, #frameworks, cochise
Cc: tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, funkybomber, abika, 
pino, davidedmundson, ngraham, atha.kane, spoorun, nicolasfella, 
kde-frameworks-devel, LeGast00n, GB_2, michaelh

D26191: Add support for FileJob->truncate() in smb/sftp slaves

[Alexander Saoutkin]

feverfew marked 3 inline comments as done.

REPOSITORY
  R320 KIO Extras

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

To: feverfew, sitter, dfaure, fvogt
Cc: kde-frameworks-devel, kfm-devel, ngraham, pberestov, iasensio, fprice, 
LeGast00n, MrPepe, fbampaloukas, alexde, GB_2, Codezela, feverfew, meven, 
michaelh, spoorun, navarromorales, firef, andrebarros, bruns, emmanuelp, 
mikesomov

D23384: Adding support for mounting KIOFuse URLs for applications that don't use KIO

[Nathaniel Graham]

ngraham added a comment.


  @dfaure is this good to go now?

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: alexde, broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, 
LeGast00n, GB_2, michaelh, bruns

D17816: Support for xattrs on kio copy/move

[Thomas Marshall]

tmarshall added inline comments.

INLINE COMMENTS

> dfaure wrote in copyjob.cpp:1119
> That sounds racy; the subjob might not be finished by the time the main job 
> is finished, if you just "start and forget".
> 
> I cannot accept this commit with an exec() in here. The easy and modern way 
> to make this async is just a connect and a lambda (which contains the rest of 
> the code here).
> 
> I have to wonder though: do we have a fast way to determine whether we 
> actually need the additional subjob? (to avoid making this slower on systems 
> -- or files -- without xattr)
> 
> Hmm, or a much bigger architectural question: why doesn't kio_file copy the 
> xattr as part of FileProtocol::copy(), as it already does with e.g. 
> permissions and ACLs, instead of doing this in a separate job?

Thanks to @ahmad78 for chatting with me about this in irc.

Everything works for me if I replace the `exec` call with a `start` call. 
Currently the result of the exec call is thrown away as the job itself does the 
error handling and there isn't much to do if the xattrs can't be copied for 
whatever reason.

As for why this is a different job, that was a design decision by the original 
author. It seems like a decent enough way forward to me as it leans on the side 
of modularity. The applications of xattrs are so broad that it's hard to say 
what they will be used for in the future or what the desired functionality 
might be. Having a separate job will allow for more flexibility regarding 
xattrs going forward.

In short, I don't really think it hurts to have a separate job and there is 
certainly the potential that it comes in handy.

REPOSITORY
  R241 KIO

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

To: tmarshall, dfaure, chinmoyr, bruns, #frameworks, cochise
Cc: tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, funkybomber, abika, 
pino, davidedmundson, ngraham, atha.kane, spoorun, nicolasfella, 
kde-frameworks-devel, LeGast00n, GB_2, michaelh

D26191: Add support for FileJob->truncate() in smb/sftp slaves

[Alexander Saoutkin]

feverfew updated this revision to Diff 72155.
feverfew added a comment.


  Avoid code duplication

REPOSITORY
  R320 KIO Extras

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26191?vs=72154&id=72155

BRANCH
  arcpatch-D26191

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

AFFECTED FILES
  CMakeLists.txt
  sftp/kio_sftp.cpp
  sftp/kio_sftp.h
  smb/kio_smb.h
  smb/kio_smb_file.cpp

To: feverfew, sitter, dfaure, fvogt
Cc: kde-frameworks-devel, kfm-devel, ngraham, pberestov, iasensio, fprice, 
LeGast00n, MrPepe, fbampaloukas, alexde, GB_2, Codezela, feverfew, meven, 
michaelh, spoorun, navarromorales, firef, andrebarros, bruns, emmanuelp, 
mikesomov

D26191: Add support for FileJob->truncate() in smb/sftp slaves

[Alexander Saoutkin]

feverfew updated this revision to Diff 72154.
feverfew added a comment.


  Avoid free(NULL)

REPOSITORY
  R320 KIO Extras

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26191?vs=72104&id=72154

BRANCH
  arcpatch-D26191

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

AFFECTED FILES
  CMakeLists.txt
  sftp/kio_sftp.cpp
  sftp/kio_sftp.h
  smb/kio_smb.h
  smb/kio_smb_file.cpp

To: feverfew, sitter, dfaure, fvogt
Cc: kde-frameworks-devel, kfm-devel, ngraham, pberestov, iasensio, fprice, 
LeGast00n, MrPepe, fbampaloukas, alexde, GB_2, Codezela, feverfew, meven, 
michaelh, spoorun, navarromorales, firef, andrebarros, bruns, emmanuelp, 
mikesomov

D26197: Display fully qualified class/namespace name as page header

[Elvis Angelaccio]

elvisangelaccio added a reviewer: ochurlaud.

REPOSITORY
  R264 KApiDox

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

To: davidre, ochurlaud
Cc: ngraham, aacid, jucato, kde-frameworks-devel, kde-doc-english, LeGast00n, 
gennad, fbampaloukas, GB_2, michaelh, bruns, skadinna

D26210: [KCoreAddons] Port QRegExp to QRegularExpression

[Ahmad Samir]

ahmadsamir added inline comments.

INLINE COMMENTS

> apol wrote in klistopenfilesjob_unix.cpp:69
> splitRef?

I didn't even know there was a splitRef...

It'll have to be something like:

  QVector pidList = 
out.splitRef(QRegularExpression(QStringLiteral("\\s+")), 
QString::SkipEmptyParts);
  // remove duplicates
  std::unique(pidList.begin(), pidList.end());

I'd better do it in a separate commit, given it's really going to change stuff 
here (that compiles BTW, so I know it works).

Interestingly running `lsof | wc -l` on my system gives 162709; so yeah, we 
should definitely replace QList with QVector and QString with QStringRef here. 
:D

REPOSITORY
  R244 KCoreAddons

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

To: ahmadsamir, #frameworks, mpyne, dfaure, apol
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26211: Port QRegExp to QRegularExpression

[Ahmad Samir]

ahmadsamir updated this revision to Diff 72149.
ahmadsamir added a comment.


  Use capturedRef(), cheaper

REPOSITORY
  R246 Sonnet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26211?vs=72148&id=72149

BRANCH
  l-qregularexpression (branched from master)

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

AFFECTED FILES
  data/parsetrigrams.cpp

To: ahmadsamir, cullmann, apol
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26211: Port QRegExp to QRegularExpression

[Aleix Pol Gonzalez]

apol added a comment.


  Looks good otherwise

INLINE COMMENTS

> parsetrigrams.cpp:59
> +if (match.hasMatch()) {
> +models[fname][line.left(3)] = match.captured(2).toInt();
>  }

capturedRef

REPOSITORY
  R246 Sonnet

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

To: ahmadsamir, cullmann, apol
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26210: [KCoreAddons] Port QRegExp to QRegularExpression

[Aleix Pol Gonzalez]

apol added a comment.


  Looks good overall

INLINE COMMENTS

> klistopenfilesjob_unix.cpp:69
>  const QString out(QString::fromLocal8Bit(lsofProcess.readAll()));
> -QStringList pidList = out.split(QRegExp(QStringLiteral("\\s+")), 
> QString::SkipEmptyParts);
> +QStringList pidList = 
> out.split(QRegularExpression(QStringLiteral("\\s+")), 
> QString::SkipEmptyParts);
>  pidList.removeDuplicates();

splitRef?

REPOSITORY
  R244 KCoreAddons

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

To: ahmadsamir, #frameworks, mpyne, dfaure, apol
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26197: Display fully qualified class/namespace name as page header

[Nathaniel Graham]

ngraham added a comment.


  Looks sensible to me too FWIW.

REPOSITORY
  R264 KApiDox

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

To: davidre
Cc: ngraham, aacid, jucato, kde-frameworks-devel, kde-doc-english, LeGast00n, 
gennad, fbampaloukas, GB_2, michaelh, bruns, skadinna

D26211: Port QRegExp to QRegularExpression

[Ahmad Samir]

ahmadsamir created this revision.
ahmadsamir added reviewers: cullmann, apol.
Herald added a project: Frameworks.
ahmadsamir requested review of this revision.

TEST PLAN
  make && ctest

REPOSITORY
  R246 Sonnet

BRANCH
  l-qregularexpression (branched from master)

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

AFFECTED FILES
  data/parsetrigrams.cpp

To: ahmadsamir, cullmann, apol
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

KDE CI: Frameworks » kio » kf5-qt5 SUSEQt5.12 - Build # 371 - Fixed!

[CI System]

BUILD SUCCESS
 Build URL
https://build.kde.org/job/Frameworks/job/kio/job/kf5-qt5%20SUSEQt5.12/371/
 Project:
kf5-qt5 SUSEQt5.12
 Date of build:
Tue, 24 Dec 2019 12:30:44 +
 Build duration:
10 min and counting
   BUILD ARTIFACTS
  acc/KF5KIO-5.66.0.xmllogs/KF5KIO/5.66.0/log.txt
   JUnit Tests
  Name: (root) Failed: 0 test(s), Passed: 1 test(s), Skipped: 0 test(s), Total: 1 test(s)Name: projectroot Failed: 0 test(s), Passed: 53 test(s), Skipped: 0 test(s), Total: 53 test(s)Name: projectroot.autotests Failed: 0 test(s), Passed: 6 test(s), Skipped: 0 test(s), Total: 6 test(s)Name: projectroot.src.ioslaves.trash Failed: 0 test(s), Passed: 1 test(s), Skipped: 0 test(s), Total: 1 test(s)Name: projectroot.src.kpasswdserver Failed: 0 test(s), Passed: 1 test(s), Skipped: 0 test(s), Total: 1 test(s)
   Cobertura Report
  
   Project Coverage Summary
  
   Name
  PackagesFilesClassesLinesConditionalsCobertura Coverage Report67%
(24/36)67%
(270/405)67%
(270/405)56%
(34650/61744)40%
(17607/43600)Coverage Breakdown by Package
Name
   FilesClassesLinesConditionalsautotests100%
(58/58)100%
(58/58)95%
(9643/10105)47%
(4502/9556)autotests.http100%
(5/5)100%
(5/5)99%
(580/581)68%
(108/160)autotests.kcookiejar100%
(1/1)100%
(1/1)91%
(179/197)72%
(49/68)src100%
(1/1)100%
(1/1)86%
(6/7)67%
(4/6)src.core88%
(104/118)88%
(104/118)59%
(8657/14562)51%
(4524/8826)src.core.kssl100%
(1/1)100%
(1/1)40%
(35/88)50%
(3/6)src.filewidgets68%
(26/38)68%
(26/38)56%
(4661/8294)43%
(2029/4748)src.gui100%
(2/2)100%
(2/2)94%
(102/108)74%
(49/66)src.ioslaves.file100%
(7/7)100%
(7/7)54%
(678/1249)40%
(392/971)src.ioslaves.file.kauth0%
(0/2)0%
(0/2)0%
(0/168)0%
(0/89)src.ioslaves.ftp100%
(2/2)100%
(2/2)47%
(644/1370)37%
(525/1420)src.ioslaves.help0%
(0/5)0%
(0/5)0%
(0/247)0%
(0/148)src.ioslaves.http88%
(7/8)88%
(7/8)42%
(1796/4288)36%
(1309/3636)src.ioslaves.http.kcookiejar40%
(2/5)40%
(2/5)47%
(631/1330)56%
(576/1027)src.ioslaves.remote100%
(2/2)100%
(2/2)27%
(73/267)8%
(14/184)src.ioslaves.remote.kdedmodule0%
(0/2)0%
(0/2)0%

KDE CI: Frameworks » kio » kf5-qt5 FreeBSDQt5.13 - Build # 238 - Still Unstable!

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks/job/kio/job/kf5-qt5%20FreeBSDQt5.13/238/
 Project:
kf5-qt5 FreeBSDQt5.13
 Date of build:
Tue, 24 Dec 2019 12:30:45 +
 Build duration:
9 min 14 sec and counting
   JUnit Tests
  Name: projectroot Failed: 3 test(s), Passed: 49 test(s), Skipped: 0 test(s), Total: 52 test(s)Failed: projectroot.autotests.kiocore_kmountpointtestFailed: projectroot.autotests.kiowidgets_kdirlistertestFailed: projectroot.autotests.kiowidgets_kdirmodeltestName: projectroot.autotests Failed: 0 test(s), Passed: 6 test(s), Skipped: 0 test(s), Total: 6 test(s)Name: projectroot.src.ioslaves.trash Failed: 0 test(s), Passed: 1 test(s), Skipped: 0 test(s), Total: 1 test(s)Name: projectroot.src.kpasswdserver Failed: 0 test(s), Passed: 1 test(s), Skipped: 0 test(s), Total: 1 test(s)

KDE CI: Frameworks » kio » kf5-qt5 SUSEQt5.13 - Build # 253 - Fixed!

[CI System]

BUILD SUCCESS
 Build URL
https://build.kde.org/job/Frameworks/job/kio/job/kf5-qt5%20SUSEQt5.13/253/
 Project:
kf5-qt5 SUSEQt5.13
 Date of build:
Tue, 24 Dec 2019 12:30:45 +
 Build duration:
9 min 56 sec and counting
   BUILD ARTIFACTS
  acc/KF5KIO-5.66.0.xmllogs/KF5KIO/5.66.0/log.txt
   JUnit Tests
  Name: (root) Failed: 0 test(s), Passed: 1 test(s), Skipped: 0 test(s), Total: 1 test(s)Name: projectroot Failed: 0 test(s), Passed: 53 test(s), Skipped: 0 test(s), Total: 53 test(s)Name: projectroot.autotests Failed: 0 test(s), Passed: 6 test(s), Skipped: 0 test(s), Total: 6 test(s)Name: projectroot.src.ioslaves.trash Failed: 0 test(s), Passed: 1 test(s), Skipped: 0 test(s), Total: 1 test(s)Name: projectroot.src.kpasswdserver Failed: 0 test(s), Passed: 1 test(s), Skipped: 0 test(s), Total: 1 test(s)
   Cobertura Report
  
   Project Coverage Summary
  
   Name
  PackagesFilesClassesLinesConditionalsCobertura Coverage Report67%
(24/36)67%
(270/405)67%
(270/405)56%
(34658/61744)40%
(17609/43596)Coverage Breakdown by Package
Name
   FilesClassesLinesConditionalsautotests100%
(58/58)100%
(58/58)95%
(9645/10105)47%
(4502/9556)autotests.http100%
(5/5)100%
(5/5)99%
(580/581)68%
(108/160)autotests.kcookiejar100%
(1/1)100%
(1/1)91%
(179/197)72%
(49/68)src100%
(1/1)100%
(1/1)86%
(6/7)67%
(4/6)src.core88%
(104/118)88%
(104/118)59%
(8662/14562)51%
(4523/8826)src.core.kssl100%
(1/1)100%
(1/1)40%
(35/88)50%
(3/6)src.filewidgets68%
(26/38)68%
(26/38)56%
(4662/8294)43%
(2028/4744)src.gui100%
(2/2)100%
(2/2)94%
(102/108)74%
(49/66)src.ioslaves.file100%
(7/7)100%
(7/7)54%
(678/1249)40%
(392/971)src.ioslaves.file.kauth0%
(0/2)0%
(0/2)0%
(0/168)0%
(0/89)src.ioslaves.ftp100%
(2/2)100%
(2/2)47%
(644/1370)37%
(525/1420)src.ioslaves.help0%
(0/5)0%
(0/5)0%
(0/247)0%
(0/148)src.ioslaves.http88%
(7/8)88%
(7/8)42%
(1796/4288)36%
(1309/3636)src.ioslaves.http.kcookiejar40%
(2/5)40%
(2/5)47%
(631/1330)56%
(576/1027)src.ioslaves.remote100%
(2/2)100%
(2/2)27%
(73/267)8%
(14/184)src.ioslaves.remote.kdedmodule0%
(0/2)0%
(0/2)0%
  

D21783: Show more details in warning dialog shown before starting a privileged operation

[Chinmoy Ranjan Pradhan]

chinmoyr added a comment.


  In D21783#581781 , @dfaure wrote:
  
  > @chinmoyr This change also breaks the corresponding unittest in CI, please 
take a look.
  >  
https://build.kde.org/job/Frameworks/job/kio/job/kf5-qt5%20SUSEQt5.12/368/testReport/junit/projectroot/autotests/kiocore_privilegejobtest/
  
  
  Fixed. https://commits.kde.org/kio/a5b669263573dd7976c14f06b5c13f4da55dc6f0

REPOSITORY
  R241 KIO

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

To: chinmoyr, #vdg, #frameworks, dfaure
Cc: bcooksley, mreeves, ngraham, kde-frameworks-devel, LeGast00n, GB_2, 
michaelh, bruns

Exemptions to try KF "grow" vs. consistent experience (Re: Submitting Grantlee as a KF5 Framework)

[Friedrich W. H. Kossebau]

Am Montag, 23. Dezember 2019, 09:57:57 CET schrieb Volker Krause:
> On Sunday, 22 December 2019 09:46:02 CET Dominik Haumann wrote:
> > Hi all,
> > 
> > in any case, maybe the discussed points should go to the KF6 workboard?
> > https://phabricator.kde.org/project/view/310/
> > 
> > I indeed believe that consistency in the KF5 world is an important
> > feature,
> > so Friedrich does have a point here. Other framework additions had to
> > adapt
> > as well (what comes to my mind is renaming of KQuickCharts or
> > KCalendarCore).
> 
> There is one important difference between KCalendarCore/KQuickCharts/etc and
> Grantlee/QKeychain/etc though. The former had no previous release promising
> a public API and therefore only KDE-internal users. The latter have such
> releases and guarantees, and a significant KDE-external user base. That's
> what makes me consider a transitional pragmatic exemption from certain
> conventions, if we assume it would help to grow our external user base, and
> thus the pool of potential contributors.

Having to make exemptions shows a principal design flaw though: if the idea is 
to pull in more libraries into KF, incl. some with previous releases & thus 
existing userbase hoping on longer-term stable ABI, the same will also happen 
in the KF6 series. And even for the currently discussed two libs Grantlee & 
QKeychain it means at least 1 1/2 years & longer (assuming KF 6.0.0 is coming 
then, and see how long kdelibs4 survived) for being just "exemptions".
It's rather that the "exemptions" become part of the rules that way.

And this would add exceptions which have to be found out about on a case-by-
case base, as nothing in the individual name suggests which KF modules follow 
all KF patterns and which not. Who in some wees remembers which modules are 
exceptions and which not? So this makes things also for current KF modules 
more complex and thus KF a worse meta-product.
More complex and worse for all of users, support people & contributors.

Ruining the current consistent rules for some hunt on some bigger numbers of 
"external user base" of KF by adding more libraries might result in a net loss 
of users though, as KF would get more confusing and thus less interesting.
I guess at least I am not the only one who prefers simple & thus easy things 
to create solutions from :)

So, IMHO if libraries do not follow KF rules, they should not use the 
"KF"/"KDE Frameworks" label. Anything else just blurs the concepts and lowers 
the quality of this meta-product.

My 2 cents as mainly KF user and only casual contributor :)

Cheers
Friedrich

PS: And yes, even current KF set of libs has some pattern issues which confuse 
and thus steal time & joy now and then, so ideally would be fixed.
Like KSyntaxHighlighting having a non-pattern repo name "syntax-highlighting" 
still, where one expects it to be "ksyntaxhighlighting".
Or modemmanager-qt/networkmanager-qt/bluez-qt being off the KDE naming 
pattern, setting them apart a bit, not feeling full kdeframeworkish.

D26106: Port QRegExp to QRegularExpression

[Ahmad Samir]

ahmadsamir added a task: T12279: Port frameworks away from QRegExp.

REPOSITORY
  R269 BluezQt

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

To: ahmadsamir, drosca, #frameworks, apol
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26099: Port QRegExp to QRegularExpression

[Ahmad Samir]

ahmadsamir added a task: T12279: Port frameworks away from QRegExp.

REPOSITORY
  R293 Baloo

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

To: ahmadsamir, #baloo, meven, bruns, astippich, mlaurent
Cc: kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, fbampaloukas, GB_2, 
domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams

D26210: [KCoreAddons] Port QRegExp to QRegularExpression

[Ahmad Samir]

ahmadsamir created this revision.
ahmadsamir added reviewers: Frameworks, mpyne, dfaure, apol.
Herald added a project: Frameworks.
ahmadsamir requested review of this revision.

REVISION SUMMARY
  Port QRegExp::exactMatch() by using QRegularExpression::anchoredPattern()
  to match the entire subject string.
  
  KStringHandler::tagUrls(), make the regex slightly easier to read and capture
  the whole matched url to use when calling QString::replace().
  
  Note that KStringHandler::perlSplit(QRegExp... will have to be kept and
  marked for deprecation, since it's called by some other apps in KDE.
  A new perlSplit() overload should be added, possibly, just one perlSplit()
  function that takes a QString, and a param indicating whether to treat it
  as plain text or a regular expression.

TEST PLAN
  make && ctest

REPOSITORY
  R244 KCoreAddons

BRANCH
  l-qregularexpression (branched from master)

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

AFFECTED FILES
  autotests/krandomtest.cpp
  src/lib/io/kfileutils.cpp
  src/lib/text/kmacroexpander_unix.cpp
  src/lib/text/kstringhandler.cpp
  src/lib/util/klistopenfilesjob_unix.cpp
  src/lib/util/kshell_win.cpp

To: ahmadsamir, #frameworks, mpyne, dfaure, apol
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26205: KWallet: Port QRegExp to QRegularExpression

[Ahmad Samir]

This revision was automatically updated to reflect the committed changes.
Closed by commit R311:bb4eed3bfda0: KWallet: Port QRegExp to QRegularExpression 
(authored by ahmadsamir).

REPOSITORY
  R311 KWallet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26205?vs=72132&id=72146

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

AFFECTED FILES
  src/api/KWallet/kwallet.cpp
  src/runtime/kwalletd/backend/kwalletbackend.cc
  src/runtime/kwalletd/kwalletd.cpp

To: ahmadsamir, #frameworks, aacid, apol
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26205: KWallet: Port QRegExp to QRegularExpression

[Aleix Pol Gonzalez]

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

REPOSITORY
  R311 KWallet

BRANCH
  l-qregularexpression (branched from master)

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

To: ahmadsamir, #frameworks, aacid, apol
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26198: Deprecate KCModuleContainer

[Aleix Pol Gonzalez]

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

REPOSITORY
  R295 KCMUtils

BRANCH
  container

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

To: nicolasfella, #frameworks, apol
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

File missing license header in KCoreAddons

[Ahmad Samir]

Hi there.

While working on KCoreAddons I found that 
"kcoreaddons/src/autotests/kstringhandlertest.cpp" doesn't have a 
license header at all; digging around in the old kdelibs git repo 
history, I found it was initially written by Waldo Bastian, commit 
6b1c11fbf708ca6717c1ce2907c9862e6be4ae05.



--
Ahmad Samir

D26197: Display fully qualified class/namespace name as page header

[Albert Astals Cid]

aacid added a comment.


  I've no idea about the code, but looks sensible to me, so take this as a +2 
if noone reviews in a while i guess :)

REPOSITORY
  R264 KApiDox

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

To: davidre
Cc: aacid, jucato, kde-frameworks-devel, kde-doc-english, LeGast00n, gennad, 
fbampaloukas, GB_2, michaelh, ngraham, bruns, skadinna

Re: D26126: Simplify param method: Return Early, Use a Map and Assert.

[Tomaz Canabrava]

David,

Thanks for the detailed explanation, I'm currently reworking most of the
patches here. About the optimizations - I know the linear search is a bad
optimization, but it adds legibility, that's what I tried to do.
I tried to emulate the python `if value in vector` that is really easy to
read, compared to the current code.



On Tue, Dec 24, 2019 at 9:40 AM David Faure 
wrote:

> dfaure requested changes to this revision.
> dfaure added inline comments. View Revision
> 
> *INLINE COMMENTS*
> View Inline 
> kconfig_compiler.cpp:1020
> } else if (type == QLatin1String("font")) {
> return QStringLiteral("const QFont &");
> } else if (type == QLatin1String("rect")) {
> QLatin1String("double")}
> ).contains(type)) {
> return type;
>
> This linear search doesn't look like an optimization to me. It would be
> better to incorporate this into the map, so that a single search is
> performed, rather than two.
>
> View Inline ervin wrote
> in kconfig_compiler.cpp:1024
>
> std::map looks like a bad idea here, either use QHash (preferred in
> massively based Qt code) or std::unordered_map.
>
> Also for both temporaries you pay the price of their allocation and
> deallocation at each call. Even a mutex used at each call would do better.
> ;-)
>
> I'm not 100% sure about std::map vs QHash given the number of elements,
> this would need benchmarking.
>
> But I agree 100% that compiler-generated thread safety around a static is
> NOTHING compared to amount of nodes allocated to fill in a map or hash.
>
> @tcanabrava  Please have a
> look at https://gist.github.com/jboner/2841832
> Locking an available mutex is 4 times faster than even fetching data from
> main memory (i.e. data which isn't yet in a CPU cache). This is many orders
> of magnitude faster than creating a filling a map or a hash full of
> QStrings. On top of that, compilers don't even generate a full-blown mutex
> for threadsafe statics, they generate a three-state atomic (a bit like
> Q_GLOBAL_STATIC does) (3 because not created, being created, already
> created).
>
> The most performant solution is to turn the input string into a QByteArray
> and then perform a binary search in a C array of const char* (no
> allocations, even the very first time).
> The most readable (but still good, unlike the current code in git)
> solution is a static map.
>
> *REPOSITORY*
> R237 KConfig
>
> *REVISION DETAIL*
> https://phabricator.kde.org/D26126
>
> *To: *tcanabrava, ervin, dfaure
> *Cc: *dfaure, ervin, GB_2, apol, kde-frameworks-devel, LeGast00n,
> michaelh, ngraham, bruns
>

D17816: Support for xattrs on kio copy/move

[Thomas Marshall]

tmarshall added inline comments.

INLINE COMMENTS

> tmarshall wrote in copyjob.cpp:1119
> Please help me understand for a moment. I'm not the original author of this 
> patch and am trying to bring it to completion. I just need to be caught up to 
> speed. In reading this piece of code over, I wasn't entirely sure of the 
> author's intent. It seems like he wants to create a new job which copies the 
> xattrs and then run it asynchronously. Why is the exec call bad? What does a 
> lambda do that the exec call does not?
> 
> In terms of determining when the job is actually required, one could test to 
> see if the file has xattrs or indeed if the system has xattrs support. We 
> could surround the invokation of the xattrs copy job with an `#ifdef 
> HAVE_SYS_XATTR_H`, for example.

And do we want this to be sync or async? That much isn't clear to me.

REPOSITORY
  R241 KIO

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

To: tmarshall, dfaure, chinmoyr, bruns, #frameworks, cochise
Cc: tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, funkybomber, abika, 
pino, davidedmundson, ngraham, atha.kane, spoorun, nicolasfella, 
kde-frameworks-devel, LeGast00n, GB_2, michaelh

D17816: Support for xattrs on kio copy/move

[Thomas Marshall]

tmarshall updated this revision to Diff 72144.
tmarshall added a comment.


  - Added helper functions to clean up tests

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17816?vs=72135&id=72144

BRANCH
  arcpatch-D17816

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

AFFECTED FILES
  autotests/jobtest.cpp
  autotests/jobtest.h
  src/core/CMakeLists.txt
  src/core/ConfigureChecks.cmake
  src/core/config-kiocore.h.cmake
  src/core/copyjob.cpp
  src/core/copyxattrjob.cpp
  src/core/copyxattrjob.h
  src/core/filecopyjob.cpp

To: tmarshall, dfaure, chinmoyr, bruns, #frameworks, cochise
Cc: tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, funkybomber, abika, 
pino, davidedmundson, ngraham, atha.kane, spoorun, nicolasfella, 
kde-frameworks-devel, LeGast00n, GB_2, michaelh

D17816: Support for xattrs on kio copy/move

[Thomas Marshall]

tmarshall marked 7 inline comments as done.

REPOSITORY
  R241 KIO

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

To: tmarshall, dfaure, chinmoyr, bruns, #frameworks, cochise
Cc: tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, funkybomber, abika, 
pino, davidedmundson, ngraham, atha.kane, spoorun, nicolasfella, 
kde-frameworks-devel, LeGast00n, GB_2, michaelh

D26195: Port QRegExp to QRegularExpression

[Ahmad Samir]

This revision was automatically updated to reflect the committed changes.
Closed by commit R236:84d17cbde725: Port QRegExp to QRegularExpression 
(authored by ahmadsamir).

REPOSITORY
  R236 KWidgetsAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26195?vs=72115&id=72143

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

AFFECTED FILES
  src/kcharselect.cpp
  src/kmimetypechooser.h
  src/ksqueezedtextlabel.cpp

To: ahmadsamir, #frameworks, apol, dfaure, vkrause
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25974: Fix Invalid-enum-value assignment

[Albert Astals Cid]

aacid added a comment.


  Thanks for taking care of it, i was busy yesterday with pre-xmas stuff

REPOSITORY
  R270 KCodecs

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

To: aacid, dfaure
Cc: dfaure, security-team, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D26208: [KEmailAddress] Remove redundant bool var

[Ahmad Samir]

ahmadsamir created this revision.
ahmadsamir added a reviewer: dfaure.
Herald added a project: Frameworks.
ahmadsamir requested review of this revision.

REVISION SUMMARY
  It looks like 'bool tooManyAtsFlag' was copied over from isValidAddress()
  (where it's actually used/useful) to isValidSimpleAddress() where it's
  redundant.

REPOSITORY
  R270 KCodecs

BRANCH
  l-ununsed-bool (branched from master)

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

AFFECTED FILES
  src/kemailaddress.cpp

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

D26123: Port QRegExp to QRegularExpression

[Ahmad Samir]

This revision was automatically updated to reflect the committed changes.
Closed by commit R270:7310a343212a: Port QRegExp to QRegularExpression 
(authored by ahmadsamir).

REPOSITORY
  R270 KCodecs

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26123?vs=72139&id=72141

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

AFFECTED FILES
  autotests/kemailaddresstest.cpp
  src/kemailaddress.cpp

To: ahmadsamir, #frameworks, dfaure, mlaurent, vkrause
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26123: Port QRegExp to QRegularExpression

[Ahmad Samir]

ahmadsamir updated this revision to Diff 72139.
ahmadsamir marked an inline comment as done.
ahmadsamir edited the summary of this revision.
ahmadsamir added a comment.


  Fix coding style

REPOSITORY
  R270 KCodecs

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26123?vs=72045&id=72139

BRANCH
  l-qregexp (branched from master)

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

AFFECTED FILES
  autotests/kemailaddresstest.cpp
  src/kemailaddress.cpp

To: ahmadsamir, #frameworks, dfaure, mlaurent, vkrause
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26177: Port QRegExp to QRegularExpression

[Ahmad Samir]

ahmadsamir updated this revision to Diff 72138.
ahmadsamir edited the summary of this revision.
ahmadsamir added a comment.


  Ver-blinking-batim

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26177?vs=72137&id=72138

BRANCH
  l-qregularexpression (branched from master)

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

AFFECTED FILES
  src/core/kauthorized.cpp
  src/kconfig_compiler/kconfig_compiler.cpp

To: ahmadsamir, #frameworks, dfaure, ervin, apol
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26177: Port QRegExp to QRegularExpression

[Ahmad Samir]

ahmadsamir added inline comments.

INLINE COMMENTS

> dfaure wrote in kconfig_compiler.cpp:646
> I wonder if qMax(path.lastIndexOf('/'), path.lastIndexOf('\\')) wouldn't be 
> simpler and faster :-)
> 
> But yes you're right, might as well just delete this function.
> I bet someone didn't realize, LONG ago, that even on Windows, Qt uses '/' in 
> almost all of the API.

Yep; (and who knows if Qt docs were as expansive/exhaustive as they're now :)).

And then fileNameOnly() became unneeded when QFileInfo was used in the code.

REPOSITORY
  R237 KConfig

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

To: ahmadsamir, #frameworks, dfaure, ervin, apol
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26177: Port QRegExp to QRegularExpression

[Ahmad Samir]

ahmadsamir updated this revision to Diff 72137.
ahmadsamir marked an inline comment as done.
ahmadsamir added a comment.


  Remove fileNameOnly() as per discussion in the review

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26177?vs=72049&id=72137

BRANCH
  l-qregularexpression (branched from master)

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

AFFECTED FILES
  src/core/kauthorized.cpp
  src/kconfig_compiler/kconfig_compiler.cpp

To: ahmadsamir, #frameworks, dfaure, ervin, apol
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26202: WIP: Refactor KConfigXT

[Tomaz Canabrava]

tcanabrava added a comment.


  In D26202#582541 , @ngraham wrote:
  
  > > This is really a wip, completely broken. It's just so people can point me 
to the right direction.
  >
  > Maybe could you clarify which direction you want to be pointed in? What's 
the goal here? What do you need help with?
  
  
  Fair Enough:
  
  The current KConfigXT compiler is in a sad state: 
  It's a massive file with loads of global variables that handle state, the 
generator is done within the main() function and it seems to have grown 
organically. There are no classes to separate logic / state / generation, what 
exists is code that generates code from a xml / ini pair, but it's hard to even 
discover what a bit of code is doing. The code istyle is C++ / Java from the 
nineties, which is not bad per see but it also uses quite a few things that are 
going to be deprecated in Qt 6 so I'm also taking the time make the code more 
streamlined with newer code style (no iterators, lambdas, auto usage, etc).
  
  The code that generates the files simplly pushes strings to a text stream, 
and it's hard to figure out when something starts or something ends: for 
instance, the code that generates the Constructor has more than sixty lines of 
code englobing some nested if - for - if - for constructs.
  
  This code tries to Separate the compiler code into many different files / 
classes to be more obvious what's happening, and each class also has many 
helper methods to minimize copypaste.
  
  - CodeGenerator: Has base code for the header and source files that can be 
shared
  - HeaderGenerator: Logic for generating the header file
  - SourceGenerator: Logic for generating the source file
  - KcfgParser: Logic for parsing the kcfg file and extracting the information
  - CommonStructs: a header that contains the structs that are currently used 
everywhere.
  - KConfigParameters: (was CfgConfig - ConfigConfig, wat) - Has information 
passed via the kcfgc file
  - kcfg_compiler - will be renamed to main - start the other classes and 
generates the files.
  
  This code here currently has the begining of this separation, with the 
CodeGenerator and the HeaderGenerator in a ~good~ state, but unfinished.

REPOSITORY
  R237 KConfig

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

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

D17816: Support for xattrs on kio copy/move

[Thomas Marshall]

tmarshall updated this revision to Diff 72135.
tmarshall added a comment.


  - Addressed comments, changed xattr system calls to use file descriptors 
where possible

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17816?vs=72131&id=72135

BRANCH
  arcpatch-D17816

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

AFFECTED FILES
  autotests/jobtest.cpp
  autotests/jobtest.h
  src/core/CMakeLists.txt
  src/core/ConfigureChecks.cmake
  src/core/config-kiocore.h.cmake
  src/core/copyjob.cpp
  src/core/copyxattrjob.cpp
  src/core/copyxattrjob.h
  src/core/filecopyjob.cpp

To: tmarshall, dfaure, chinmoyr, bruns, #frameworks, cochise
Cc: tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, funkybomber, abika, 
pino, davidedmundson, ngraham, atha.kane, spoorun, nicolasfella, 
kde-frameworks-devel, LeGast00n, GB_2, michaelh

D17816: Support for xattrs on kio copy/move

[Thomas Marshall]

tmarshall added inline comments.

INLINE COMMENTS

> dfaure wrote in copyjob.cpp:1119
> That sounds racy; the subjob might not be finished by the time the main job 
> is finished, if you just "start and forget".
> 
> I cannot accept this commit with an exec() in here. The easy and modern way 
> to make this async is just a connect and a lambda (which contains the rest of 
> the code here).
> 
> I have to wonder though: do we have a fast way to determine whether we 
> actually need the additional subjob? (to avoid making this slower on systems 
> -- or files -- without xattr)
> 
> Hmm, or a much bigger architectural question: why doesn't kio_file copy the 
> xattr as part of FileProtocol::copy(), as it already does with e.g. 
> permissions and ACLs, instead of doing this in a separate job?

Please help me understand for a moment. I'm not the original author of this 
patch and am trying to bring it to completion. I just need to be caught up to 
speed. In reading this piece of code over, I wasn't entirely sure of the 
author's intent. It seems like he wants to create a new job which copies the 
xattrs and then run it asynchronously. Why is the exec call bad? What does a 
lambda do that the exec call does not?

In terms of determining when the job is actually required, one could test to 
see if the file has xattrs or indeed if the system has xattrs support. We could 
surround the invokation of the xattrs copy job with an `#ifdef 
HAVE_SYS_XATTR_H`, for example.

REPOSITORY
  R241 KIO

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

To: tmarshall, dfaure, chinmoyr, bruns, #frameworks, cochise
Cc: tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, funkybomber, abika, 
pino, davidedmundson, ngraham, atha.kane, spoorun, nicolasfella, 
kde-frameworks-devel, LeGast00n, GB_2, michaelh

D26195: Port QRegExp to QRegularExpression

[David Faure]

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

REPOSITORY
  R236 KWidgetsAddons

BRANCH
  l-qregularexpression (branched from master)

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

To: ahmadsamir, #frameworks, apol, dfaure, vkrause
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26207: Port QRegExp to QRegularExpression

[Ahmad Samir]

ahmadsamir created this revision.
ahmadsamir added reviewers: Frameworks, dfaure, mlaurent, apol.
Herald added a project: Frameworks.
ahmadsamir requested review of this revision.

REVISION SUMMARY
  make && ctest

REPOSITORY
  R306 KParts

BRANCH
  l-qregularexpression (branched from master)

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

AFFECTED FILES
  src/browserextension.cpp

To: ahmadsamir, #frameworks, dfaure, mlaurent, apol
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26123: Port QRegExp to QRegularExpression

[David Faure]

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


  Thanks for the extensive research!

INLINE COMMENTS

> kemailaddress.cpp:608
>  
>  bool tooManyAtsFlag = false;
>  bool inQuotedString = false;

[pre-existing: this bool is never set to true]

> kemailaddress.cpp:643
> +const QRegularExpression rx(QRegularExpression::anchoredPattern(addrRx)
> +, 
> QRegularExpression::UseUnicodePropertiesOption);
> +return  rx.match(aStr).hasMatch() && !tooManyAtsFlag;

A bit weird that this comma isn't at the end of the previous line instead :)

REPOSITORY
  R270 KCodecs

BRANCH
  l-qregexp (branched from master)

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

To: ahmadsamir, #frameworks, dfaure, mlaurent, vkrause
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26177: Port QRegExp to QRegularExpression

[David Faure]

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

INLINE COMMENTS

> kconfig_compiler.cpp:646
>  {
> -int i = path.lastIndexOf(QRegExp(QStringLiteral("[/\\]")));
> +int i = path.lastIndexOf(QRegularExpression(QStringLiteral("[/]")));
>  if (i >= 0) {

I wonder if qMax(path.lastIndexOf('/'), path.lastIndexOf('\\')) wouldn't be 
simpler and faster :-)

But yes you're right, might as well just delete this function.
I bet someone didn't realize, LONG ago, that even on Windows, Qt uses '/' in 
almost all of the API.

REPOSITORY
  R237 KConfig

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

To: ahmadsamir, #frameworks, dfaure, ervin, apol
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26205: KWallet: Port QRegExp to QRegularExpression

[Ahmad Samir]

ahmadsamir created this revision.
ahmadsamir added reviewers: Frameworks, aacid.
Herald added a project: Frameworks.
ahmadsamir requested review of this revision.

REVISION SUMMARY
  Port QRegExp::exactMatch() with QRegularExpression::anchoredPattern()
  Port QRegExp::Wildcard with QRegularExpression::wildcardToRegularExpression()
  
  The code compiles (there is only one unrelated unit test, it passes).

REPOSITORY
  R311 KWallet

BRANCH
  l-qregularexpression (branched from master)

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

AFFECTED FILES
  src/api/KWallet/kwallet.cpp
  src/runtime/kwalletd/backend/kwalletbackend.cc
  src/runtime/kwalletd/kwalletd.cpp

To: ahmadsamir, #frameworks, aacid
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D17816: Support for xattrs on kio copy/move

[David Faure]

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

INLINE COMMENTS

> jobtest.cpp:535
> +xattrwriter.start(command, arguments);
> +xattrwriter.waitForFinished(-1);
> +

QVERIFY() around all calls to waitForFinished

(repeats)

> jobtest.cpp:537
> +
> +QHash attrs;
> +attrs["user.name with space"] = "value with spaces";

This entire QHash is duplicated as is between both methods, right?

Extract it into a function that returns the QHash (by value).

> jobtest.cpp:556
> +xattrwriter.waitForStarted();
> +QVERIFY(xattrwriter.state() == QProcess::Running);
> +xattrwriter.waitForFinished(-1);

Whenever you see QVERIFY(a==b) it should in fact be QCOMPARE(a, b) so that we 
can see the value on failure (this isn't gtest).

If needed, cast to int.

[repeats]

> jobtest.cpp:660
> +job->setUiDelegate(nullptr);
> +job->exec();
> +compareXattr(src, dest, command); // Our test

always use QVERIFY2(job->exec(), qPrintable(job->errorString()));

(repeats)

> jobtest.cpp:752-764
> +QString command = QStandardPaths::findExecutable("setfattr");
> +if (command.isEmpty()) {
> +command = QStandardPaths::findExecutable("setextattr");
> +if (command.isEmpty()) {
> +command = QStandardPaths::findExecutable("xattr");
> +}
> +}

Please extract a function from those 12 lines, which are duplicated.

> jobtest.cpp:766
> +
> +QUrl u = QUrl::fromLocalFile(src);
> +QString dest(_dest);

const

> jobtest.cpp:768
> +QString dest(_dest);
> +QUrl d = QUrl::fromLocalFile(dest);
> +

const

> cochise wrote in copyjob.cpp:1119
> I tried various ways to call this as a subjob, but all of them leads to 
> breakage of non xattr related tests. Maybe some major changes to the class 
> are needed.
> 
> But the call can be asynchronous with little effort. All tests still pass if 
> `start()` is called, instead of `exec()`.

That sounds racy; the subjob might not be finished by the time the main job is 
finished, if you just "start and forget".

I cannot accept this commit with an exec() in here. The easy and modern way to 
make this async is just a connect and a lambda (which contains the rest of the 
code here).

I have to wonder though: do we have a fast way to determine whether we actually 
need the additional subjob? (to avoid making this slower on systems -- or files 
-- without xattr)

Hmm, or a much bigger architectural question: why doesn't kio_file copy the 
xattr as part of FileProtocol::copy(), as it already does with e.g. permissions 
and ACLs, instead of doing this in a separate job?

REPOSITORY
  R241 KIO

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

To: tmarshall, dfaure, chinmoyr, bruns, #frameworks, cochise
Cc: tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, funkybomber, abika, 
pino, davidedmundson, ngraham, atha.kane, spoorun, nicolasfella, 
kde-frameworks-devel, LeGast00n, GB_2, michaelh

D26126: Simplify param method: Return Early, Use a Map and Assert.

[David Faure]

dfaure requested changes to this revision.
dfaure added inline comments.

INLINE COMMENTS

> kconfig_compiler.cpp:1020
> +QLatin1String("double")}
> +).contains(type)) {
> +return type;

This linear search doesn't look like an optimization to me. It would be better 
to incorporate this into the map, so that a single search is performed, rather 
than two.

> ervin wrote in kconfig_compiler.cpp:1024
> std::map looks like a bad idea here, either use QHash (preferred in massively 
> based Qt code) or std::unordered_map.
> 
> Also for both temporaries you pay the price of their allocation and 
> deallocation at each call. Even a mutex used at each call would do better. ;-)

I'm not 100% sure about std::map vs QHash given the number of elements, this 
would need benchmarking.

But I agree 100% that compiler-generated thread safety around a static is 
NOTHING compared to amount of nodes allocated to fill in a map or hash.

@tcanabrava Please have a look at https://gist.github.com/jboner/2841832
Locking an available mutex is 4 times faster than even fetching data from main 
memory (i.e. data which isn't yet in a CPU cache). This is many orders of 
magnitude faster than creating a filling a map or a hash full of QStrings. On 
top of that, compilers don't even generate a full-blown mutex for threadsafe 
statics, they generate a three-state atomic (a bit like Q_GLOBAL_STATIC does) 
(3 because not created, being created, already created).

The most performant solution is to turn the input string into a QByteArray and 
then perform a binary search in a C array of const char* (no allocations, even 
the very first time).
The most readable (but still good, unlike the current code in git) solution is 
a static map.

REPOSITORY
  R237 KConfig

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

To: tcanabrava, ervin, dfaure
Cc: dfaure, ervin, GB_2, apol, kde-frameworks-devel, LeGast00n, michaelh, 
ngraham, bruns

D17816: Support for xattrs on kio copy/move

[Thomas Marshall]

tmarshall marked 9 inline comments as done.

REPOSITORY
  R241 KIO

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

To: tmarshall, dfaure, chinmoyr, bruns, #frameworks, cochise
Cc: tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, funkybomber, abika, 
pino, davidedmundson, ngraham, atha.kane, spoorun, nicolasfella, 
kde-frameworks-devel, LeGast00n, GB_2, michaelh

KDE CI: Frameworks » kcodecs » kf5-qt5 SUSEQt5.12 - Build # 65 - Fixed!

[CI System]

BUILD SUCCESS
 Build URL
https://build.kde.org/job/Frameworks/job/kcodecs/job/kf5-qt5%20SUSEQt5.12/65/
 Project:
kf5-qt5 SUSEQt5.12
 Date of build:
Tue, 24 Dec 2019 08:03:20 +
 Build duration:
3 min 23 sec and counting
   BUILD ARTIFACTS
  abi-compatibility-results.yamlacc/KF5Codecs-5.66.0.xmlcompat_reports/KF5Codecs_compat_report.htmllogs/KF5Codecs/5.66.0/log.txt
   JUnit Tests
  Name: (root) Failed: 0 test(s), Passed: 1 test(s), Skipped: 0 test(s), Total: 1 test(s)Name: projectroot Failed: 0 test(s), Passed: 6 test(s), Skipped: 0 test(s), Total: 6 test(s)
   Cobertura Report
  
   Project Coverage Summary
  
   Name
  PackagesFilesClassesLinesConditionalsCobertura Coverage Report100%
(3/3)85%
(50/59)85%
(50/59)76%
(2926/3825)60%
(1312/2205)Coverage Breakdown by Package
Name
   FilesClassesLinesConditionalsautotests100%
(6/6)100%
(6/6)99%
(660/667)61%
(113/184)src81%
(13/16)81%
(13/16)74%
(1490/2008)62%
(839/1344)src.probers84%
(31/37)84%
(31/37)67%
(776/1150)53%
(360/677)

KDE CI: Frameworks » kcodecs » kf5-qt5 SUSEQt5.13 - Build # 37 - Fixed!

[CI System]

BUILD SUCCESS
 Build URL
https://build.kde.org/job/Frameworks/job/kcodecs/job/kf5-qt5%20SUSEQt5.13/37/
 Project:
kf5-qt5 SUSEQt5.13
 Date of build:
Tue, 24 Dec 2019 08:03:20 +
 Build duration:
2 min 40 sec and counting
   BUILD ARTIFACTS
  abi-compatibility-results.yamlacc/KF5Codecs-5.66.0.xmlcompat_reports/KF5Codecs_compat_report.htmllogs/KF5Codecs/5.66.0/log.txt
   JUnit Tests
  Name: (root) Failed: 0 test(s), Passed: 1 test(s), Skipped: 0 test(s), Total: 1 test(s)Name: projectroot Failed: 0 test(s), Passed: 6 test(s), Skipped: 0 test(s), Total: 6 test(s)
   Cobertura Report
  
   Project Coverage Summary
  
   Name
  PackagesFilesClassesLinesConditionalsCobertura Coverage Report100%
(3/3)85%
(50/59)85%
(50/59)76%
(2926/3825)60%
(1312/2205)Coverage Breakdown by Package
Name
   FilesClassesLinesConditionalsautotests100%
(6/6)100%
(6/6)99%
(660/667)61%
(113/184)src81%
(13/16)81%
(13/16)74%
(1490/2008)62%
(839/1344)src.probers84%
(31/37)84%
(31/37)67%
(776/1150)53%
(360/677)

KDE CI: Frameworks » kcodecs » kf5-qt5 FreeBSDQt5.13 - Build # 33 - Fixed!

[CI System]

BUILD SUCCESS
 Build URL
https://build.kde.org/job/Frameworks/job/kcodecs/job/kf5-qt5%20FreeBSDQt5.13/33/
 Project:
kf5-qt5 FreeBSDQt5.13
 Date of build:
Tue, 24 Dec 2019 08:03:20 +
 Build duration:
1 min 44 sec and counting
   JUnit Tests
  Name: projectroot Failed: 0 test(s), Passed: 6 test(s), Skipped: 0 test(s), Total: 6 test(s)

D25974: Fix Invalid-enum-value assignment

[David Faure]

dfaure added a comment.


  Fixed in 
https://commits.kde.org/kcodecs/6fcd8af5b4be2082c4af7d11c18d39151f1e25d0

REPOSITORY
  R270 KCodecs

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

To: aacid, dfaure
Cc: dfaure, security-team, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns