D28606: Add iconSize property to PC2 ToolButton

[Noah Davis]

ndavis added a comment.


  In D28606#642513 , @ngraham wrote:
  
  > Darn. Maybe we should fix the PC3 tooltips though? :)
  
  
  Maybe, but I don't know what that would entail.
  For more context: I asked Kai about porting notifications to PC3 so that I 
could fix the size of icons with larger fonts. He said he wouldn't accept 
patches that do that for notifications or KRunner because of the issues with 
PC3, so I figured it would be easier to just add icon sizes to PC2.

REPOSITORY
  R242 Plasma Framework (Library)

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

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

D28606: Add iconSize property to PC2 ToolButton

[Noah Davis]

ndavis added a comment.


  In D28606#642514 , @ngraham wrote:
  
  > Adding Marco as a reviewer since I know he just lves 
ToolButton sizing. :)
  
  
  This doesn't actually affect toolbutton sizing, just icon sizing. Although, 
if you make the icon size too big, it'll bleed out of the button.

REPOSITORY
  R242 Plasma Framework (Library)

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

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

D28606: Add iconSize property to PC2 ToolButton

[Nathaniel Graham]

ngraham added a reviewer: mart.
ngraham added a comment.


  Adding Marco as a reviewer since I know he just lves 
ToolButton sizing. :)

REPOSITORY
  R242 Plasma Framework (Library)

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

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

D28606: Add iconSize property to PC2 ToolButton

[Nathaniel Graham]

ngraham added a comment.


  Darn. Maybe we should fix the PC3 tooltips though? :)

REPOSITORY
  R242 Plasma Framework (Library)

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

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

D28606: Add iconSize property to PC2 ToolButton

[Noah Davis]

ndavis added a subscriber: broulik.
ndavis added a comment.


  In D28606#642509 , @ngraham wrote:
  
  > If the developer has these needs, why not port to the PC3 version, which 
has this feature already?
  
  
  @broulik said we can't do that for some things like notifications and KRunner 
because of how the tooltips work.

REPOSITORY
  R242 Plasma Framework (Library)

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

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

D28606: Add iconSize property to PC2 ToolButton

[Nathaniel Graham]

ngraham added a comment.


  If the developer has these needs, why not port to the PC3 version, which has 
this feature already?

REPOSITORY
  R242 Plasma Framework (Library)

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

To: ndavis, #plasma
Cc: ngraham, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, bruns

D28606: Add iconSize property to PC2 ToolButton

[Noah Davis]

ndavis added a comment.


  I'm getting a binding loop: : QML QQuickLayoutAttached: Binding 
loop detected for property "minimumWidth"
  I'm guessing that's `Layout.minimumWidth` inside the `ToolButtonStyle` 
`IconItem`

REPOSITORY
  R242 Plasma Framework (Library)

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

To: ndavis, #plasma
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28606: Add iconSize property to PC2 ToolButton

[Noah Davis]

ndavis edited the summary of this revision.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: ndavis, #plasma
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28606: Add iconSize to PC2 ToolButton

[Noah Davis]

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

REVISION SUMMARY
  iconSize sets the size of the ToolButton's icon if it is set to a value.
  If no value is set, the ToolButtonStyle IconItem's minimumWidth is bound to 
buttonContent.height like it was before this patch and iconSize is bound to the 
paintedWidth of the icon.
  
  Usecase: In places where a custom icon size is wanted, a developer must 
create an IconItem nested in the ToolButton. This makes it so a developer only 
needs to change a single property.

TEST PLAN
  1. Set iconSize for a PC2 ToolButton and see if the icon size is changed.
  2. Bind the width of a component to a ToolButton's iconSize and see if it 
works.3. Do step 2, but don't do step 1 and see if it works

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  pc2-toolbutton-iconSize (branched from master)

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

AFFECTED FILES
  src/declarativeimports/plasmacomponents/qml/ToolButton.qml
  src/declarativeimports/plasmastyle/ToolButtonStyle.qml

To: ndavis, #plasma
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28606: Add iconSize property to PC2 ToolButton

[Noah Davis]

ndavis retitled this revision from "Add iconSize to PC2 ToolButton" to "Add 
iconSize property to PC2 ToolButton".

REPOSITORY
  R242 Plasma Framework (Library)

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

To: ndavis, #plasma
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28573: Add 16px System Settings icon

[Nathaniel Graham]

This revision was automatically updated to reflect the committed changes.
Closed by commit R266:b8b972a547d1: Add 16px System Settings icon (authored by 
manueljlin, committed by ngraham).

REPOSITORY
  R266 Breeze Icons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28573?vs=79340=79442

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

AFFECTED FILES
  icons-dark/apps/16/preferences-system.svg
  icons-dark/apps/16/systemsettings.svg
  icons/apps/16/preferences-system.svg
  icons/apps/16/systemsettings.svg

To: manueljlin, #vdg, ndavis, ngraham
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D14598: Keep checking return value from calling browse_stat_path() in SMBSlave::stat() in sync with SMBSlave::listDir()

[Ralf Habacker]

habacker closed this revision.
habacker added a comment.


  This review request was superseeded by D16272 
.

REPOSITORY
  R320 KIO Extras

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

To: habacker, dfaure
Cc: kde-frameworks-devel, kfm-devel, azyx, nikolaik, pberestov, iasensio, 
fprice, LeGast00n, cblack, fbampaloukas, alexde, GB_2, Codezela, feverfew, 
meven, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, 
emmanuelp, rdieter, mikesomov

KDE CI: Frameworks » plasma-framework » kf5-qt5 SUSEQt5.14 - Build # 11 - Fixed!

[CI System]

BUILD SUCCESS
 Build URL
https://build.kde.org/job/Frameworks/job/plasma-framework/job/kf5-qt5%20SUSEQt5.14/11/
 Project:
kf5-qt5 SUSEQt5.14
 Date of build:
Mon, 06 Apr 2020 00:22:20 +
 Build duration:
5 min 14 sec and counting
   BUILD ARTIFACTS
  abi-compatibility-results.yamlacc/KF5Plasma-5.69.0.xmlacc/KF5PlasmaQuick-5.69.0.xmlcompat_reports/KF5Plasma_compat_report.htmllogs/KF5Plasma/5.69.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: 11 test(s), Skipped: 0 test(s), Total: 11 test(s)
   Cobertura Report
  
   Project Coverage Summary
  
   Name
  PackagesFilesClassesLinesConditionalsCobertura Coverage Report44%
(8/18)36%
(45/126)36%
(45/126)35%
(4633/13362)26%
(2525/9714)Coverage Breakdown by Package
Name
   FilesClassesLinesConditionalsautotests92%
(11/12)92%
(11/12)95%
(869/916)52%
(439/852)src.declarativeimports.calendar0%
(0/6)0%
(0/6)0%
(0/472)0%
(0/245)src.declarativeimports.core44%
(7/16)44%
(7/16)34%
(775/2311)26%
(395/1499)src.declarativeimports.plasmacomponents0%
(0/6)0%
(0/6)0%
(0/518)0%
(0/207)src.declarativeimports.plasmaextracomponents0%
(0/3)0%
(0/3)0%
(0/42)0%
(0/22)src.declarativeimports.platformcomponents0%
(0/3)0%
(0/3)0%
(0/59)0%
(0/14)src.declarativeimports.platformcomponents.utils0%
(0/2)0%
(0/2)0%
(0/14)0%
(0/2)src.plasma45%
(10/22)45%
(10/22)43%
(1533/3567)33%
(954/2894)src.plasma.packagestructure43%
(3/7)43%
(3/7)36%
(49/137)42%
(5/12)src.plasma.private39%
(7/18)39%
(7/18)48%
(750/1578)37%
(384/1045)src.plasma.scripting33%
(1/3)33%
(1/3)12%
(20/173)7%
(7/105)src.plasmapkg0%
(0/1)0%
(0/1)0%
(0/45)0%
(0/40)src.plasmaquick38%
(5/13)38%
(5/13)29%
(606/2086)19%
(336/1745)src.plasmaquick.private50%
(1/2)50%
(1/2)29%
(31/106)36%
(5/14)src.scriptengines.qml.plasmoid0%
(0/7)0%
(0/7)0%
(0/1183)0%
(0/994)tests.dpi0%
(0/2)0%
(0/2)0%
(0/21)0%
(0/2)tests.kplugins0%
 

KDE CI: Frameworks » plasma-framework » kf5-qt5 SUSEQt5.12 - Build # 333 - Fixed!

[CI System]

BUILD SUCCESS
 Build URL
https://build.kde.org/job/Frameworks/job/plasma-framework/job/kf5-qt5%20SUSEQt5.12/333/
 Project:
kf5-qt5 SUSEQt5.12
 Date of build:
Mon, 06 Apr 2020 00:22:20 +
 Build duration:
4 min 31 sec and counting
   BUILD ARTIFACTS
  abi-compatibility-results.yamlacc/KF5Plasma-5.69.0.xmlacc/KF5PlasmaQuick-5.69.0.xmlcompat_reports/KF5Plasma_compat_report.htmllogs/KF5Plasma/5.69.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: 11 test(s), Skipped: 0 test(s), Total: 11 test(s)
   Cobertura Report
  
   Project Coverage Summary
  
   Name
  PackagesFilesClassesLinesConditionalsCobertura Coverage Report44%
(8/18)36%
(45/126)36%
(45/126)35%
(4637/13364)26%
(2526/9714)Coverage Breakdown by Package
Name
   FilesClassesLinesConditionalsautotests92%
(11/12)92%
(11/12)95%
(869/916)52%
(439/852)src.declarativeimports.calendar0%
(0/6)0%
(0/6)0%
(0/472)0%
(0/245)src.declarativeimports.core44%
(7/16)44%
(7/16)34%
(775/2311)26%
(395/1499)src.declarativeimports.plasmacomponents0%
(0/6)0%
(0/6)0%
(0/518)0%
(0/207)src.declarativeimports.plasmaextracomponents0%
(0/3)0%
(0/3)0%
(0/42)0%
(0/22)src.declarativeimports.platformcomponents0%
(0/3)0%
(0/3)0%
(0/59)0%
(0/14)src.declarativeimports.platformcomponents.utils0%
(0/2)0%
(0/2)0%
(0/14)0%
(0/2)src.plasma45%
(10/22)45%
(10/22)43%
(1533/3567)33%
(954/2894)src.plasma.packagestructure43%
(3/7)43%
(3/7)36%
(49/137)42%
(5/12)src.plasma.private39%
(7/18)39%
(7/18)48%
(750/1578)37%
(384/1045)src.plasma.scripting33%
(1/3)33%
(1/3)12%
(20/173)7%
(7/105)src.plasmapkg0%
(0/1)0%
(0/1)0%
(0/45)0%
(0/40)src.plasmaquick38%
(5/13)38%
(5/13)29%
(610/2088)19%
(337/1745)src.plasmaquick.private50%
(1/2)50%
(1/2)29%
(31/106)36%
(5/14)src.scriptengines.qml.plasmoid0%
(0/7)0%
(0/7)0%
(0/1183)0%
(0/994)tests.dpi0%
(0/2)0%
(0/2)0%
(0/21)0%
(0/2)tests.kplugins0%

KDE CI: Frameworks » plasma-framework » kf5-qt5 FreeBSDQt5.14 - Build # 15 - Fixed!

[CI System]

BUILD SUCCESS
 Build URL
https://build.kde.org/job/Frameworks/job/plasma-framework/job/kf5-qt5%20FreeBSDQt5.14/15/
 Project:
kf5-qt5 FreeBSDQt5.14
 Date of build:
Mon, 06 Apr 2020 00:22:22 +
 Build duration:
3 min 6 sec and counting
   JUnit Tests
  Name: projectroot Failed: 0 test(s), Passed: 11 test(s), Skipped: 0 test(s), Total: 11 test(s)

D28033: Create ExpandableListItem

[Nathaniel Graham]

ngraham added a comment.


  Fixed in 96d203b4b86e829fa958a8aa2ac42c8f5e1a4d31 
.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: ngraham, #vdg, #plasma, davidedmundson
Cc: dfaure, mart, davidedmundson, bruns, niccolove, cblack, davidre, 
kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham

D28033: Create ExpandableListItem

[Nathaniel Graham]

ngraham added a comment.


  Oops, sorry. Will fix.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: ngraham, #vdg, #plasma, davidedmundson
Cc: dfaure, mart, davidedmundson, bruns, niccolove, cblack, davidre, 
kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham

KDE CI: Frameworks » kio » kf5-qt5 FreeBSDQt5.14 - Build # 39 - Fixed!

[CI System]

BUILD SUCCESS
 Build URL
https://build.kde.org/job/Frameworks/job/kio/job/kf5-qt5%20FreeBSDQt5.14/39/
 Project:
kf5-qt5 FreeBSDQt5.14
 Date of build:
Sun, 05 Apr 2020 22:04:06 +
 Build duration:
8 min 3 sec and counting
   JUnit Tests
  Name: projectroot Failed: 0 test(s), Passed: 54 test(s), Skipped: 0 test(s), Total: 54 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)

D28603: API dox: document more of the default property values of KUrlRequester

[Friedrich W. H. Kossebau]

This revision was automatically updated to reflect the committed changes.
Closed by commit R241:d31ebc6aade3: API dox: document more of the default 
property values of KUrlRequester (authored by kossebau).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28603?vs=79432=79434

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

AFFECTED FILES
  src/widgets/kurlrequester.h

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

D28603: API dox: document more of the default property values of KUrlRequester

[David Faure]

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

REPOSITORY
  R241 KIO

BRANCH
  documentmorekrulrequesterdefaults

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

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

D28601: Fix DirectorySizeJob so it doesn't depend on listing order

[David Faure]

dfaure closed this revision.

REPOSITORY
  R241 KIO

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

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

D28603: API dox: document more of the default property values of KUrlRequester

[Friedrich W. H. Kossebau]

kossebau created this revision.
kossebau added reviewers: Frameworks, meven, dfaure, ahmadsamir.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
kossebau requested review of this revision.

REPOSITORY
  R241 KIO

BRANCH
  documentmorekrulrequesterdefaults

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

AFFECTED FILES
  src/widgets/kurlrequester.h

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

D28444: WIP/RFC: Add ECMCargo module

[Carson Black]

cblack updated this revision to Diff 79430.
cblack added a comment.


  Address feedback

REPOSITORY
  R240 Extra CMake Modules

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28444?vs=78925=79430

BRANCH
  cblack/cargo-integration

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

AFFECTED FILES
  modules/ECMCargo.cmake

To: cblack, #frameworks, #build_system
Cc: apol, IlyaBizyaev, kde-frameworks-devel, kde-buildsystem, LeGast00n, 
cblack, GB_2, bencreasy, michaelh, ngraham, bruns

D28444: Add ECMCargo module

[Carson Black]

cblack retitled this revision from "WIP/RFC: Add ECMCargo module" to "Add 
ECMCargo module".
cblack edited the summary of this revision.

REPOSITORY
  R240 Extra CMake Modules

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

To: cblack, #frameworks, #build_system
Cc: apol, IlyaBizyaev, kde-frameworks-devel, kde-buildsystem, LeGast00n, 
cblack, GB_2, bencreasy, michaelh, ngraham, bruns

D28553: Rename stylesheet color ButtonFocus -> Highlight

[Noah Davis]

ndavis added a comment.


  @davidhurka Actually, I'm using Highlight. I still can't get ButtonFocus to 
work reliably and Highlight in icons now changes to white when a UI element 
gets highlighted, which is good. BTW, join the VDG chat if you haven't already.

REPOSITORY
  R266 Breeze Icons

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

To: davidhurka, ndavis
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28601: Fix DirectorySizeJob so it doesn't depend on listing order

[Ahmad Samir]

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


  I am sure the test passed locally many times before I submitted D28472 
 (and it still passes).
  
  But indeed it shouldn't depend on which was listed first, the dir or the link 
to it (retrospective wisdom on my part :D).

REPOSITORY
  R241 KIO

BRANCH
  master

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

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

D28579: Improve the look of kcachegrind

[Noah Davis]

This revision was automatically updated to reflect the committed changes.
Closed by commit R266:0e3911c84d37: Improve the look of kcachegrind (authored 
by ndavis).

REPOSITORY
  R266 Breeze Icons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28579?vs=79359=79428

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

AFFECTED FILES
  icons-dark/apps/48/kcachegrind.svg
  icons/apps/48/kcachegrind.svg

To: ndavis, #vdg, cblack, ngraham
Cc: apol, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D28472: [DirectorySizeJob] Fix sub-dirs count when resolving symlinks to dirs

[David Faure]

dfaure added a comment.


  Suggested fix in D28601 

REPOSITORY
  R241 KIO

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

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

D28601: Fix DirectorySizeJob so it doesn't depend on listing order

[David Faure]

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

TEST PLAN
  jobtest passes locally again

REPOSITORY
  R241 KIO

BRANCH
  master

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

AFFECTED FILES
  src/core/directorysizejob.cpp

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

D28472: [DirectorySizeJob] Fix sub-dirs count when resolving symlinks to dirs

[David Faure]

dfaure added a comment.


  Does the test pass for you now?
  
  For me it fails (and it fails on FreeBSD CI too 
https://build.kde.org/job/Frameworks/job/kio/job/kf5-qt5%20FreeBSDQt5.14/37/testReport/junit/projectroot/autotests/kiocore_jobtest/)
  
  FAIL!  : JobTest::directorySize() Compared values are not the same
  
Actual   (job->totalSubdirs()): 3
Expected (4ULL)   : 4
Loc: [/d/kde/src/5/frameworks/kio/autotests/jobtest.cpp(1133)]
  
  What's happening is
  
  QDEBUG : JobTest::directorySize() KIO::DirectorySizeJobPrivate::slotEntries 
subdir "dirFromHome" => 1
  QDEBUG : JobTest::directorySize() KIO::DirectorySizeJobPrivate::slotEntries 
Skip visited inode "dirFromHome_link"
  QDEBUG : JobTest::directorySize() KIO::DirectorySizeJobPrivate::slotEntries 
subdir "dirFromHome_copied" => 2
  QDEBUG : JobTest::directorySize() KIO::DirectorySizeJobPrivate::slotEntries 
subdir "dirFromHome_copied/dirFromHome" => 3
  
  http://www.davidfaure.fr/2020/debug.diff
  
  I think the problem is that order is undefined. If the symlink is seen first, 
both will be counted (your if() is only around the insert). If it's seen 
second, it'll be skipped.
  
  I think we should skip the whole visitedInodes thing for symlinks, not just 
the insert.

REPOSITORY
  R241 KIO

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

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

D28599: [configview] Simplify code / workaround Qt5.15 crash

[Aleix Pol Gonzalez]

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


  LGTM

INLINE COMMENTS

> configview.cpp:137
>  
> +
> q->engine()->rootContext()->setContextProperty(QStringLiteral("plasmoid"), 
> applet.data()->property("_plasma_graphicObject").value());
> +
> q->engine()->rootContext()->setContextProperty(QStringLiteral("configDialog"),
>  q);

It could make sense to use the opportunity to change to 
QQmlContext::setContextProperties.

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  master

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

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

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

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks/job/kio/job/kf5-qt5%20FreeBSDQt5.14/38/
 Project:
kf5-qt5 FreeBSDQt5.14
 Date of build:
Sun, 05 Apr 2020 17:51:46 +
 Build duration:
6 min 14 sec and counting
   JUnit Tests
  Name: projectroot Failed: 1 test(s), Passed: 53 test(s), Skipped: 0 test(s), Total: 54 test(s)Failed: projectroot.autotests.kiocore_jobtestName: 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 FreeBSDQt5.14 - Build # 37 - Unstable!

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks/job/kio/job/kf5-qt5%20FreeBSDQt5.14/37/
 Project:
kf5-qt5 FreeBSDQt5.14
 Date of build:
Sun, 05 Apr 2020 17:13:56 +
 Build duration:
6 min 35 sec and counting
   JUnit Tests
  Name: projectroot Failed: 1 test(s), Passed: 53 test(s), Skipped: 0 test(s), Total: 54 test(s)Failed: projectroot.autotests.kiocore_jobtestName: 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)

D28599: [configview] Simplify code / workaround Qt5.15 crash

[David Edmundson]

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

REVISION SUMMARY
  QQmlComponent::beginCreate and completeCreate are useful if you need to
  set properties on the object explicitly. We're not doing that here, we
  can just call create.

TEST PLAN
  On Qt5.15
  Right click a panel
  choose edit

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  master

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

AFFECTED FILES
  src/plasmaquick/configview.cpp

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

D28598: KRun: fix assert when failing to start an application

[David Faure]

dfaure closed this revision.

REPOSITORY
  R241 KIO

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

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

D28598: KRun: fix assert when failing to start an application

[Ahmad Samir]

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


  Of course. That makes much more sense than the band aid fix in the (my) other 
diff.

REPOSITORY
  R241 KIO

BRANCH
  master

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

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

D28592: [ApplicationLauncherJob] Guard against m_pids being empty

[Ahmad Samir]

ahmadsamir abandoned this revision.

REPOSITORY
  R241 KIO

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

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

D27216: [KProcessRunner] Improve error messages on failure

[David Faure]

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

INLINE COMMENTS

> kprocessrunner.cpp:279
> +if (exitCode != 0) {
> +KMessageBox::error(nullptr, i18n("The program '%1' terminated 
> abnormally.\n"
> + "For more details try running it 
> from a terminal emulator (e.g. Konsole).",

Now that KProcessRunner is in KioGui, it's not supposed to use any message 
boxes.

It's used by jobs which emit errors, so it should just pass along the error.

HOWEVER the jobs finish as soon as the application has started.
By the time the process exits, the job is already gone and we have no way to 
relay errors to the user.

We don't want jobs to keep running for 5 hours if the application is used for 5 
hours. The success of the job is that the application started, not that it 
finished.

But maybe we should reconsider this. E.g. the job could survive for just 
another second, to wait for very early exits like this one.

REPOSITORY
  R241 KIO

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

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

D27216: [KProcessRunner] Improve error messages on failure

[David Faure]

dfaure added reviewers: davidedmundson, broulik.

REPOSITORY
  R241 KIO

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

To: ahmadsamir, #frameworks, dfaure, davidedmundson, broulik
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

KDE CI: Frameworks » ktextwidgets » kf5-qt5 WindowsMSVCQt5.14 - Build # 17 - Unstable!

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks/job/ktextwidgets/job/kf5-qt5%20WindowsMSVCQt5.14/17/
 Project:
kf5-qt5 WindowsMSVCQt5.14
 Date of build:
Sun, 05 Apr 2020 17:02:16 +
 Build duration:
1 min 55 sec and counting
   JUnit Tests
  Name: projectroot Failed: 1 test(s), Passed: 4 test(s), Skipped: 0 test(s), Total: 5 test(s)Failed: projectroot.autotests.ktextwidgets_kfindtest

KDE CI: Frameworks » ktextwidgets » kf5-qt5 SUSEQt5.12 - Build # 77 - Unstable!

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks/job/ktextwidgets/job/kf5-qt5%20SUSEQt5.12/77/
 Project:
kf5-qt5 SUSEQt5.12
 Date of build:
Sun, 05 Apr 2020 17:02:15 +
 Build duration:
1 min 44 sec and counting
   BUILD ARTIFACTS
  abi-compatibility-results.yamlacc/KF5TextWidgets-5.69.0.xmlcompat_reports/KF5TextWidgets_compat_report.htmllogs/KF5TextWidgets/5.69.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: 1 test(s), Passed: 4 test(s), Skipped: 0 test(s), Total: 5 test(s)Failed: projectroot.autotests.ktextwidgets_kfindtest
   Cobertura Report
  
   Project Coverage Summary
  
   Name
  PackagesFilesClassesLinesConditionalsCobertura Coverage Report60%
(3/5)82%
(18/22)82%
(18/22)50%
(1529/3056)39%
(574/1477)Coverage Breakdown by Package
Name
   FilesClassesLinesConditionalsautotests100%
(7/7)100%
(7/7)97%
(622/639)59%
(137/233)src.dialogs0%
(0/1)0%
(0/1)0%
(0/40)0%
(0/4)src.findreplace100%
(7/7)100%
(7/7)63%
(598/944)51%
(266/520)src.widgets67%
(4/6)67%
(4/6)22%
(309/1422)24%
(171/718)tests0%
(0/1)0%
(0/1)0%
(0/11)0%
(0/2)

KDE CI: Frameworks » ktextwidgets » kf5-qt5 FreeBSDQt5.14 - Build # 5 - Unstable!

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks/job/ktextwidgets/job/kf5-qt5%20FreeBSDQt5.14/5/
 Project:
kf5-qt5 FreeBSDQt5.14
 Date of build:
Sun, 05 Apr 2020 17:02:16 +
 Build duration:
56 sec and counting
   JUnit Tests
  Name: projectroot Failed: 1 test(s), Passed: 4 test(s), Skipped: 0 test(s), Total: 5 test(s)Failed: projectroot.autotests.ktextwidgets_kfindtest

KDE CI: Frameworks » ktextwidgets » kf5-qt5 SUSEQt5.14 - Build # 4 - Unstable!

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks/job/ktextwidgets/job/kf5-qt5%20SUSEQt5.14/4/
 Project:
kf5-qt5 SUSEQt5.14
 Date of build:
Sun, 05 Apr 2020 17:02:15 +
 Build duration:
1 min 47 sec and counting
   BUILD ARTIFACTS
  abi-compatibility-results.yamlacc/KF5TextWidgets-5.69.0.xmlcompat_reports/KF5TextWidgets_compat_report.htmllogs/KF5TextWidgets/5.69.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: 1 test(s), Passed: 4 test(s), Skipped: 0 test(s), Total: 5 test(s)Failed: projectroot.autotests.ktextwidgets_kfindtest
   Cobertura Report
  
   Project Coverage Summary
  
   Name
  PackagesFilesClassesLinesConditionalsCobertura Coverage Report60%
(3/5)82%
(18/22)82%
(18/22)50%
(1528/3055)39%
(574/1477)Coverage Breakdown by Package
Name
   FilesClassesLinesConditionalsautotests100%
(7/7)100%
(7/7)97%
(621/638)59%
(137/233)src.dialogs0%
(0/1)0%
(0/1)0%
(0/40)0%
(0/4)src.findreplace100%
(7/7)100%
(7/7)63%
(598/944)51%
(266/520)src.widgets67%
(4/6)67%
(4/6)22%
(309/1422)24%
(171/718)tests0%
(0/1)0%
(0/1)0%
(0/11)0%
(0/2)

D27097: [KTextWidgets] Port from QRegExp to QRegularExpression

[Ahmad Samir]

This revision was automatically updated to reflect the committed changes.
Closed by commit R310:d7ec040f5aa0: [KTextWidgets] Port from QRegExp to 
QRegularExpression (authored by ahmadsamir).

REPOSITORY
  R310 KTextWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27097?vs=79373=79422

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

AFFECTED FILES
  autotests/kfindtest.cpp
  autotests/kfindtest.h
  autotests/krichtextedittest.cpp
  src/CMakeLists.txt
  src/findreplace/kfind.cpp
  src/findreplace/kfind.h
  src/findreplace/kfinddialog.cpp
  src/findreplace/kreplace.cpp
  src/findreplace/kreplace.h
  src/widgets/krichtextedit.cpp

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

D27097: [KTextWidgets] Port from QRegExp to QRegularExpression

[David Faure]

dfaure added a comment.


  It's tagged, you can go ahead.

REPOSITORY
  R310 KTextWidgets

BRANCH
  l-qregularexpression (branched from master)

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

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

D28598: KRun: fix assert when failing to start an application

[David Faure]

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

TEST PLAN
  'sudo chmod a-x /usr/bin/gwenview'
  Try opening a picure file from e.g. dolphin, you get an error message as 
expected, clicking OK makes dolphin crash because m_pids is empty and we hit an 
assert in QVector

REPOSITORY
  R241 KIO

BRANCH
  master

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

AFFECTED FILES
  src/widgets/krun.cpp

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

D28592: [ApplicationLauncherJob] Guard against m_pids being empty

[David Faure]

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


  This, hmm, sounds wrong. Is the caller calling pid() even though the job 
finished with an error code? Ah, yes, indeed, in the case of KRun. Whoops.
  
  Here's my suggested fix instead: D28598 

REPOSITORY
  R241 KIO

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

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

D27097: [KTextWidgets] Port from QRegExp to QRegularExpression

[Ahmad Samir]

ahmadsamir added a comment.


  I think I should wait until 5.69 is released before landing, right?

REPOSITORY
  R310 KTextWidgets

BRANCH
  l-qregularexpression (branched from master)

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

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

D28535: [KIO-MTP] Fix null pointer dereference

[Alexander Saoutkin]

feverfew added a comment.


  In D28535#642289 , 
@elvisangelaccio wrote:
  
  > In D28535#640833 , @fvogt wrote:
  >
  > > I assume there is a reason why `MTPDevice::getDevice()` has code for 
handling this very specific case, so I wouldn't just remove it without figuring 
out why: https://i.redd.it/hfnl7xo8yovy.gif
  > >
  > > If not, that would indeed be the best option.
  >
  >
  > Unfortunately git blame doesn't seem to help us here.
  >
  > I suggest to push this fix to master only and see what happens.
  
  
  By pushing this to master would we still be able to throw it up to 20.04.* if 
we decide it's stable enough?

REPOSITORY
  R320 KIO Extras

BRANCH
  fixNullPtr (branched from master)

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

To: feverfew, akrutzler, dfaure, elvisangelaccio
Cc: anthonyfieroni, kde-frameworks-devel, fvogt, kfm-devel, ngraham, nikolaik, 
pberestov, iasensio, fprice, LeGast00n, cblack, fbampaloukas, alexde, GB_2, 
Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, 
andrebarros, bruns, emmanuelp, rdieter, mikesomov

D27097: [KTextWidgets] Port from QRegExp to QRegularExpression

[David Faure]

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


  Thanks!

REPOSITORY
  R310 KTextWidgets

BRANCH
  l-qregularexpression (branched from master)

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

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

D28535: [KIO-MTP] Fix null pointer dereference

[Anthony Fieroni]

anthonyfieroni added a comment.


  @elvisangelaccio this peace of code is purely wrong at least `m_storages` is 
not updated to new device and not only. This code should never exists or try to 
hide some other bug.

REPOSITORY
  R320 KIO Extras

BRANCH
  fixNullPtr (branched from master)

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

To: feverfew, akrutzler, dfaure, elvisangelaccio
Cc: anthonyfieroni, kde-frameworks-devel, fvogt, kfm-devel, ngraham, nikolaik, 
pberestov, iasensio, fprice, LeGast00n, cblack, fbampaloukas, alexde, GB_2, 
Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, 
andrebarros, bruns, emmanuelp, rdieter, mikesomov

D28535: [KIO-MTP] Fix null pointer dereference

[Elvis Angelaccio]

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


  In D28535#640833 , @fvogt wrote:
  
  > I assume there is a reason why `MTPDevice::getDevice()` has code for 
handling this very specific case, so I wouldn't just remove it without figuring 
out why: https://i.redd.it/hfnl7xo8yovy.gif
  >
  > If not, that would indeed be the best option.
  
  
  Unfortunately git blame doesn't seem to help us here.
  
  I suggest to push this fix to master only and see what happens.

REPOSITORY
  R320 KIO Extras

BRANCH
  fixNullPtr (branched from master)

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

To: feverfew, akrutzler, dfaure, elvisangelaccio
Cc: anthonyfieroni, kde-frameworks-devel, fvogt, kfm-devel, ngraham, nikolaik, 
pberestov, iasensio, fprice, LeGast00n, cblack, fbampaloukas, alexde, GB_2, 
Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, 
andrebarros, bruns, emmanuelp, rdieter, mikesomov

D28528: UDSEntry: add constructor variant with std::initializer_list

[David Faure]

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

INLINE COMMENTS

> udsentry.h:132
> +const long long l;
> +char s[sizeof(QString)];
> +} m_u;

Why not simply QString*?

> udsentrybenchmark.cpp:141
> +};
> +m_smallEntries.append(entry);
> +}

std::move(entry) if you want to skip the copying?

> udsentrybenchmark.cpp:145
> +
> +Q_ASSERT(m_smallEntries.count() == numberOfSmallUDSEntries);
> +}

QCOMPARE()

REPOSITORY
  R241 KIO

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

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

D28520: Fix lifetime of slot in KIO-MTP

[Elvis Angelaccio]

elvisangelaccio accepted this revision.
elvisangelaccio added a comment.


  Please push to 20.04, there is still time.

REPOSITORY
  R320 KIO Extras

BRANCH
  slotLifetime (branched from master)

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

To: feverfew, akrutzler, dfaure, elvisangelaccio, apol, meven
Cc: apol, kde-frameworks-devel, kfm-devel, fvogt, nikolaik, pberestov, 
iasensio, fprice, LeGast00n, cblack, fbampaloukas, alexde, GB_2, Codezela, 
feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, rdieter, mikesomov

D28478: [FileProtocol] change statx stat_dev() to return makedev(major, minor)

[Méven Car]

meven accepted this revision.

REPOSITORY
  R241 KIO

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

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

D28590: Add a QString Solid::Device::label, used in Fstab Device for network mounts

[Méven Car]

meven marked an inline comment as done.
meven added a comment.


  In D28590#642133 , @bruns wrote:
  
  > Btw, label() is a bad name, it can be confused with the filesystem label to 
easily. Maybe shortName().
  
  
  I meant to reuse this name on purpose as it serves the same use.
  I would favor `displayName()` as it is reminiscent somewhat of 
Qt::DisplayRole and clearer than `name`.
  `shortName` implies there is another longer name somewhere, describing what 
it contains rather than what use case it fulfills.
  
  In D28590#642130 , @bruns wrote:
  
  > I think it would be better to not change behaviour for any backend, but 
just default label() to description() everywhere.
  
  
  So you mean I should split commit the label/displayName() addition and the 
fstabdevice change ?
  I was keeping label() as an alias to description() everywhere expect in 
RootDevice, and FstabDevice.
  
  I didn't know how to avoid defining label() everywhere because of the way the 
Solid interface is made.
  
  > Then in the next step, adjust description() and label() for each backend, 
shorten label where possible, and extend description() so it becomes more 
informative.
  
  Agreed but so in a subsequent diff(s), as it will change the meaning of 
description, which essentially will be a behavior change, potentially breaking 
UIs,
  We will need to look for the usage of all description and decide whether it 
should use the new function instead of description.

INLINE COMMENTS

> bruns wrote in fstabdevice.cpp:146
> filePath in most cases does not depend on isAccessible ... it would be 
> annoying if the label changed everytime the device is mounted.

Indeed thanks for pointing it out.

> bruns wrote in udisksdevice.cpp:234
> This would be the correct value only for label() now, no longer for the 
> description (which should be more verbose).

Can it be missing ? And what should be it then ?
I don't have an answer to this.
In the meantime I meant to keep label() as description() as a first step.

REPOSITORY
  R245 Solid

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

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

D28409: ECM: attempt to fix KDEInstallDirsTest.relative_or_absolute on Windows

[David Faure]

dfaure closed this revision.

REPOSITORY
  R240 Extra CMake Modules

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

To: dfaure, kossebau, apol, cgiboudeaux
Cc: kde-frameworks-devel, kde-buildsystem, LeGast00n, cblack, GB_2, bencreasy, 
michaelh, ngraham, bruns

D28409: ECM: attempt to fix KDEInstallDirsTest.relative_or_absolute on Windows

[Aleix Pol Gonzalez]

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

REPOSITORY
  R240 Extra CMake Modules

BRANCH
  master

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

To: dfaure, kossebau, apol, cgiboudeaux
Cc: kde-frameworks-devel, kde-buildsystem, LeGast00n, cblack, GB_2, bencreasy, 
michaelh, ngraham, bruns

D28409: ECM: attempt to fix KDEInstallDirsTest.relative_or_absolute on Windows

[David Faure]

dfaure added inline comments.

INLINE COMMENTS

> CMakeLists.txt:146
>  set(KDEInstallDirsTest.relative_or_absolute_usr_EXTRA_OPTIONS
>  --build-options -DCMAKE_INSTALL_PREFIX=/usr
>  -DKDE_INSTALL_USE_QT_SYS_PATHS=FALSE

This test uses /usr and passes on Windows.

It's not like those paths have to exist.

REPOSITORY
  R240 Extra CMake Modules

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

To: dfaure, kossebau, apol, cgiboudeaux
Cc: kde-frameworks-devel, kde-buildsystem, LeGast00n, cblack, GB_2, bencreasy, 
michaelh, ngraham, bruns

D28409: ECM: attempt to fix KDEInstallDirsTest.relative_or_absolute on Windows

[Aleix Pol Gonzalez]

apol added a comment.


  Will /tmp then still work on Windows?

REPOSITORY
  R240 Extra CMake Modules

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

To: dfaure, kossebau, apol, cgiboudeaux
Cc: kde-frameworks-devel, kde-buildsystem, LeGast00n, cblack, GB_2, bencreasy, 
michaelh, ngraham, bruns

D28478: [FileProtocol] change statx stat_dev() to return makedev(major, minor)

[Ahmad Samir]

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

INLINE COMMENTS

> bruns wrote in jobtest.h:88
> wrong name

Of course, the one time I forgot to build before using 'arc diff' in a long 
time...

REPOSITORY
  R241 KIO

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

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

D28478: [FileProtocol] change statx stat_dev() to return makedev(major, minor)

[Ahmad Samir]

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


  Fix test name in header
  Add one more QCOMPARE()

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28478?vs=79410=79411

BRANCH
  l-statx (branched from master)

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

AFFECTED FILES
  autotests/jobtest.cpp
  autotests/jobtest.h
  src/ioslaves/file/file_unix.cpp

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

D28478: [FileProtocol] change statx stat_dev() to return makedev(major, minor)

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> jobtest.cpp:1540
> +// On the CI where the two tmp dirs are on the only parition 
> available
> +// in the LXC container, the device ID's would be identical
> +}

QCOMPARE(device, otherDevice);

> jobtest.h:88
>  void statDetailsBasicSetDetails();
> +void statUniqueDeviceIDs();
>  #ifndef Q_OS_WIN

wrong name

REPOSITORY
  R241 KIO

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

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

D28478: [FileProtocol] change statx stat_dev() to return makedev(major, minor)

[Ahmad Samir]

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


  Address comments

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28478?vs=79339=79410

BRANCH
  l-statx (branched from master)

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

AFFECTED FILES
  autotests/jobtest.cpp
  autotests/jobtest.h
  src/ioslaves/file/file_unix.cpp

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

D28592: [ApplicationLauncherJob] Guard against m_pids being empty

[Ahmad Samir]

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

TEST PLAN
  - 'sudo chmod a-x /usr/bin/gwenview'
  - Try opening a picure file from e.g. dolphin, you get an error message as 
expected, clicking OK makes dolphin crash because m_pids is empty and we hit an 
assert in QVector

REPOSITORY
  R241 KIO

BRANCH
  l-pids (branched from master)

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

AFFECTED FILES
  src/gui/applicationlauncherjob.cpp

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

D28161: [kio-extras] Port some QRegExp usage to QRegularExpression

[Ahmad Samir]

This revision was automatically updated to reflect the committed changes.
Closed by commit R320:bc3003f77270: [kio-extras] Port some QRegExp usage to 
QRegularExpression (authored by ahmadsamir).

REPOSITORY
  R320 KIO Extras

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28161?vs=79406=79407

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

AFFECTED FILES
  bookmarks/kio_bookmarks.cpp
  fish/fish.cpp
  man/kio_man.cpp
  man/man2html.cpp

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

D28161: [kio-extras] Port some QRegExp usage to QRegularExpression

[Ahmad Samir]

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


  Rebase

REPOSITORY
  R320 KIO Extras

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28161?vs=78528=79406

BRANCH
  l-QRE (branched from master)

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

AFFECTED FILES
  bookmarks/kio_bookmarks.cpp
  fish/fish.cpp
  man/kio_man.cpp
  man/man2html.cpp

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

D28590: Add a QString Solid::Device::label, used in Fstab Device for network mounts

[Stefan Brüns]

bruns added a comment.


  Btw, label() is a bad name, it can be confused with the filesystem label to 
easily. Maybe shortName().

REPOSITORY
  R245 Solid

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

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

D28590: Add a QString Solid::Device::label, used in Fstab Device for network mounts

[Stefan Brüns]

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

REPOSITORY
  R245 Solid

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

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

D28590: Add a QString Solid::Device::label, used in Fstab Device for network mounts

[Stefan Brüns]

bruns added a comment.


  I think it would be better to not change behaviour for any backend, but just 
default label() to description() everywhere.
  
  Then in the next step, adjust description() and label() for each backend, 
shorten label where possible, and extend description() so it becomes more 
informative.

INLINE COMMENTS

> fstabdevice.cpp:146
> +if (m_storageAccess->isAccessible()) {
> +return m_storageAccess->filePath();
> +} else {

filePath in most cases does not depend on isAccessible ... it would be annoying 
if the label changed everytime the device is mounted.

> udisksdevice.cpp:234
>  {
>  const QString hintName = property("HintName").toString(); // non-cached
>  if (!hintName.isEmpty()) {

This would be the correct value only for label() now, no longer for the 
description (which should be more verbose).

REPOSITORY
  R245 Solid

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

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

D27216: [KProcessRunner] Improve error messages on failure

[Ahmad Samir]

ahmadsamir added a comment.


  The QSignalSpy stuff are fixed.
  
  However, krununittest won't work with QTEST_GUILESS_MAIN() as there's a 
dialog that wants to get displayed (from my previous post). However I still 
can't figure out why the cp command is existing with code 15...

REPOSITORY
  R241 KIO

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

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

D27216: [KProcessRunner] Improve error messages on failure

[Ahmad Samir]

ahmadsamir updated this revision to Diff 79399.
ahmadsamir retitled this revision from "[KProcessRunner] Improve error reported 
to user when exit code != 0" to "[KProcessRunner] Improve error messages on 
failure".
ahmadsamir edited the summary of this revision.
ahmadsamir edited the test plan for this revision.
ahmadsamir added a comment.


  Update and rebase

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27216?vs=75179=79399

BRANCH
  l-kprunner-message (branched from master)

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

AFFECTED FILES
  src/gui/CMakeLists.txt
  src/gui/kprocessrunner.cpp

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

D28590: Add a QString Solid::Device::label, used in Fstab Device for network mounts

[Méven Car]

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

REVISION SUMMARY
  This adds a label field, so that devices can provide a label and a more 
detailled description.
  This label should be used as the device label and description used for 
subText or tooltips for instance.
  The description is used as label by default.
  
  CCBUG: 415281
  
  For use in D26113  and D26114 


REPOSITORY
  R245 Solid

BRANCH
  master

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

AFFECTED FILES
  src/solid/devices/backends/fakehw/fakedevice.cpp
  src/solid/devices/backends/fakehw/fakedevice.h
  src/solid/devices/backends/fstab/fstabdevice.cpp
  src/solid/devices/backends/fstab/fstabdevice.h
  src/solid/devices/backends/fstab/fstabmanager.cpp
  src/solid/devices/backends/shared/rootdevice.cpp
  src/solid/devices/backends/shared/rootdevice.h
  src/solid/devices/backends/udev/udevdevice.cpp
  src/solid/devices/backends/udev/udevdevice.h
  src/solid/devices/backends/udev/udevmanager.cpp
  src/solid/devices/backends/udisks2/udisksdevice.cpp
  src/solid/devices/backends/udisks2/udisksdevice.h
  src/solid/devices/backends/udisks2/udisksmanager.cpp
  src/solid/devices/backends/upower/upowerdevice.cpp
  src/solid/devices/backends/upower/upowerdevice.h
  src/solid/devices/backends/upower/upowermanager.cpp
  src/solid/devices/frontend/device.cpp
  src/solid/devices/frontend/device.h
  src/solid/devices/ifaces/device.h

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

D28561: Remove border from format-border-set-* icons

[David Hurka]

This revision was automatically updated to reflect the committed changes.
Closed by commit R266:07487651e13c: Remove border from format-border-set-* 
icons (authored by davidhurka).

REPOSITORY
  R266 Breeze Icons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28561?vs=79386=79393

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

AFFECTED FILES
  icons-dark/actions/16/format-border-set-bottom.svg
  icons-dark/actions/16/format-border-set-diagonal-bl-tr.svg
  icons-dark/actions/16/format-border-set-diagonal-tl-br.svg
  icons-dark/actions/16/format-border-set-external.svg
  icons-dark/actions/16/format-border-set-internal-horizontal.svg
  icons-dark/actions/16/format-border-set-internal-vertical.svg
  icons-dark/actions/16/format-border-set-internal.svg
  icons-dark/actions/16/format-border-set-left.svg
  icons-dark/actions/16/format-border-set-none.svg
  icons-dark/actions/16/format-border-set-right.svg
  icons-dark/actions/16/format-border-set-top.svg
  icons-dark/actions/22/format-border-set-bottom.svg
  icons-dark/actions/22/format-border-set-diagonal-bl-tr.svg
  icons-dark/actions/22/format-border-set-diagonal-tl-br.svg
  icons-dark/actions/22/format-border-set-external.svg
  icons-dark/actions/22/format-border-set-internal-horizontal.svg
  icons-dark/actions/22/format-border-set-internal-vertical.svg
  icons-dark/actions/22/format-border-set-internal.svg
  icons-dark/actions/22/format-border-set-left.svg
  icons-dark/actions/22/format-border-set-none.svg
  icons-dark/actions/22/format-border-set-right.svg
  icons-dark/actions/22/format-border-set-top.svg
  icons/actions/16/format-border-set-bottom.svg
  icons/actions/16/format-border-set-diagonal-bl-tr.svg
  icons/actions/16/format-border-set-diagonal-tl-br.svg
  icons/actions/16/format-border-set-external.svg
  icons/actions/16/format-border-set-internal-horizontal.svg
  icons/actions/16/format-border-set-internal-vertical.svg
  icons/actions/16/format-border-set-internal.svg
  icons/actions/16/format-border-set-left.svg
  icons/actions/16/format-border-set-none.svg
  icons/actions/16/format-border-set-right.svg
  icons/actions/16/format-border-set-top.svg
  icons/actions/22/format-border-set-bottom.svg
  icons/actions/22/format-border-set-diagonal-bl-tr.svg
  icons/actions/22/format-border-set-diagonal-tl-br.svg
  icons/actions/22/format-border-set-external.svg
  icons/actions/22/format-border-set-internal-horizontal.svg
  icons/actions/22/format-border-set-internal-vertical.svg
  icons/actions/22/format-border-set-internal.svg
  icons/actions/22/format-border-set-left.svg
  icons/actions/22/format-border-set-none.svg
  icons/actions/22/format-border-set-right.svg
  icons/actions/22/format-border-set-top.svg

To: davidhurka, #vdg, ndavis
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D26113: Places: For mounted volume display mount points instead of description

[Méven Car]

meven added a comment.


  In D26113#642028 , @bruns wrote:
  
  > I say its the responsibility of Vaults to set the option. For Vaults, there 
is no fstab anyway, just an options value in the mtab at runtime.
  
  
  My primary concern is for network shares.
  The vault issue is a distinct bug than the one I am meaning to fix here.
  
  > If you want to display more info in the tooltip, probably start with 
`StorageAccess.filePath`.
  
  The point of this diff is to use the filePath by default as **label**, which 
is way more explicit than the description "/export on 192.168.0.10" for network 
shares for instance, while not breaking the x-gvfs-name case (which the state 
of this patch currently does not respect)
  All you are saying is not contradicting my suggestion in 
https://phabricator.kde.org/D26113#641998
  So I feel this conversation is sterile, and I should go ahead with it in 
solid and loop back here once it is done.

REPOSITORY
  R241 KIO

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

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

D26113: Places: For mounted volume display mount points instead of description

[Stefan Brüns]

bruns added a comment.


  In D26113#642022 , @meven wrote:
  
  > In D26113#642018 , @bruns wrote:
  >
  > > In D26113#641998 , @meven 
wrote:
  > >
  > > > In D26113#641190 , @bruns 
wrote:
  > > >
  > > > > In D26113#581228 , 
@ngraham wrote:
  > > > >
  > > > > > > As Vaults are not backed by an fstab entry, it would be the 
responsibility of Vaults to set a useful name (via x-gvfs-name).
  > > > > >
  > > > > > What I'm saying is that I think it makes sense for Solid to provide 
a better default display string so each app doesn't have to do this.
  > > > >
  > > > >
  > > > > What I'm saying is that Vaults should set a meaningful value via 
x-gvfs-name so Solid has not to invent something. Then Solid can use that, and 
it would be consistent everywhere.
  > > >
  > > >
  > > > I see, so I think we should add a "displayName" (or label() similarly 
to StorageVolume) function to Solid::Device, so that x-gvfs-name is used for 
this or the mount point when not defined and keeping description as is : a 
verbose description of the Device that could be used in tooltips for instance.
  > > >  This displayName would default on description() so that it would be an 
opt-in change by Device implementation and then users can choose between 
description() and displayName() depending on the context and role they need. So 
here in kfileplacesitem we would replace description() by displayName().
  > > >  Does this idea make sense to you @bruns ?
  > >
  > >
  > > Its already there:
  > >
  > >   grep x-gvfs-name  /etc/fstab 
  > >   //pebbles/share /mnt cifs 
defaults,x-gvfs-show,x-gvfs-icon=yast-samba-server,x-gvfs-name=SambaShare
  > >
  > >
  > >   solid-hardware5  details /org/kde/fstab///pebbles/share
  > >   udi = '/org/kde/fstab///pebbles/share'
  > > parent = '/org/kde/fstab'  (string)
  > > vendor = 'pebbles'  (string)
  > > product = 'share'  (string)
  > > description = 'SambaShare'  (string)
  > > icon = 'yast-samba-server'  (string)
  > > StorageAccess.accessible = false  (bool)
  > > StorageAccess.filePath = '/mnt'  (string)
  > > StorageAccess.ignored = false  (bool)
  > > NetworkShare.type = 'Cifs'  (0x2)  (enum)
  > > NetworkShare.url = 'smb://pebbles/share'  (string)
  > >
  > >
  > > F8216997: Screenshot_20200405_124325.png 

  >
  >
  > A lot of users won't have a x-gvfs-name for whatever reason and we can't 
expect them to be take savy with root access to set up a x-gvfs-name mount 
option.
  >  I see your point but my last one was how to make the display by default 
better (not using description()), but keeping displaying x-gvfs-name value when 
defined by the user.
  
  
  I say its the responsibility of Vaults to set the option. For Vaults, there 
is no fstab anyway, just an options value in the mtab at runtime.
  
  If you want to display more info in the tooltip, probably start with 
`StorageAccess.filePath`.

REPOSITORY
  R241 KIO

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

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

D28561: Remove border from format-border-set-* icons

[Noah Davis]

ndavis accepted this revision.
ndavis added a comment.


  LGTM

REPOSITORY
  R266 Breeze Icons

BRANCH
  simplify-format-border

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

To: davidhurka, #vdg, ndavis
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D26113: Places: For mounted volume display mount points instead of description

[Méven Car]

meven added a comment.


  In D26113#642018 , @bruns wrote:
  
  > In D26113#641998 , @meven wrote:
  >
  > > In D26113#641190 , @bruns 
wrote:
  > >
  > > > In D26113#581228 , @ngraham 
wrote:
  > > >
  > > > > > As Vaults are not backed by an fstab entry, it would be the 
responsibility of Vaults to set a useful name (via x-gvfs-name).
  > > > >
  > > > > What I'm saying is that I think it makes sense for Solid to provide a 
better default display string so each app doesn't have to do this.
  > > >
  > > >
  > > > What I'm saying is that Vaults should set a meaningful value via 
x-gvfs-name so Solid has not to invent something. Then Solid can use that, and 
it would be consistent everywhere.
  > >
  > >
  > > I see, so I think we should add a "displayName" (or label() similarly to 
StorageVolume) function to Solid::Device, so that x-gvfs-name is used for this 
or the mount point when not defined and keeping description as is : a verbose 
description of the Device that could be used in tooltips for instance.
  > >  This displayName would default on description() so that it would be an 
opt-in change by Device implementation and then users can choose between 
description() and displayName() depending on the context and role they need. So 
here in kfileplacesitem we would replace description() by displayName().
  > >  Does this idea make sense to you @bruns ?
  >
  >
  > Its already there:
  >
  >   grep x-gvfs-name  /etc/fstab 
  >   //pebbles/share /mnt cifs 
defaults,x-gvfs-show,x-gvfs-icon=yast-samba-server,x-gvfs-name=SambaShare
  >
  >
  >   solid-hardware5  details /org/kde/fstab///pebbles/share
  >   udi = '/org/kde/fstab///pebbles/share'
  > parent = '/org/kde/fstab'  (string)
  > vendor = 'pebbles'  (string)
  > product = 'share'  (string)
  > description = 'SambaShare'  (string)
  > icon = 'yast-samba-server'  (string)
  > StorageAccess.accessible = false  (bool)
  > StorageAccess.filePath = '/mnt'  (string)
  > StorageAccess.ignored = false  (bool)
  > NetworkShare.type = 'Cifs'  (0x2)  (enum)
  > NetworkShare.url = 'smb://pebbles/share'  (string)
  >
  >
  > F8216997: Screenshot_20200405_124325.png 

  
  
  A lot of users won't have a x-gvfs-name for whatever reason and we can't 
expect them to be take savy with root access to set up a x-gvfs-name mount 
option.
  I see your point but my last one was how to make the display by default 
better (not using description()), but keeping displaying x-gvfs-name value when 
defined by the user.
  
  So we need a more general solution, which is what I suggest.
  
  Moreover setting description to x-gvfs-name makes the original description 
content inaccessible to application despite it could be valuable, for instance 
for tooltip.
  So I think putting x-gvfs-name in description was abusing the API for it was 
lacking a proper displayName/label field to begin with.
  I hope this clarifies my suggestion in 
https://phabricator.kde.org/D26113#641998

REPOSITORY
  R241 KIO

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

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

D28565: [QueryParser] Replace single-use helper with std::none_of

[Stefan Brüns]

This revision was automatically updated to reflect the committed changes.
Closed by commit R293:8a872e502d22: [QueryParser] Replace single-use helper 
with std::none_of (authored by bruns).

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28565?vs=79308=79387

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

AFFECTED FILES
  src/engine/queryparser.cpp

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

D26113: Places: For mounted volume display mount points instead of description

[Stefan Brüns]

bruns added a comment.


  In D26113#641998 , @meven wrote:
  
  > In D26113#641190 , @bruns wrote:
  >
  > > In D26113#581228 , @ngraham 
wrote:
  > >
  > > > > As Vaults are not backed by an fstab entry, it would be the 
responsibility of Vaults to set a useful name (via x-gvfs-name).
  > > >
  > > > What I'm saying is that I think it makes sense for Solid to provide a 
better default display string so each app doesn't have to do this.
  > >
  > >
  > > What I'm saying is that Vaults should set a meaningful value via 
x-gvfs-name so Solid has not to invent something. Then Solid can use that, and 
it would be consistent everywhere.
  >
  >
  > I see, so I think we should add a "displayName" (or label() similarly to 
StorageVolume) function to Solid::Device, so that x-gvfs-name is used for this 
or the mount point when not defined and keeping description as is : a verbose 
description of the Device that could be used in tooltips for instance.
  >  This displayName would default on description() so that it would be an 
opt-in change by Device implementation and then users can choose between 
description() and displayName() depending on the context and role they need. So 
here in kfileplacesitem we would replace description() by displayName().
  >  Does this idea make sense to you @bruns ?
  
  
  Its already there:
  
grep x-gvfs-name  /etc/fstab 
//pebbles/share /mnt cifs 
defaults,x-gvfs-show,x-gvfs-icon=yast-samba-server,x-gvfs-name=SambaShare
  
solid-hardware5  details /org/kde/fstab///pebbles/share
udi = '/org/kde/fstab///pebbles/share'
  parent = '/org/kde/fstab'  (string)
  vendor = 'pebbles'  (string)
  product = 'share'  (string)
  description = 'SambaShare'  (string)
  icon = 'yast-samba-server'  (string)
  StorageAccess.accessible = false  (bool)
  StorageAccess.filePath = '/mnt'  (string)
  StorageAccess.ignored = false  (bool)
  NetworkShare.type = 'Cifs'  (0x2)  (enum)
  NetworkShare.url = 'smb://pebbles/share'  (string)
  
  F8216997: Screenshot_20200405_124325.png 


REPOSITORY
  R241 KIO

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

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

D28561: Remove border from format-border-set-* icons

[David Hurka]

davidhurka edited the summary of this revision.

REPOSITORY
  R266 Breeze Icons

BRANCH
  simplify-format-border

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

To: davidhurka, #vdg, ndavis
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28561: Remove border from format-border-set-* icons

[David Hurka]

davidhurka updated this revision to Diff 79386.
davidhurka added a comment.


  - Remove border elements in 22px versions of format-border-*
  
  I would like if you accept this new patch version, to be formally correct.

REPOSITORY
  R266 Breeze Icons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28561?vs=79303=79386

BRANCH
  simplify-format-border

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

AFFECTED FILES
  icons-dark/actions/16/format-border-set-bottom.svg
  icons-dark/actions/16/format-border-set-diagonal-bl-tr.svg
  icons-dark/actions/16/format-border-set-diagonal-tl-br.svg
  icons-dark/actions/16/format-border-set-external.svg
  icons-dark/actions/16/format-border-set-internal-horizontal.svg
  icons-dark/actions/16/format-border-set-internal-vertical.svg
  icons-dark/actions/16/format-border-set-internal.svg
  icons-dark/actions/16/format-border-set-left.svg
  icons-dark/actions/16/format-border-set-none.svg
  icons-dark/actions/16/format-border-set-right.svg
  icons-dark/actions/16/format-border-set-top.svg
  icons-dark/actions/22/format-border-set-bottom.svg
  icons-dark/actions/22/format-border-set-diagonal-bl-tr.svg
  icons-dark/actions/22/format-border-set-diagonal-tl-br.svg
  icons-dark/actions/22/format-border-set-external.svg
  icons-dark/actions/22/format-border-set-internal-horizontal.svg
  icons-dark/actions/22/format-border-set-internal-vertical.svg
  icons-dark/actions/22/format-border-set-internal.svg
  icons-dark/actions/22/format-border-set-left.svg
  icons-dark/actions/22/format-border-set-none.svg
  icons-dark/actions/22/format-border-set-right.svg
  icons-dark/actions/22/format-border-set-top.svg
  icons/actions/16/format-border-set-bottom.svg
  icons/actions/16/format-border-set-diagonal-bl-tr.svg
  icons/actions/16/format-border-set-diagonal-tl-br.svg
  icons/actions/16/format-border-set-external.svg
  icons/actions/16/format-border-set-internal-horizontal.svg
  icons/actions/16/format-border-set-internal-vertical.svg
  icons/actions/16/format-border-set-internal.svg
  icons/actions/16/format-border-set-left.svg
  icons/actions/16/format-border-set-none.svg
  icons/actions/16/format-border-set-right.svg
  icons/actions/16/format-border-set-top.svg
  icons/actions/22/format-border-set-bottom.svg
  icons/actions/22/format-border-set-diagonal-bl-tr.svg
  icons/actions/22/format-border-set-diagonal-tl-br.svg
  icons/actions/22/format-border-set-external.svg
  icons/actions/22/format-border-set-internal-horizontal.svg
  icons/actions/22/format-border-set-internal-vertical.svg
  icons/actions/22/format-border-set-internal.svg
  icons/actions/22/format-border-set-left.svg
  icons/actions/22/format-border-set-none.svg
  icons/actions/22/format-border-set-right.svg
  icons/actions/22/format-border-set-top.svg

To: davidhurka, #vdg, ndavis
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28161: [kio-extras] Port some QRegExp usage to QRegularExpression

[Méven Car]

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

REPOSITORY
  R320 KIO Extras

BRANCH
  l-QRE (branched from master)

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

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

D28161: [kio-extras] Port some QRegExp usage to QRegularExpression

[Méven Car]

meven added a comment.


  Seems good to me

REPOSITORY
  R320 KIO Extras

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

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

D26113: Places: For mounted volume display mount points instead of description

[Méven Car]

meven added a comment.


  In D26113#641190 , @bruns wrote:
  
  > In D26113#581228 , @ngraham 
wrote:
  >
  > > > As Vaults are not backed by an fstab entry, it would be the 
responsibility of Vaults to set a useful name (via x-gvfs-name).
  > >
  > > What I'm saying is that I think it makes sense for Solid to provide a 
better default display string so each app doesn't have to do this.
  >
  >
  > What I'm saying is that Vaults should set a meaningful value via 
x-gvfs-name so Solid has not to invent something. Then Solid can use that, and 
it would be consistent everywhere.
  
  
  I see, so I think we should add a "displayName" (or label() similarly to 
StorageVolume) function to Solid::Device, so that x-gvfs-name is used for this 
or the mount point when not defined and keeping description as is : a verbose 
description of the Device that could be used in tooltips for instance.
  This displayName would default on description() so that it would be an opt-in 
change by Device implementation and then users can choose between description() 
and displayName() depending on the context and role they need. So here in 
kfileplacesitem we would replace description() by displayName().
  Does this idea make sense to you @bruns ?

REPOSITORY
  R241 KIO

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

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

D28221: Don't write default value to configuration file when default value came from /etc/* file

[David Faure]

dfaure added a comment.


  I have a hard time accepting that the documentation was wrong -- and if it 
was, then this commit has to fix it, and port as much of the app code that does 
exactly this, as possible.
  
  I don't really know what mReference is. What about a test that uses KConfig 
directly (no skeletons), to test e.g. what kwindowconfig.cpp?
  Sorry to be a non-believer, I just sense a very strong risk of regression 
here, and if not, then a lot of porting effort. Unless there's a good reason 
for this stuff to be that way, that we still have to find out.
  
  I lack time and concentration right now to dig further, but I wanted to not 
delay my reply longer.

REPOSITORY
  R237 KConfig

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

To: bport, ervin, dfaure, davidedmundson
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28577: Add StatusBarExtension(KParts::Part *) overloaded constructor.

[David Faure]

dfaure closed this revision.

REPOSITORY
  R306 KParts

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

To: dfaure, vkrause, aacid, cgiboudeaux, kossebau
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28577: Add StatusBarExtension(KParts::Part *) overloaded constructor.

[Volker Krause]

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

REPOSITORY
  R306 KParts

BRANCH
  master

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

To: dfaure, vkrause, aacid, cgiboudeaux, kossebau
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28582: KRunner: Show error message for Actions in dbus runner

[Alexander Lohnau]

alex updated this revision to Diff 79377.
alex added a comment.


  Update comment

REPOSITORY
  R308 KRunner

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28582?vs=79376=79377

BRANCH
  dbusrunner_cleanup (branched from master)

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

AFFECTED FILES
  src/dbusrunner.cpp

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

D28582: KRunner: Show error message for Actions in dbus runner

[Alexander Lohnau]

alex updated this revision to Diff 79376.
alex added a comment.


  Update if condition

REPOSITORY
  R308 KRunner

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28582?vs=79375=79376

BRANCH
  dbusrunner_cleanup (branched from master)

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

AFFECTED FILES
  src/dbusrunner.cpp

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

D28582: KRunner: Show error message for Actions in dbus runner

[Alexander Lohnau]

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

REVISION SUMMARY
  KRunner now shows error messages for the Actions dbus request.
  But they get only shown if the service is not unknown, because the same error 
message will be shown in the match method.

TEST PLAN
  Make sure that the logging category is enabled and verify that you get debug 
output when starting KRunner.
  Create a runner for example 
https://cgit.kde.org/scratch/davidedmundson/pythonrunner.git/tree/runner.py and 
install it.
  Make sure that the Actions method is valid and returns actions, they should 
be displayed without warnings.
  Change the return value from a list to 123, a TypeError message should be 
displayed.
  Kill the runner, and type one letter in KRunner. The ServiceUnknown error 
message gets shown only once (from the match method).

REPOSITORY
  R308 KRunner

BRANCH
  dbusrunner_cleanup (branched from master)

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

AFFECTED FILES
  src/dbusrunner.cpp

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

D28573: Add 16px System Settings icon

[Manuel Jesús de la Fuente]

manueljlin added a comment.


  manueljlin @ pm.me
  I'll look into using arc for future patches, thanks :D

REPOSITORY
  R266 Breeze Icons

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

To: manueljlin, #vdg, ndavis, ngraham
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28478: [FileProtocol] change statx stat_dev() to return makedev(major, minor)

[Stefan Brüns]

bruns added a comment.


  looks good to me otherwise

INLINE COMMENTS

> jobtest.cpp:1537
> +
> +// this test doesn't make sense on the CI as it's an LXC container with 
> one partition
> +if (otherTmpDirIsOnSamePartition()) {

slight rewording

  // In case the two tmp dirs are on the same partition (e.g. on the CI), the
  // device IDs should  match, otherwise they should be different

and then

  } else {
 QCOMPARE(device, otherDevice);
  }

> ahmadsamir wrote in jobtest.cpp:1510
> OK, that makes sense. Done.

nitpick - the test name now again is wrong ;-)

REPOSITORY
  R241 KIO

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

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

D27216: [KProcessRunner] Improve error reported to user when exit code != 0

[David Faure]

dfaure added a comment.


  Oops I totally missed this one, sorry.
  
  Does this testcase work better with the new code in kiogui? Hopefully 
QProcess start should fail?

REPOSITORY
  R241 KIO

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

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

D28033: Create ExpandableListItem

[David Faure]

dfaure added a comment.


  This commit broke the i18ndcheck unittest.
  
  
https://build.kde.org/job/Frameworks/view/Platform%20-%20SUSEQt5.14/job/plasma-framework/job/kf5-qt5%20SUSEQt5.14/9/testReport/junit/projectroot/autotests/i18ndcheck/
  
  Reproducible locally. Works before and breaks after this commit.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: ngraham, #vdg, #plasma, davidedmundson
Cc: dfaure, mart, davidedmundson, bruns, niccolove, cblack, davidre, 
kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham