KDE CI: Frameworks » ktexteditor » kf5-qt5 FreeBSDQt5.13 - Build # 138 - Unstable!

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks/job/ktexteditor/job/kf5-qt5%20FreeBSDQt5.13/138/
 Project:
kf5-qt5 FreeBSDQt5.13
 Date of build:
Fri, 01 Nov 2019 04:25:32 +
 Build duration:
19 min and counting
   JUnit Tests
  Name: projectroot Failed: 1 test(s), Passed: 61 test(s), Skipped: 0 test(s), Total: 62 test(s)Failed: projectroot.autotests.messagetestName: projectroot.autotests.src Failed: 0 test(s), Passed: 5 test(s), Skipped: 0 test(s), Total: 5 test(s)

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

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks/job/kio/job/kf5-qt5%20FreeBSDQt5.13/164/
 Project:
kf5-qt5 FreeBSDQt5.13
 Date of build:
Fri, 01 Nov 2019 04:24:55 +
 Build duration:
11 min 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 » kdelibs4support » kf5-qt5 SUSEQt5.13 - Build # 41 - Fixed!

[CI System]

BUILD SUCCESS
 Build URL
https://build.kde.org/job/Frameworks/job/kdelibs4support/job/kf5-qt5%20SUSEQt5.13/41/
 Project:
kf5-qt5 SUSEQt5.13
 Date of build:
Fri, 01 Nov 2019 04:24:30 +
 Build duration:
7 min 6 sec and counting
   BUILD ARTIFACTS
  acc/KF5KDE4Support-5.64.0.xmlacc/KF5KDELibs4Support-5.64.0.xml
   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: 39 test(s), Skipped: 0 test(s), Total: 39 test(s)
   Cobertura Report
  
   Project Coverage Summary
  
   Name
  PackagesFilesClassesLinesConditionalsCobertura Coverage Report29%
(4/14)43%
(124/291)43%
(124/291)47%
(21579/45973)38%
(13019/34269)Coverage Breakdown by Package
Name
   FilesClassesLinesConditionalsdefault0%
(0/1)0%
(0/1)0%
(0/87)0%
(0/56)autotests98%
(44/45)98%
(44/45)97%
(11348/11739)50%
(7172/14369)src0%
(0/1)0%
(0/1)0%
(0/4)0%
(0/2)src.kdebugdialog0%
(0/7)0%
(0/7)0%
(0/256)0%
(0/73)src.kdecore76%
(65/86)76%
(65/86)53%
(9479/17867)48%
(5539/11439)src.kdeui19%
(13/67)19%
(13/67)8%
(742/9697)6%
(307/5396)src.kio7%
(2/27)7%
(2/27)0%
(10/2260)0%
(1/1242)src.kioslave.metainfo0%
(0/1)0%
(0/1)0%
(0/32)0%
(0/4)src.kparts0%
(0/1)0%
(0/1)0%
(0/24)0%
(0/12)src.kssl0%
(0/8)0%
(0/8)0%
(0/1807)0%
(0/855)src.kssl.kcm0%
(0/3)0%
(0/3)0%
(0/264)0%
(0/145)src.solid0%
(0/3)0%
(0/3)0%
(0/186)0%
(0/87)src.solid-networkstatus.kded0%
(0/7)0%
(0/7)0%
(0/220)0%
(0/102)tests0%
(0/34)0%
(0/34)0%
(0/1530)0%
(0/487)

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

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks/job/kdelibs4support/job/kf5-qt5%20FreeBSDQt5.13/38/
 Project:
kf5-qt5 FreeBSDQt5.13
 Date of build:
Fri, 01 Nov 2019 04:24:30 +
 Build duration:
3 min 50 sec and counting
   JUnit Tests
  Name: projectroot Failed: 2 test(s), Passed: 37 test(s), Skipped: 0 test(s), Total: 39 test(s)Failed: projectroot.autotests.kmimetypetestFailed: projectroot.autotests.kstandarddirstest

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

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks/job/kcoreaddons/job/kf5-qt5%20FreeBSDQt5.13/54/
 Project:
kf5-qt5 FreeBSDQt5.13
 Date of build:
Fri, 01 Nov 2019 01:46:55 +
 Build duration:
2 min 35 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

D22946: Include API to generically implement --replace arguments

[David Edmundson]

davidedmundson added a comment.


  I think I didn't explain myself properly. There's a way to do a version that 
has no race conditions.
  
  Order of events needs to be
  
  1. We try to register our service name with the queued flag
  2. We see if we succeeded to register it immediately
  3. If it fails, we send a non-blocking quit method to whoever currently owns 
that name
  4. and then wait for our application to be given the service name we've 
already requested
  
  all except 3 exist already.

INLINE COMMENTS

> kdbusservice.cpp:171
>  
> -if (options & KDBusService::Unique) {
> +if (options & KDBusService::Replace && 
> QDBusConnection::sessionBus().interface()->isServiceRegistered(d->serviceName))
>  {
> +auto message = QDBusMessage::createMethodCall(d->serviceName,

This is still racey

> kdbusservice.cpp:181
> +QEventLoop unregistrationLoop;
> +connect(bus, ::serviceUnregistered,
> +this, [, this](const QString 
> ) {

QDBusConnectionInterface::serviceUnregistered is very different to 
QDBusServiceWatcher::serviceUnregistered

QDBusConnectionInterface::serviceUnregistered will be called when our process 
loses the service name it had.

Our process hasn't even requested a service name yet, so this won't happen.

REPOSITORY
  R271 KDBusAddons

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

To: apol, #frameworks, davidedmundson
Cc: davidedmundson, broulik, kossebau, kde-frameworks-devel, LeGast00n, GB_2, 
michaelh, ngraham, bruns

T11950: Reduce the pain of working on monochrome Breeze icons

[Noah Davis]

ndavis added a comment.


  I don't trust optimizers unless it's `scour`. That's the only one I know of 
that doesn't seem to cause problems occasionally. Inkscape's Optimized SVG save 
option actually uses `scour` to do the optimization.
  
  I'm somewhat open to the idea of only requiring optimization at build time 
for the sake of making it easier for newbies to contribute, but I prefer 
reading optimized SVGs in patches because that makes it easier for me to spot 
mistakes. The massive quantity of noise that Inkscape puts into SVGs, even 
plain SVGs, is staggering. Maybe we could have something that cleans the SVGs 
when submitted for a patch? If it won't work with Phabricator, it might work 
with GitLab.
  
  In T11950#206330 , @ngraham wrote:
  
  > As far as I can tell, every single icon in the `icons-dark` folder is 
identical to the other ones with the exception of having the colors in the 
stylesheet changed. Auto-generating these in CMake seems quite feasible.
  
  
  There are a few that can't be auto-generated that way (gradients don't work 
with stylesheets), so we should have some logic to avoid overwriting files that 
already exist. Then we can eliminate all the icons that only need to have their 
stylesheet changed while keeping the option to do Breeze Dark specific icons.
  
  > Optimization creates real problem - sometimes icons after optimizations 
become hard to edit, so less people want to do modifications.
  
  There are 2 main issues that Optimizers introduce (in order of severity):
  
  - That gradients that don't get their properties from another gradient that 
they reference can't be swapped, unless you unset and reset the fill of the 
object you want to change the gradient for. This seems like a bug in Inkscape.
  - That some optimizers combine paths with similar properties by default. This 
isn't a big deal if you know to go to Path > Break Apart in Inkscape, but 
obviously not everyone is going to know Inkscape or SVGs very well. It's hard 
to say how much a new contributor should be expected to know about SVGs or 
Inkscape.
- Sometimes Break Apart causes objects to lose their properties and turn 
black. Usually not a big deal, it's very easy to fix, but not good for workflow.
- I suppose we could just make sure we don't use any settings in the 
optimizer for combining paths.

TASK DETAIL
  https://phabricator.kde.org/T11950

To: ngraham, ndavis
Cc: mglb, #frameworks, mart, trickyricky26, ndavis, #vdg, ngraham, LeGast00n, 
cblack, konkinartem, ian, jguidon, hannahk, Ghost6, jraleigh, MrPepe, 
fbampaloukas, squeakypancakes, alexde, IohannesPetros, GB_2, michaelh, crozbo, 
firef, bruns, skadinna, aaronhoneycutt, mbohlender

D22946: Include API to generically implement --replace arguments

[Aleix Pol Gonzalez]

apol marked an inline comment as done.

REPOSITORY
  R271 KDBusAddons

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

To: apol, #frameworks, davidedmundson
Cc: davidedmundson, broulik, kossebau, kde-frameworks-devel, LeGast00n, GB_2, 
michaelh, ngraham, bruns

D22946: Include API to generically implement --replace arguments

[Aleix Pol Gonzalez]

apol updated this revision to Diff 69147.
apol added a comment.


  Rebase and get inspiration from 5bf091ee07ac44ed1bf1e75a4d07847edb86c5d6 
 as 
suggested by David

REPOSITORY
  R271 KDBusAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22946?vs=63124=69147

BRANCH
  arcpatch-D22946

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

AFFECTED FILES
  src/kdbusservice.cpp
  src/kdbusservice.h

To: apol, #frameworks, davidedmundson
Cc: davidedmundson, broulik, kossebau, kde-frameworks-devel, LeGast00n, GB_2, 
michaelh, ngraham, bruns

D24621: [RFC] New Konsole and Yakuake icons

[Nathaniel Graham]

ngraham added a comment.


  In D24621#547787 , @ngraham wrote:
  
  > I really like the Yakuake icon that has a subtle "Y" in it. If everyone 
else does too, maybe that should be moved into a separate patch so we can get 
it in quickly and then concentrate on the Konsole icon.
  
  
  @mglb could we move forward with this so the Yakuake icon doesn't get stuck 
waiting on the results of the conversation about the Konsole icon?

REPOSITORY
  R266 Breeze Icons

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

To: mglb, #vdg, #konsole, #yakuake, trickyricky26
Cc: ngraham, trickyricky26, ndavis, cblack, kde-frameworks-devel, LeGast00n, 
GB_2, michaelh, bruns

D25084: Allow a Multiple instances to become Unique

[David Edmundson]

davidedmundson added a comment.


  I don't follow this patch.
  
  Say I'm starting a unique app (like org.kde.PlasmaShell)
  
  If another bus name already exists, which with the same prefix (such as 
"org.kde.PlasmaShellasdfasdf") then my new unique app will try to register as 
"org.kde.PlasmaShellasdfasdf" even though we know for sure that that name is 
already used by someone else?

REPOSITORY
  R271 KDBusAddons

BRANCH
  multiple_to_unique

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

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

T11950: Reduce the pain of working on monochrome Breeze icons

[Mariusz Glebocki]

mglb added a comment.


  Inserting and applying styles can be done with icon template file with 
predefined swatches. Swatches (which are internally one-stop gradients) would 
be converted into style later with a script. Not trivial (i.e. not one line 
sed) to write due to indirect gradient use in inkscape, but also not really 
hard. Xml parser is the way to go. For extra newbie-friendly solution, "save as 
monochrome breeze svg" extension could be made.
  From designer point of view use is really simple - in fill and stroke dock 
click "swatch" type and pick named color from a list.
  
  Why breeze icons are "optimized"? As someone pointed out in another task, 
they are cached as bitmaps, so slightly more complicated XML is not a problem. 
Optimization creates real problem - sometimes icons after optimizations become 
hard to edit, so less people want to do modifications.

TASK DETAIL
  https://phabricator.kde.org/T11950

To: ngraham, mglb
Cc: mglb, #frameworks, mart, trickyricky26, ndavis, #vdg, ngraham, LeGast00n, 
cblack, konkinartem, ian, jguidon, hannahk, Ghost6, jraleigh, MrPepe, 
fbampaloukas, squeakypancakes, alexde, IohannesPetros, GB_2, michaelh, crozbo, 
firef, bruns, skadinna, aaronhoneycutt, mbohlender

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

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks/job/kio/job/kf5-qt5%20FreeBSDQt5.13/163/
 Project:
kf5-qt5 FreeBSDQt5.13
 Date of build:
Thu, 31 Oct 2019 21:54:54 +
 Build duration:
12 min 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)

D24726: Fix null pointer reference when extraction fails

[Albert Astals Cid]

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

REPOSITORY
  R243 KArchive

BRANCH
  upstream_master

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

To: pvuorela, davidllewellynjones, dfaure, aacid
Cc: apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25049: Conditionally show navigation buttons in the header for multi-page KCMs

[Nathaniel Graham]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit R295:47f23c0f6ad1: Conditionally show navigation buttons in 
the header for multi-page KCMs (authored by ngraham).

REPOSITORY
  R295 KCMUtils

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25049?vs=68988=69138

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

AFFECTED FILES
  src/kcmoduleqml.cpp

To: ngraham, #vdg, #plasma, mart
Cc: GB_2, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D25084: Allow a Multiple instances to become Unique

[Adriaan de Groot]

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


  This has been applied to FreeBSD ports because it works. +1 (with the other 
konsole patch that **is** on invent).

REPOSITORY
  R271 KDBusAddons

BRANCH
  multiple_to_unique

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

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

D25049: Conditionally show navigation buttons in the header for multi-page KCMs

[Nathaniel Graham]

ngraham added a comment.


  No regressions found and seems low-risk. Landing now to get it into 5.64 
ahead of tagging on Nov 2nd.

REPOSITORY
  R295 KCMUtils

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

To: ngraham, #vdg, #plasma, mart
Cc: GB_2, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

KDE CI: Frameworks » breeze-icons » kf5-qt5 WindowsMSVCQt5.13 - Build # 57 - Fixed!

[CI System]

BUILD SUCCESS
 Build URL
https://build.kde.org/job/Frameworks/job/breeze-icons/job/kf5-qt5%20WindowsMSVCQt5.13/57/
 Project:
kf5-qt5 WindowsMSVCQt5.13
 Date of build:
Thu, 31 Oct 2019 19:09:23 +
 Build duration:
7 min 2 sec and counting
   JUnit Tests
  Name: projectroot Failed: 0 test(s), Passed: 4 test(s), Skipped: 0 test(s), Total: 4 test(s)

KDE CI: Frameworks » breeze-icons » kf5-qt5 SUSEQt5.12 - Build # 192 - Fixed!

[CI System]

BUILD SUCCESS
 Build URL
https://build.kde.org/job/Frameworks/job/breeze-icons/job/kf5-qt5%20SUSEQt5.12/192/
 Project:
kf5-qt5 SUSEQt5.12
 Date of build:
Thu, 31 Oct 2019 19:01:57 +
 Build duration:
2 min 45 sec and counting
   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: 4 test(s), Skipped: 0 test(s), Total: 4 test(s)
   Cobertura Report
  
   Project Coverage Summary
  
   Name
  PackagesFilesClassesLinesConditionalsCobertura Coverage Report100%
(2/2)100%
(6/6)100%
(6/6)77%
(231/299)58%
(105/182)Coverage Breakdown by Package
Name
   FilesClassesLinesConditionalsdefault100%
(1/1)100%
(1/1)73%
(40/55)56%
(10/18)autotests100%
(5/5)100%
(5/5)78%
(191/244)58%
(95/164)

KDE CI: Frameworks » breeze-icons » kf5-qt5 SUSEQt5.12 - Build # 191 - Still Unstable!

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks/job/breeze-icons/job/kf5-qt5%20SUSEQt5.12/191/
 Project:
kf5-qt5 SUSEQt5.12
 Date of build:
Thu, 31 Oct 2019 18:55:56 +
 Build duration:
6 min 0 sec and counting
   JUnit Tests
  Name: (root) Failed: 0 test(s), Passed: 1 test(s), Skipped: 0 test(s), Total: 1 test(s)Name: projectroot Failed: 1 test(s), Passed: 3 test(s), Skipped: 0 test(s), Total: 4 test(s)Failed: projectroot.autotests.scalable
   Cobertura Report
  
   Project Coverage Summary
  
   Name
  PackagesFilesClassesLinesConditionalsCobertura Coverage Report100%
(2/2)100%
(6/6)100%
(6/6)78%
(233/299)58%
(106/182)Coverage Breakdown by Package
Name
   FilesClassesLinesConditionalsdefault100%
(1/1)100%
(1/1)73%
(40/55)56%
(10/18)autotests100%
(5/5)100%
(5/5)79%
(193/244)59%
(96/164)

KDE CI: Frameworks » breeze-icons » kf5-qt5 SUSEQt5.13 - Build # 83 - Fixed!

[CI System]

BUILD SUCCESS
 Build URL
https://build.kde.org/job/Frameworks/job/breeze-icons/job/kf5-qt5%20SUSEQt5.13/83/
 Project:
kf5-qt5 SUSEQt5.13
 Date of build:
Thu, 31 Oct 2019 18:58:46 +
 Build duration:
1 min 40 sec and counting
   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: 4 test(s), Skipped: 0 test(s), Total: 4 test(s)
   Cobertura Report
  
   Project Coverage Summary
  
   Name
  PackagesFilesClassesLinesConditionalsCobertura Coverage Report100%
(2/2)100%
(6/6)100%
(6/6)77%
(231/299)58%
(105/182)Coverage Breakdown by Package
Name
   FilesClassesLinesConditionalsdefault100%
(1/1)100%
(1/1)73%
(40/55)56%
(10/18)autotests100%
(5/5)100%
(5/5)78%
(191/244)58%
(95/164)

KDE CI: Frameworks » breeze-icons » kf5-qt5 FreeBSDQt5.13 - Build # 78 - Fixed!

[CI System]

BUILD SUCCESS
 Build URL
https://build.kde.org/job/Frameworks/job/breeze-icons/job/kf5-qt5%20FreeBSDQt5.13/78/
 Project:
kf5-qt5 FreeBSDQt5.13
 Date of build:
Thu, 31 Oct 2019 18:58:46 +
 Build duration:
1 min 26 sec and counting
   JUnit Tests
  Name: projectroot Failed: 0 test(s), Passed: 4 test(s), Skipped: 0 test(s), Total: 4 test(s)

KDE CI: Frameworks » breeze-icons » kf5-qt5 FreeBSDQt5.13 - Build # 77 - Still Unstable!

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks/job/breeze-icons/job/kf5-qt5%20FreeBSDQt5.13/77/
 Project:
kf5-qt5 FreeBSDQt5.13
 Date of build:
Thu, 31 Oct 2019 18:55:56 +
 Build duration:
1 min 29 sec and counting
   JUnit Tests
  Name: projectroot Failed: 1 test(s), Passed: 3 test(s), Skipped: 0 test(s), Total: 4 test(s)Failed: projectroot.autotests.scalable

KDE CI: Frameworks » breeze-icons » kf5-qt5 SUSEQt5.13 - Build # 82 - Still Unstable!

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks/job/breeze-icons/job/kf5-qt5%20SUSEQt5.13/82/
 Project:
kf5-qt5 SUSEQt5.13
 Date of build:
Thu, 31 Oct 2019 18:55:56 +
 Build duration:
1 min 34 sec and counting
   JUnit Tests
  Name: (root) Failed: 0 test(s), Passed: 1 test(s), Skipped: 0 test(s), Total: 1 test(s)Name: projectroot Failed: 1 test(s), Passed: 3 test(s), Skipped: 0 test(s), Total: 4 test(s)Failed: projectroot.autotests.scalable
   Cobertura Report
  
   Project Coverage Summary
  
   Name
  PackagesFilesClassesLinesConditionalsCobertura Coverage Report100%
(2/2)100%
(6/6)100%
(6/6)78%
(233/299)58%
(106/182)Coverage Breakdown by Package
Name
   FilesClassesLinesConditionalsdefault100%
(1/1)100%
(1/1)73%
(40/55)56%
(10/18)autotests100%
(5/5)100%
(5/5)79%
(193/244)59%
(96/164)

T11950: Reduce the pain of working on monochrome Breeze icons

[Nathaniel Graham]

ngraham added a project: Frameworks.
ngraham added a subscriber: Frameworks.

TASK DETAIL
  https://phabricator.kde.org/T11950

To: ngraham
Cc: #frameworks, mart, trickyricky26, ndavis, #vdg, ngraham, LeGast00n, cblack, 
konkinartem, ian, jguidon, hannahk, Ghost6, jraleigh, MrPepe, fbampaloukas, 
squeakypancakes, alexde, IohannesPetros, GB_2, mglb, michaelh, crozbo, firef, 
bruns, skadinna, aaronhoneycutt, mbohlender

KDE CI: Frameworks » breeze-icons » kf5-qt5 SUSEQt5.13 - Build # 81 - Still Unstable!

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks/job/breeze-icons/job/kf5-qt5%20SUSEQt5.13/81/
 Project:
kf5-qt5 SUSEQt5.13
 Date of build:
Thu, 31 Oct 2019 18:10:17 +
 Build duration:
4 min 59 sec and counting
   JUnit Tests
  Name: (root) Failed: 0 test(s), Passed: 1 test(s), Skipped: 0 test(s), Total: 1 test(s)Name: projectroot Failed: 1 test(s), Passed: 3 test(s), Skipped: 0 test(s), Total: 4 test(s)Failed: projectroot.autotests.scalable
   Cobertura Report
  
   Project Coverage Summary
  
   Name
  PackagesFilesClassesLinesConditionalsCobertura Coverage Report100%
(2/2)100%
(6/6)100%
(6/6)78%
(233/299)58%
(106/182)Coverage Breakdown by Package
Name
   FilesClassesLinesConditionalsdefault100%
(1/1)100%
(1/1)73%
(40/55)56%
(10/18)autotests100%
(5/5)100%
(5/5)79%
(193/244)59%
(96/164)

KDE CI: Frameworks » breeze-icons » kf5-qt5 SUSEQt5.12 - Build # 190 - Still Unstable!

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks/job/breeze-icons/job/kf5-qt5%20SUSEQt5.12/190/
 Project:
kf5-qt5 SUSEQt5.12
 Date of build:
Thu, 31 Oct 2019 18:10:17 +
 Build duration:
2 min 21 sec and counting
   JUnit Tests
  Name: (root) Failed: 0 test(s), Passed: 1 test(s), Skipped: 0 test(s), Total: 1 test(s)Name: projectroot Failed: 1 test(s), Passed: 3 test(s), Skipped: 0 test(s), Total: 4 test(s)Failed: projectroot.autotests.scalable
   Cobertura Report
  
   Project Coverage Summary
  
   Name
  PackagesFilesClassesLinesConditionalsCobertura Coverage Report100%
(2/2)100%
(6/6)100%
(6/6)78%
(233/299)58%
(106/182)Coverage Breakdown by Package
Name
   FilesClassesLinesConditionalsdefault100%
(1/1)100%
(1/1)73%
(40/55)56%
(10/18)autotests100%
(5/5)100%
(5/5)79%
(193/244)59%
(96/164)

KDE CI: Frameworks » breeze-icons » kf5-qt5 FreeBSDQt5.13 - Build # 76 - Still Unstable!

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks/job/breeze-icons/job/kf5-qt5%20FreeBSDQt5.13/76/
 Project:
kf5-qt5 FreeBSDQt5.13
 Date of build:
Thu, 31 Oct 2019 18:10:17 +
 Build duration:
2 min 16 sec and counting
   JUnit Tests
  Name: projectroot Failed: 1 test(s), Passed: 3 test(s), Skipped: 0 test(s), Total: 4 test(s)Failed: projectroot.autotests.scalable

D23801: Port kpac from QtScript

[Allan Sandfeld Jensen]

carewolf added inline comments.

INLINE COMMENTS

> kossebau wrote in script.cpp:316
> With queued signals, any const-reference arguments are passed via an internal 
> value-copy IIRC, so references are not out-dated.
> Can the same technique not be expected with any usages of invocables, like 
> from scripting engines?

Sure, as I said, I only write it this way because a copy should be taken, and 
while Qt can work around declaring an async argument as a reference, I still 
consider it bad style to make that mistake.

In any case the difference is basically academic when it comes reference 
counted Qt containers. You can save some nanoseconds on doing a reference pass 
when it isn't async, but I have wasted more time hunting down obscure bugs 
caused by using references in cross-thread methods in other frameworks, so I 
prefer this. Feel free to change it if you like though. As I said it is just a 
best practice/coding style for me, and not necessary for Qt invokables.

REPOSITORY
  R241 KIO

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

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

D25039: Fix Clazy performance issues, const

[Méven Car]

meven updated this revision to Diff 69121.
meven marked 7 inline comments as done.
meven added a comment.


  Review feedback

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25039?vs=69083=69121

BRANCH
  arcpatch-D25039

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

AFFECTED FILES
  src/core/copyjob.cpp
  src/core/deletejob.cpp
  src/core/kcoredirlister.cpp
  src/core/slavebase.cpp
  src/core/slavebase.h
  src/core/slaveinterface.cpp
  src/filewidgets/kdiroperator.cpp
  src/filewidgets/knewfilemenu.cpp
  src/gui/faviconrequestjob.cpp
  src/ioslaves/ftp/ftp.cpp
  src/ioslaves/help/kio_help.cpp
  src/ioslaves/http/http.cpp
  src/ioslaves/http/kcookiejar/kcookiejar.cpp
  src/ioslaves/http/kcookiejar/kcookiejar.h
  src/ioslaves/http/kcookiejar/kcookieserver.h
  src/ioslaves/trash/tests/testtrash.cpp
  src/ioslaves/trash/trashimpl.cpp
  src/widgets/kdirmodel.cpp
  src/widgets/kfileitemactions.cpp
  src/widgets/kpropertiesdialog.cpp
  src/widgets/krun.cpp

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

D25039: Fix Clazy performance issues, const

[Friedrich W. H. Kossebau]

kossebau added inline comments.

INLINE COMMENTS

> ahmadsamir wrote in ftp.cpp:1376
> IIUC, the compiler will use a temporary object to hold the return of 
> tempurl.fileName().
> 
> The temporary is used to initialize filename, and then it's gone:
> const QString filename = tempurl.filename();
> 
> filename here is a reference-to-const to the temporary object and the 
> temporary will have the same lifetime as the reference (for objects on the 
> stack), so until the end of the scope:
> const QString  = tempurl.fileName();
> c.f. 
> https://herbsutter.com/2008/01/01/gotw-88-a-candidate-for-the-most-important-const/
> 
> In my mind it kind of makes more sense to use a reference-to-const when the 
> rvalue (tempurl.fileName()) is not a temporary object, because I am saving 
> nothing by using a reference here since the compiler will create the 
> temporary object and hold it until the end of the scope anyway.
> 
> In this particular case, it's probably exactly the same whether the temporary 
> is used to initialize a const non-reference object and then (the temporary) 
> is dropped/gone or the object being initialized is a reference to const to 
> the temporary until the end of the scope...
> 
> job->statResult() and job->url() are different because both of them return a 
> reference to const.

Thanks for the reference :) to some docs about it. So my memory about this 
being a chance to limit scope of lifetime of that temporary object was wrong 
then, so it's simply the same scope as the reference variable, not limited to 
the last line where the variable is actually used, did I get it right from 
reading that? (perhaps was some limit in some compiler, it's a decade ago that 
I was hinted to that, by accident remember the occasion, but less the content 
:) ).
If so, for the rest, seems we align in not using a const ref for the variable 
type, instead just making it the official variable holding the object/value, 
not just a reference. No gain with a const reference, and just confusing by 
being non-standard code.

REPOSITORY
  R241 KIO

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

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

D25039: Fix Clazy performance issues, const

[Ahmad Samir]

ahmadsamir added inline comments.

INLINE COMMENTS

> kossebau wrote in ftp.cpp:1376
> `QUrl::fileName()` returns a value QString, so just
> 
>   const QString filename = empurl.fileName();
> 
> While
> 
>   const QString  = empurl.fileName();
> 
> also is fine code IIRC, as I once learned to my surprise,  as the `const 
> QString &` here means to tell the compiler the actual value instance should 
> be hold only until the last use of the variable, not the end of the scope in 
> which the variable exists, this seems also not well known by others, so might 
> only make them confuse.
> Given this is an optimization not needed here, IMHO no need to use this 
> technique here.

IIUC, the compiler will use a temporary object to hold the return of 
tempurl.fileName().

The temporary is used to initialize filename, and then it's gone:
const QString filename = tempurl.filename();

filename here is a reference-to-const to the temporary object and the temporary 
will have the same lifetime as the reference (for objects on the stack), so 
until the end of the scope:
const QString  = tempurl.fileName();
c.f. 
https://herbsutter.com/2008/01/01/gotw-88-a-candidate-for-the-most-important-const/

In my mind it kind of makes more sense to use a reference-to-const when the 
rvalue (tempurl.fileName()) is not a temporary object, because I am saving 
nothing by using a reference here since the compiler will create the temporary 
object and hold it until the end of the scope anyway.

In this particular case, it's probably exactly the same whether the temporary 
is used to initialize a const non-reference object and then (the temporary) is 
dropped/gone or the object being initialized is a reference to const to the 
temporary until the end of the scope...

job->statResult() and job->url() are different because both of them return a 
reference to const.

REPOSITORY
  R241 KIO

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

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

D25039: Fix Clazy performance issues, const

[Friedrich W. H. Kossebau]

kossebau added inline comments.

INLINE COMMENTS

> ftp.cpp:1376
>  QString parentDir;
> -QString filename = tempurl.fileName();
> +const QString  = tempurl.fileName();
>  Q_ASSERT(!filename.isEmpty());

`QUrl::fileName()` returns a value QString, so just

  const QString filename = empurl.fileName();

While

  const QString  = empurl.fileName();

also is fine code IIRC, as I once learned to my surprise,  as the `const 
QString &` here means to tell the compiler the actual value instance should be 
hold only until the last use of the variable, not the end of the scope in which 
the variable exists, this seems also not well known by others, so might only 
make them confuse.
Given this is an optimization not needed here, IMHO no need to use this 
technique here.

> kossebau wrote in kcookiejar.cpp:1103
> This here seems some overleft from when the method actually needed a 
> modfyable copy of _domain. Seems this in no longer the case.
> So we can just rename `_domain` to `domain` in the arg list instead, and be 
> done.

Hm, why the change of all _domain to domain, not the other way around?
The _xyz naming of argument variables has been seen usually when it was needed 
to create a copy of the argument in the implementation, because e.g. the 
const-refness was in the way. As the normal naming of arguments and variables 
is without the _ prefix,. The prefix is then used with the argument name to 
denote this is the input to the actual variable then used in the implementation.

So:

  void foo(const Type arg)
   {
  // need to do non-const things with arg value, meh
  }

->

  void foo(const Type _arg)
   {
  Type arg(_arg);
  // do non-const things with arg value
  }

> kdirmodel.cpp:495
>  if (!urlsBeingFetched.isEmpty()) {
> -const QUrl dirUrl(url);
> +const QUrl (url);
>  for (const QUrl  : qAsConst(urlsBeingFetched)) {

`const QUrl  = url;` for consistency, please.

REPOSITORY
  R241 KIO

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

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

D25088: Use X-KDE-NoGlobalShortcut instead of NoDisplay to store that a component is disabled

[David Redondo]

davidre added inline comments.

INLINE COMMENTS

> kserviceactioncomponent.cpp:36
>  m_serviceStorageId(serviceStorageId),
>  
> m_desktopFile(QStandardPaths::locate(QStandardPaths::GenericDataLocation, 
> QStringLiteral("kglobalaccel/") + serviceStorageId))
>  {

Related to this discussion the KCM copies the desktop file to 
`.local/share/kglobalaccel` if you add one with the plus button. (As does 
D25086 )

REPOSITORY
  R268 KGlobalAccel

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

To: meven, #plasma, broulik, apol
Cc: davidre, davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D25067: Fix the header layouts for EntryDetails and Page components

[Nathaniel Graham]

ngraham added a comment.


  Now that's fixed on the main page but it's missing margins on the right side: 
F7702718: Screenshot_20191031_081429.png 
  
  And it's still broken on details pages: F7702721: 
Screenshot_20191031_081517.png 

REPOSITORY
  R304 KNewStuff

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

To: leinir, ngraham, #knewstuff, #frameworks, #plasma
Cc: ahiemstra, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25084: Allow a Multiple instances to become Unique

[Nathaniel Graham]

ngraham added a comment.


  Put this on invent maybe?

REPOSITORY
  R271 KDBusAddons

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

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

D25088: Use X-KDE-NoGlobalShortcut instead of NoDisplay to store that a component is disabled

[Méven Car]

meven added inline comments.

INLINE COMMENTS

> davidedmundson wrote in kserviceactioncomponent.cpp:36
> I don't think I follow.
> 
> We're editing the .desktop file inside 
> {XDG_DATA_DIRS}/kglobalaccel/someName.desktop
> The menu only reads from XDG_DATA_DIRS/applications/someName.desktop
> 
> where's the conflict?

Because usually the desktop files are copies of the same as applications :
install( DIRECTORY DESTINATION "${KDE_INSTALL_FULL_DATAROOTDIR}/kglobalaccel" )

So if you need to "NoDisplay" an app for krunner for instance, you also disable 
its shortcuts because the desktop files are copies of each other at 
installation.

FYI XDG_DATA_DIRS is deprecated

REPOSITORY
  R268 KGlobalAccel

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

To: meven, #plasma, broulik, apol
Cc: davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D24726: Fix null pointer reference when extraction fails

[Pekka Vuorela]

pvuorela added a comment.


  Haven't looked and to be honest a little skeptical if anyone removing a null 
check is likely enough to protect against.

REPOSITORY
  R243 KArchive

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

To: pvuorela, davidllewellynjones, dfaure, aacid
Cc: apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25079: [CopyJob] Increase the amount of data sendfile can copy at once

[Nathaniel Graham]

ngraham edited the summary of this revision.

REPOSITORY
  R241 KIO

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

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

D24726: Fix null pointer reference when extraction fails

[Aleix Pol Gonzalez]

apol added a comment.


  Have you looked into creating a unit test for this?

REPOSITORY
  R243 KArchive

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

To: pvuorela, davidllewellynjones, dfaure, aacid
Cc: apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23801: Port kpac from QtScript

[Friedrich W. H. Kossebau]

kossebau added inline comments.

INLINE COMMENTS

> carewolf wrote in script.cpp:316
> Invokables can be called asynchronously (queued), if you take a reference and 
> the isn't evaluated immediatly the reference might be invalid by the time the 
> call is evaluated.

With queued signals, any const-reference arguments are passed via an internal 
value-copy IIRC, so references are not out-dated.
Can the same technique not be expected with any usages of invocables, like from 
scripting engines?

REPOSITORY
  R241 KIO

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

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

D25088: Use X-KDE-NoGlobalShortcut instead of NoDisplay to store that a component is disabled

[David Edmundson]

davidedmundson added inline comments.

INLINE COMMENTS

> kserviceactioncomponent.cpp:36
>  m_serviceStorageId(serviceStorageId),
>  
> m_desktopFile(QStandardPaths::locate(QStandardPaths::GenericDataLocation, 
> QStringLiteral("kglobalaccel/") + serviceStorageId))
>  {

I don't think I follow.

We're editing the .desktop file inside 
{XDG_DATA_DIRS}/kglobalaccel/someName.desktop
The menu only reads from XDG_DATA_DIRS/applications/someName.desktop

where's the conflict?

REPOSITORY
  R268 KGlobalAccel

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

To: meven, #plasma, broulik, apol
Cc: davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D25088: Use X-KDE-NoGlobalShortcut instead of NoDisplay to store that a component is disabled

[Méven Car]

meven created this revision.
meven added reviewers: Plasma, broulik, apol.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
meven requested review of this revision.

REVISION SUMMARY
  Needed to allow apps to not be visible in menus but have shortcuts (i.e 
Krunner)
  Using NoDisplay to store that a component is disabled conflics with the 
meaning of NoDisplay.
  This is problem in Wayland as this platforms induces more desktop files to 
allow usage of KDE Wayland protocols and settings app icons for instance.
  
  Relates to D24857 

REPOSITORY
  R268 KGlobalAccel

BRANCH
  master

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

AFFECTED FILES
  src/runtime/globalshortcutsregistry.cpp
  src/runtime/kserviceactioncomponent.cpp

To: meven, #plasma, broulik, apol
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25003: [KDesktopFile] Add a property and propertyKde functions to access custom properties

[Méven Car]

meven abandoned this revision.
meven added a comment.


  In D25003#557100 , @broulik wrote:
  
  > `KDesktopFile::desktopGroup()` just gives you a `KConfigGroup` of the 
´[Desktop Entry]` section at which point you can read whatever you want
  
  
  Missed this, covers my need.

REPOSITORY
  R237 KConfig

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

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

D25003: [KDesktopFile] Add a property and propertyKde functions to access custom properties

[Kai Uwe Broulik]

broulik added a comment.


  `KDesktopFile::desktopGroup()` just gives you a `KConfigGroup` of the 
´[Desktop Entry]` section at which point you can read whatever you want

REPOSITORY
  R237 KConfig

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

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

D25003: [KDesktopFile] Add a property and propertyKde functions to access custom properties

[Méven Car]

meven added a comment.


  In D25003#555366 , @apol wrote:
  
  > Hi Meven, I guess I didn't make myself clear, sorry about that. This is not 
what I meant.
  >
  > What I meant was here D24956  you can 
query the desktop entry directly, we don't need to expose access to it.
  
  
  Maybe I am missing something again.
  But my reasoning was that `KDesktopFile` didn't allow to access arbitrary 
properties like `KService` does.
  And the freedesktop specification as defined such defined custom properties 
https://standards.freedesktop.org/desktop-entry-spec/desktop-entry-spec-latest.html
 "Extending the format".
  So I meant to fill this gap as it would be of use in D24956 
 since  `KDesktopFile` is `This class 
implements KDE's support for the freedesktop.org Desktop Entry Spec.
  `
  Unless I would use `KService` in D24956  
but semantically `KDesktopFile` is what I need, on mimetype business here and 
using `KService` would add a dependency to KGlobalAcccel.

REPOSITORY
  R237 KConfig

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

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

D25079: [CopyJob] Increase the amount of data sendfile can copy at once

[Méven Car]

meven edited the summary of this revision.

REPOSITORY
  R241 KIO

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

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

D24726: Fix null pointer reference when extraction fails

[Pekka Vuorela]

pvuorela added reviewers: davidllewellynjones, dfaure, aacid.

REPOSITORY
  R243 KArchive

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

To: pvuorela, davidllewellynjones, dfaure, aacid
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24726: Fix null pointer reference when extraction fails

[Pekka Vuorela]

pvuorela updated this revision to Diff 69086.
pvuorela added a comment.


  Also remove the empty target file on error

REPOSITORY
  R243 KArchive

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24726?vs=68119=69086

BRANCH
  upstream_master

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

AFFECTED FILES
  src/karchive.cpp

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

D23801: Port kpac from QtScript

[Allan Sandfeld Jensen]

carewolf added inline comments.

INLINE COMMENTS

> kossebau wrote in script.cpp:316
> So why would it be preferable to have the value be copied there? In general, 
> one prefers to avoid copies, so what is the different motvation here?

Invokables can be called asynchronously (queued), if you take a reference and 
the isn't evaluated immediatly the reference might be invalid by the time the 
call is evaluated.

REPOSITORY
  R241 KIO

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

To: carewolf, dfaure
Cc: kossebau, 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

D25039: Fix Clazy performance issues, const

[Méven Car]

meven added a comment.


  In D25039#557004 , @anthonyfieroni 
wrote:
  
  > Not using references is not a big problem with QString nor QUrl since they 
are reference counting objects, say if you don't change their content they are 
immutable, so
  >
  >   const QString s = other; // it's fine
  >   void func(QString s)
  >   {
  >const QString o = s; // use o instead of s is also fine, using plain 
s is fine too, if you don't touch mutability 
  >...
  >   }
  >
  
  
  Using cont &, you still save some reference counting overhead and allow the 
compiler to make potential better optimization.
  In your example you'd also save the boilerplate needed just to have a 
variable const.
  This also makes API clearer about what to expect.

INLINE COMMENTS

> kossebau wrote in ftp.cpp:1378
> This here makes the code fragile and more confusing. What is the difference 
> between `filename` & `search`? Why is `filename` not const? Can it ever 
> change? Would that be fine for `search` to change as well?
> So while seemingly a correct optimization, as `filename` seems not passed to 
> any method modifying it, the resulting code is very strange to a human 
> reader, the intent behind to have `search` being a const reference to 
> `filename` seems mysterious.
> 
> Without having understood the code, I would simply also make `filename` a 
> const variable, and add a hint why search is a const reference only (hinting 
> this is for optimization).
> Though actually `search` could be possibly be removed and checked what the 
> purpose of the asserts have been and merge this with the code upfront. But 
> outside of scope here, so leaving just a TODO for the next person should be 
> fine. One can still be the next person oneself :)

I believe search can be removed in fact, fileName can be used interchangeably.
That's the const beauty it makes this kind of issue visisble.
But this patch is about fixing atomic warnings, not fixing code around those 
warnings.
This can be fixed in a subsequent diff, thanks for pointing it out.

REPOSITORY
  R241 KIO

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

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

D25039: Fix Clazy performance issues, const

[Anthony Fieroni]

anthonyfieroni added a comment.


  Not using references is not a big problem with QString nor QUrl since they 
are reference counting objects, say if you don't change their content they are 
immutable, so
  
const QString s = other; // it's fine
void func(QString s)
{
 const QString o = s; // use o instead of s is also fine, using plain s 
is fine too, if you don't touch mutability 
 ...
}

REPOSITORY
  R241 KIO

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

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

D25039: Fix Clazy performance issues, const

[Méven Car]

meven updated this revision to Diff 69083.
meven edited the summary of this revision.
meven added a comment.


  amend commit message

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25039?vs=69082=69083

BRANCH
  arcpatch-D25039

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

AFFECTED FILES
  src/core/copyjob.cpp
  src/core/deletejob.cpp
  src/core/kcoredirlister.cpp
  src/core/slavebase.cpp
  src/core/slavebase.h
  src/core/slaveinterface.cpp
  src/filewidgets/kdiroperator.cpp
  src/filewidgets/knewfilemenu.cpp
  src/gui/faviconrequestjob.cpp
  src/ioslaves/ftp/ftp.cpp
  src/ioslaves/help/kio_help.cpp
  src/ioslaves/http/http.cpp
  src/ioslaves/http/kcookiejar/kcookiejar.cpp
  src/ioslaves/http/kcookiejar/kcookieserver.h
  src/ioslaves/trash/tests/testtrash.cpp
  src/ioslaves/trash/trashimpl.cpp
  src/widgets/kdirmodel.cpp
  src/widgets/kfileitemactions.cpp
  src/widgets/kpropertiesdialog.cpp
  src/widgets/krun.cpp

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

D25039: Fix Clazy performance issues, const

[Méven Car]

meven edited the summary of this revision.

REPOSITORY
  R241 KIO

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

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

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

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks/job/kcoreaddons/job/kf5-qt5%20FreeBSDQt5.13/53/
 Project:
kf5-qt5 FreeBSDQt5.13
 Date of build:
Thu, 31 Oct 2019 06:01:44 +
 Build duration:
24 min 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

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

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks/job/kservice/job/kf5-qt5%20FreeBSDQt5.13/47/
 Project:
kf5-qt5 FreeBSDQt5.13
 Date of build:
Thu, 31 Oct 2019 05:58:39 +
 Build duration:
23 min and counting
   JUnit Tests
  Name: projectroot Failed: 2 test(s), Passed: 7 test(s), Skipped: 0 test(s), Total: 9 test(s)Failed: projectroot.autotests.kmimeassociationstestFailed: projectroot.autotests.ksycoca_xdgdirstestName: projectroot.tests Failed: 0 test(s), Passed: 1 test(s), Skipped: 0 test(s), Total: 1 test(s)

D25039: Fix Clazy performance issues, const

[Méven Car]

meven edited the summary of this revision.

REPOSITORY
  R241 KIO

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

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

D25039: Fix Clazy performance issues, const

[Méven Car]

meven updated this revision to Diff 69082.
meven edited the summary of this revision.
meven added a comment.


  amend commit message

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25039?vs=69081=69082

BRANCH
  arcpatch-D25039

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

AFFECTED FILES
  src/core/copyjob.cpp
  src/core/deletejob.cpp
  src/core/kcoredirlister.cpp
  src/core/slavebase.cpp
  src/core/slavebase.h
  src/core/slaveinterface.cpp
  src/filewidgets/kdiroperator.cpp
  src/filewidgets/knewfilemenu.cpp
  src/gui/faviconrequestjob.cpp
  src/ioslaves/ftp/ftp.cpp
  src/ioslaves/help/kio_help.cpp
  src/ioslaves/http/http.cpp
  src/ioslaves/http/kcookiejar/kcookiejar.cpp
  src/ioslaves/http/kcookiejar/kcookieserver.h
  src/ioslaves/trash/tests/testtrash.cpp
  src/ioslaves/trash/trashimpl.cpp
  src/widgets/kdirmodel.cpp
  src/widgets/kfileitemactions.cpp
  src/widgets/kpropertiesdialog.cpp
  src/widgets/krun.cpp

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

D25039: Fix Clazy performance issues, const

[Méven Car]

meven retitled this revision from "Fix Clazy performance issues, const &, 
noexcept" to "Fix Clazy performance issues, const &".

REPOSITORY
  R241 KIO

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

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

D25039: Fix Clazy performance issues, const &, noexcept

[Méven Car]

meven updated this revision to Diff 69081.
meven marked 5 inline comments as done.
meven added a comment.


  Remove noexcept changes, avoid touching kpac, some code formatting

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25039?vs=68955=69081

BRANCH
  arcpatch-D25039

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

AFFECTED FILES
  src/core/copyjob.cpp
  src/core/deletejob.cpp
  src/core/kcoredirlister.cpp
  src/core/slavebase.cpp
  src/core/slavebase.h
  src/core/slaveinterface.cpp
  src/filewidgets/kdiroperator.cpp
  src/filewidgets/knewfilemenu.cpp
  src/gui/faviconrequestjob.cpp
  src/ioslaves/ftp/ftp.cpp
  src/ioslaves/help/kio_help.cpp
  src/ioslaves/http/http.cpp
  src/ioslaves/http/kcookiejar/kcookiejar.cpp
  src/ioslaves/http/kcookiejar/kcookieserver.h
  src/ioslaves/trash/tests/testtrash.cpp
  src/ioslaves/trash/trashimpl.cpp
  src/widgets/kdirmodel.cpp
  src/widgets/kfileitemactions.cpp
  src/widgets/kpropertiesdialog.cpp
  src/widgets/krun.cpp

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