Plasma Framework and Kirigami unittests

[David Faure]

Since commit f09b46bec639a97008f3471b4b9bf52648979d12 by Marco Martin
titled "Draft: Replace QString cache IDs with a struct-based version"
the CI says plasma-framework unittests don't pass anymore.


FAIL!  : ThemeTest::loadSvgIcon() 'm_svg->theme()->findInCache(cacheId, result, 
info.lastModified().toSecsSinceEpoch())' returned FALSE. ()
   Loc: [/home/jenkins/workspace/Frameworks/plasma-framework/kf5-qt5 
SUSEQt5.14/autotests/themetest.cpp(81)]

See 
https://build.kde.org/job/Frameworks/view/Platform%20-%20SUSEQt5.14/job/plasma-framework/job/kf5-qt5%20SUSEQt5.14/40/testReport/projectroot/autotests/plasma_themetest/

Could someone from the plasma team please keep an eye on unittests so I don't 
have to be the unittest police?

Oh, and Kirigami has a broken unittest too:

FAIL!  : Kirigami::AvatarTests::test_bad_names() Uncaught exception: NameUtils 
is not defined
   Loc: [/home/jenkins/workspace/Frameworks/kirigami/kf5-qt5 
SUSEQt5.14/autotests/tst_avatar.qml(38)]

See 
https://build.kde.org/job/Frameworks/view/Platform%20-%20SUSEQt5.14/job/kirigami/job/kf5-qt5%20SUSEQt5.14/65/testReport/junit/projectroot.home.jenkins.workspace.Frameworks.kirigami.kf5-qt5_SUSEQt514/autotests/tst_avatar_qml/
I think that one is for Carson Black, CC'ed.

-- 
David Faure, fa...@kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5

KRunProxy

[David Faure]

In order to switch kdeclarative to 
-DKF_DISABLE_DEPRECATED_BEFORE_AND_AT=0x054700
we need to port KRunProxy away from KRun, or to deprecate KRunProxy.

AFAICS it exposes an object named KRun to QML files, but searching for
qml files with KRun in them leads to no results: 
https://lxr.kde.org/search?_filestring=*.qml&_string=KRun

Am I doing this wrong? Are there other file extensions to take into 
consideration?

If not, can I deprecate KRunProxy?

-- 
David Faure, fa...@kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5

D29687: [IconApplet] Port KRun to ApplicationLauncherJob

[David Faure]

dfaure added inline comments.

INLINE COMMENTS

> iconapplet.cpp:52
>  #include 
> +#include 
>  #include 

You're not using that class, are you?

REPOSITORY
  R120 Plasma Workspace

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

To: ahmadsamir, #plasma, broulik, dfaure
Cc: dfaure, ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29687: [IconApplet] Port KRun to ApplicationLauncherJob

[David Faure]

dfaure accepted this revision.
dfaure added a comment.


  KServiceFactory::findServiceByStorageId falls back to "new KService" if it 
can't find the storageId in ksycoca. So yes it worked, but it's kind of 
pointless (and confusing) to call it just to go to the new KService fallback, 
better do that directly.

REPOSITORY
  R120 Plasma Workspace

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

To: ahmadsamir, #plasma, broulik, dfaure
Cc: dfaure, ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27203: Don't try to open files we can't figure out where they are

[David Faure]

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

INLINE COMMENTS

> kdeplatformfiledialoghelper.cpp:226
> +//passing non-local files as the working directory is not supported.
> +//See QFileDialogPrivate::initialSelections
> +//Selectingg files should be done through the correct method.

That's not what that method says. It just says it can't know if the initial 
*selection* is a file or directory.

That's unrelated to what is the start directory.

QFileDialogPrivate::workingDirectory does support remote dirs.

REPOSITORY
  R135 Integration for Qt applications in Plasma

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

To: apol, #frameworks, #plasma, dfaure, meven, ahmadsamir
Cc: ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29125: lookandfeel kcm: port from KRun::runApplication to KIO::ApplicationLauncherJob

[David Faure]

dfaure closed this revision.

REPOSITORY
  R119 Plasma Desktop

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

To: dfaure, broulik, ngraham, crossi, mart, davidedmundson
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29125: lookandfeel kcm: port from KRun::runApplication to KIO::ApplicationLauncherJob

[David Faure]

dfaure added a comment.


  ping?

REPOSITORY
  R119 Plasma Desktop

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

To: dfaure, broulik, ngraham, crossi, mart
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29601: Port KRun::runApplication to ApplicationLauncherJob

[David Faure]

dfaure closed this revision.

REPOSITORY
  R856 Plasma Browser Integration

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

To: dfaure, broulik
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29601: Port KRun::runApplication to ApplicationLauncherJob

[David Faure]

dfaure created this revision.
dfaure added a reviewer: broulik.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
dfaure requested review of this revision.

TEST PLAN
  Builds

REPOSITORY
  R856 Plasma Browser Integration

BRANCH
  master

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

AFFECTED FILES
  CMakeLists.txt
  reminder/browserintegrationreminder.cpp

To: dfaure, broulik
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28321: [WIP] [applets/devicenotifier] Port to ExpandableListItem

[David Faure]

dfaure added inline comments.

INLINE COMMENTS

> broulik wrote in DeviceItem.qml:34
> This is all over the place for me. All the free space jobs in the dataengine 
> get the root size back. Not sure if a local issue or KIO? @dfaure

Please give me a proper bug report in KIO terms :-)
What class do you use, what do you get as a result?

I don't see a bug in a TODO comment in some QML code ;)

REPOSITORY
  R120 Plasma Workspace

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

To: ngraham, #vdg, #plasma, broulik
Cc: dfaure, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29226: [ksmserver] Use CommandLauncherJob for restoring applications

[David Faure]

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

INLINE COMMENTS

> server.cpp:134
> +for ( int i=1; i < n; i++)
> +   argList.append( command[i]);
> +auto *job = new KIO::CommandLauncherJob(app, argList);

(pre-existing) argList = command; argList.removeFirst(); would be simpler and 
faster.

> server.cpp:137
> +job->start();
> +return;
>  }

This return statement serves no purpose.

REPOSITORY
  R120 Plasma Workspace

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

To: broulik, #plasma, dfaure
Cc: dfaure, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28988: Port hotplug from KRun::runCommand to CommandLauncherJob.

[David Faure]

dfaure closed this revision.

REPOSITORY
  R120 Plasma Workspace

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

To: dfaure, davidedmundson, sitter, broulik
Cc: broulik, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29125: lookandfeel kcm: port from KRun::runApplication to KIO::ApplicationLauncherJob

[David Faure]

dfaure created this revision.
dfaure added reviewers: broulik, ngraham, crossi, mart.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
dfaure requested review of this revision.

REVISION SUMMARY
  I used the dialog ui delegate because this is a KCM, even though it
  seems to use QtQuick itself.

TEST PLAN
  Builds. `kcmshell5 lookandfeel` seems to add stuff to autostart
  only for some themes I don't have (commit 49ae81f280bd 
 
talks about lattedock
  or conky).

REPOSITORY
  R119 Plasma Desktop

BRANCH
  master

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

AFFECTED FILES
  kcms/lookandfeel/kcm.cpp

To: dfaure, broulik, ngraham, crossi, mart
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28988: Port hotplug from KRun::runCommand to CommandLauncherJob.

[David Faure]

dfaure updated this revision to Diff 80969.
dfaure added a comment.


  Use KNotificationJobUiDelegate

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28988?vs=80566=80969

BRANCH
  arcpatch-D28988

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

AFFECTED FILES
  CMakeLists.txt
  dataengines/hotplug/CMakeLists.txt
  dataengines/hotplug/deviceserviceaction.cpp

To: dfaure, davidedmundson, sitter
Cc: broulik, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28988: Port hotplug from KRun::runCommand to CommandLauncherJob.

[David Faure]

dfaure updated this revision to Diff 80970.
dfaure added a comment.


  Remove question from commit message

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28988?vs=80969=80970

BRANCH
  arcpatch-D28988

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

AFFECTED FILES
  CMakeLists.txt
  dataengines/hotplug/CMakeLists.txt
  dataengines/hotplug/deviceserviceaction.cpp

To: dfaure, davidedmundson, sitter
Cc: broulik, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29124: Port all KRun::runService/runApplication to KIO/ApplicationLauncherJob

[David Faure]

dfaure added inline comments.

INLINE COMMENTS

> broulik wrote in appentry.cpp:42
> Looks unused now?

No there's a KRun::run in this file. I only ported runService/runApplication in 
this commit.

> broulik wrote in recentusagemodel.cpp:44
> Unused?

There's a `new KRun` in this file.

REPOSITORY
  R120 Plasma Workspace

BRANCH
  master

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

To: dfaure, broulik, davidedmundson
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29124: Port all KRun::runService/runApplication to KIO/ApplicationLauncherJob

[David Faure]

dfaure closed this revision.

REPOSITORY
  R120 Plasma Workspace

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

To: dfaure, broulik, davidedmundson
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28990: Port quicklaunch from KRun::run to CommandLauncherJob

[David Faure]

dfaure closed this revision.

REPOSITORY
  R114 Plasma Addons

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

To: dfaure, drosca, davidedmundson, broulik
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29124: Port all KRun::runService/runApplication to KIO/ApplicationLauncherJob

[David Faure]

dfaure created this revision.
dfaure added reviewers: broulik, davidedmundson.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
dfaure requested review of this revision.

REVISION SUMMARY
  And use KNotificationJobUiDelegate in case of errors

TEST PLAN
  Untested other than compilation

REPOSITORY
  R120 Plasma Workspace

BRANCH
  master

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

AFFECTED FILES
  applets/icon/CMakeLists.txt
  applets/icon/iconapplet.cpp
  applets/kicker/plugin/actionlist.cpp
  applets/kicker/plugin/appentry.cpp
  applets/kicker/plugin/recentusagemodel.cpp
  klipper/urlgrabber.cpp

To: dfaure, broulik, davidedmundson
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28990: Port quicklaunch from KRun::run to CommandLauncherJob

[David Faure]

dfaure added a comment.


  OK, that's above my head. Shall I push this or you'll rework it all?
  
  I just want to be able to deprecated KRun::run and friends :)

REPOSITORY
  R114 Plasma Addons

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

To: dfaure, drosca, davidedmundson, broulik
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28988: Port hotplug from KRun::runCommand to CommandLauncherJob.

[David Faure]

dfaure added a comment.


  As it is currently implemented, CommandLauncherJob is very unlikely to ever 
fail. It doesn't even fail on non-existing executable in the command (because 
it delegates to bash) so apart from QProcess cannot fork, it never fails. 
Probably not worth doing something complicated here then. I'll just use 
KNotificationJobUiDelegate?

REPOSITORY
  R120 Plasma Workspace

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

To: dfaure, davidedmundson, sitter
Cc: broulik, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28990: Port quicklaunch from KRun::run to CommandLauncherJob

[David Faure]

dfaure updated this revision to Diff 80586.
dfaure added a comment.


  Use KNotificationJobUiDelegate

REPOSITORY
  R114 Plasma Addons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28990?vs=80568=80586

BRANCH
  master

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

AFFECTED FILES
  CMakeLists.txt
  applets/quicklaunch/plugin/CMakeLists.txt
  applets/quicklaunch/plugin/quicklaunch_p.cpp

To: dfaure, drosca, davidedmundson, broulik
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28990: Port quicklaunch from KRun::run to CommandLauncherJob

[David Faure]

dfaure added a comment.


  Agreed about notification delegate, will do.
  
  Does this really use KServiceAction anywhere? I don't see that. Confusing 
this one and D28988 ? [which however can't 
use the KServiceAction because of special support for %D and stuff]

REPOSITORY
  R114 Plasma Addons

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

To: dfaure, drosca, davidedmundson, broulik
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28990: Port quicklaunch from KRun::run to CommandLauncherJob

[David Faure]

dfaure created this revision.
dfaure added reviewers: drosca, davidedmundson, broulik.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
dfaure requested review of this revision.

TEST PLAN
  Builds, but untested.

REPOSITORY
  R114 Plasma Addons

BRANCH
  master

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

AFFECTED FILES
  CMakeLists.txt
  applets/quicklaunch/plugin/quicklaunch_p.cpp

To: dfaure, drosca, davidedmundson, broulik
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28988: Port hotplug from KRun::runCommand to CommandLauncherJob.

[David Faure]

dfaure created this revision.
dfaure added reviewers: davidedmundson, sitter.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
dfaure requested review of this revision.

REVISION SUMMARY
  Is there a better way to report error from this code than the dialog box
  that KRun used to pop up (and KDialogJobUiDelegate does too)?

TEST PLAN
  Untested.

REPOSITORY
  R120 Plasma Workspace

BRANCH
  master

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

AFFECTED FILES
  CMakeLists.txt
  dataengines/hotplug/deviceserviceaction.cpp

To: dfaure, davidedmundson, sitter
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28649: [Notifications] Port to upstream QConcatenateTablesProxyModel

[David Faure]

dfaure added a comment.


  Wait, even if the jobs model is empty (no rows), what's its columnCount()?
  If that's fixed, then it should all be fine. Maybe my analysis (and testcase) 
is incorrect.
  
  Can you check the columnCount() of the Concat model?
  
  I assume the notifications model emits dataChanged correctly when the 
ExpiredRole is set to true?

REPOSITORY
  R120 Plasma Workspace

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

To: broulik, #plasma
Cc: davidre, dfaure, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28649: [Notifications] Port to upstream QConcatenateTablesProxyModel

[David Faure]

dfaure added a comment.


  OK I see why this is happening.
  
  KConcatenateProxyModel's columnCount is the number of columns of the first 
source model. Which leads to strange things if other models have less columns.
  QConcatenateProxyModel takes the smallest number of columns of all source 
models, to solve that.
  But here the jobs model is empty. And empty model shouldn't be taken into 
account when calculating the number of columns.
  I'll fix that in QConcatenateProxyModel, but that means you can't use it for 
another 2 years or so... oh well ;)

REPOSITORY
  R120 Plasma Workspace

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

To: broulik, #plasma
Cc: davidre, dfaure, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

Re: Fwd: KDE CI: Administration » Dependency Build Plasma stable-kf5-qt5 FreeBSDQt5.14 - Build # 7 - Still Failing!

[David Faure]

On Saturday, April 18, 2020 5:06:43 PM CEST Friedrich W. H. Kossebau wrote:
> Am Samstag, 18. April 2020, 16:26:57 CEST schrieb David Faure:
> > On Saturday, April 18, 2020 7:09:25 AM CEST Ben Cooksley wrote:
> > > Hi all,
> > > 
> > > Please see below - any ideas as to why KHelpCenter no longer
> > > successfully
> > > builds?
> > > 
> > > It doesn't look like KHelpCenter has changed...
> > 
> > No, Qt has.
> 
> My question though is why. This would be a ABI-breaking change, no?
> And khelpcenter nowhere disables deprecated Qt API, from what I can tell.
> 
> Does something smuggle in e.g. via CMake config file compile flags a
> QT_DISABLE_DEPRECATED_BEFORE=0x050e00 perhaps? Or is something flawed with
> the Qt packages on FreeBSD side?

Good question, this is supposed to be just a warning.
No idea :(

-- 
David Faure, fa...@kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5

Re: Fwd: KDE CI: Administration » Dependency Build Plasma stable-kf5-qt5 FreeBSDQt5.14 - Build # 7 - Still Failing!

[David Faure]

On Saturday, April 18, 2020 7:09:25 AM CEST Ben Cooksley wrote:
> Hi all,
> 
> Please see below - any ideas as to why KHelpCenter no longer successfully
> builds?
> 
> It doesn't look like KHelpCenter has changed...

No, Qt has.

This was fixed today by Johnny Jazeix in commit 220a959bf150966f807ed.

Cheers,
David.
-- 
David Faure, fa...@kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5

D28383: Add PageRouter component

[David Faure]

dfaure added a comment.


  The unittest in this commit appears to break in CI.
  
  
https://build.kde.org/job/Frameworks/view/Platform%20-%20SUSEQt5.12/job/kirigami/job/kf5-qt5%20SUSEQt5.12/417/testReport/junit/projectroot.home.jenkins.workspace.Frameworks.kirigami.kf5-qt5_SUSEQt512/autotests/tst_pagerouter_qml/
  
PASS   : Kirigami::PageRouterGeneralTests::test_50_push()
PASS   : Kirigami::PageRouterGeneralTests::test_60_pop()
QWARN  : Kirigami::PageRouterGeneralTests::test_70_bring_to_view() Route 
"login" with data QVariant(QString, "red") is not on the current stack of 
routes.
FAIL!  : Kirigami::PageRouterGeneralTests::test_70_bring_to_view() Compared 
values are not the same
   Actual   (): 0
   Expected (): 1
   Loc: [/home/jenkins/workspace/Frameworks/kirigami/kf5-qt5 
SUSEQt5.12/autotests/tst_pagerouter.qml(38)]
FAIL!  : Kirigami::PageRouterGeneralTests::test_80_routeactive() Compared 
values are not the same
   Actual   (): false
   Expected (): true
   Loc: [/home/jenkins/workspace/Frameworks/kirigami/kf5-qt5 
SUSEQt5.12/autotests/tst_pagerouter.qml(45)]
PASS   : Kirigami::PageRouterGeneralTests::test_90_initial_route()
  
  Please investigate and fix.
  Thanks!

REPOSITORY
  R169 Kirigami

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

To: cblack, #kirigami, mart, davidedmundson
Cc: dfaure, ahiemstra, davidedmundson, plasma-devel, fbampaloukas, GB_2, 
domson, dkardarakos, ngraham, apol, mart

D28134: Add ColorUtils

[David Faure]

dfaure added a comment.


  Breaks compilation with Qt 5.13 too.
  
/data/kde/src/5/frameworks/kirigami/src/kirigamiplugin.cpp:248:122: error: 
invalid user-defined conversion from ‘KirigamiPlugin::registerTypes(const 
char*)::’ to ‘QObject* (*)(QQmlEngine*, 
QJSEngine*)’ [-fpermissive]
  248 | qmlRegisterSingletonType(uri, 2, 12, "ColorUtils", 
[](QQmlEngine*, QJSEngine*) { return new ColorUtils; });
  | 
 ^
  
  This overload is new in Qt 5.14.
  Please use something that works with 5.12, and/or use a QT_VERSION #if.

REPOSITORY
  R169 Kirigami

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

To: cblack, #plasma, mart, davidedmundson
Cc: dfaure, kossebau, fvogt, davidedmundson, plasma-devel, fbampaloukas, GB_2, 
domson, dkardarakos, ngraham, apol, ahiemstra, mart

D28286: [Task Manager] Port backend to ApplicationLauncherJob

[David Faure]

dfaure added a comment.


  That wouldn't work either, you need to be able to choose between a 
Notification delegate, a Dialog delegate (which lives in a different library 
due to the QtWidgets dependency), and some more.

REPOSITORY
  R119 Plasma Desktop

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

To: broulik, #plasma, hein, dfaure
Cc: anthonyfieroni, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28286: [Task Manager] Port backend to ApplicationLauncherJob

[David Faure]

dfaure added a comment.


  That would save only one line (the call to setUiDelegate).
  
  I prefer my earlier suggestion: job->setUiDelegate(new 
KNotificationJobUiDelegate(KJobUiDelegate::AutoErrorHandlingEnabled));
  That one alone would save 2 lines ;)

REPOSITORY
  R119 Plasma Desktop

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

To: broulik, #plasma, hein, dfaure
Cc: anthonyfieroni, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28512: Port Konsole and Kate runners and dataengine away from KToolInvocation

[David Faure]

dfaure added a comment.


  > I kinda feel CommandLauncherJob should have a QString executable, 
QStringList args constructor so I don't have to escape things myself?
  
  Sounds good, excellent idea. This would simplify the code in many callers. 
Please add :)
  
  It still needs to support the "QString command" use case though, since that's 
sometimes what we have as input. Well, at least for KRun::runCommand, but we 
can keep that public for other use cases I guess (exec lines extracted from 
desktop/config files, inputted by users etc.).

REPOSITORY
  R114 Plasma Addons

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

To: broulik, #plasma, dfaure
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28286: [Task Manager] Port backend to ApplicationLauncherJob

[David Faure]

dfaure added a comment.


  In D28286#637346 , @anthonyfieroni 
wrote:
  
  > Even 1200 is not problem to me.
  
  
  We seem to have very different opinions on good API design.

REPOSITORY
  R119 Plasma Desktop

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

To: broulik, #plasma, hein, dfaure
Cc: anthonyfieroni, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28286: [Task Manager] Port backend to ApplicationLauncherJob

[David Faure]

dfaure added a comment.


  Why not -> because it just doesn't scale. 30 jobs * 4 delegates = 120 wrapper 
methods...

REPOSITORY
  R119 Plasma Desktop

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

To: broulik, #plasma, hein, dfaure
Cc: anthonyfieroni, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28286: [Task Manager] Port backend to ApplicationLauncherJob

[David Faure]

dfaure added a comment.


  I don't think we want to have wrapper functions in KIO for all combinations 
of jobs and delegates. There are many of each.

REPOSITORY
  R119 Plasma Desktop

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

To: broulik, #plasma, hein, dfaure
Cc: anthonyfieroni, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28286: [Task Manager] Port backend to ApplicationLauncherJob

[David Faure]

dfaure added a comment.


  Alternatively I'm wondering if we should add constructors to the delegates 
that take some AutoErrorHandlingEnabled enum value, then it can become 
something like
  
auto *job = new KIO::ApplicationLauncherJob(service);
job->setUiDelegate(new 
KNotificationJobUiDelegate(KJob::AutoErrorHandlingEnabled));
  
  (and the same with KDialogJobUiDelegate in widget applications)
  
  Benefit: available everywhere, unlike a local wrapper function.

REPOSITORY
  R119 Plasma Desktop

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

To: broulik, #plasma, hein, dfaure
Cc: anthonyfieroni, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28286: [Task Manager] Port backend to ApplicationLauncherJob

[David Faure]

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


  Looks good to me, but please wait until D28367 
 and D28268 
 land, since they change the API of 
ApplicationLauncherJob for KServiceAction.
  
  Many thanks for helping with this!

REPOSITORY
  R119 Plasma Desktop

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

To: broulik, #plasma, hein, dfaure
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28347: Port services and shell runner away from KRun

[David Faure]

dfaure added a comment.


  Looks good to me, but please wait until D28367 
 and D28268 
 land, since they change the API of 
ApplicationLauncherJob for KServiceAction.
  
  I do not think KActivities::ResourceInstance::notifyAccessed should be done 
from KIO, which doesn't depend on KActivities (and I would like to keep it that 
way).

REPOSITORY
  R120 Plasma Workspace

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

To: broulik, #plasma, dfaure
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28286: WIP: [Task Manager] Port backend to ApplicationLauncherJob

[David Faure]

dfaure added inline comments.

INLINE COMMENTS

> backend.cpp:178
> +auto *job = new KIO::ApplicationLauncherJob(service, 
> serviceAction);
> +// TODO uidelegate
> +job->start();

Only if you like messageboxes.

Plasma code might prefer something else? A notification maybe? I think we both 
know a guy who likes them very much ;)

REPOSITORY
  R119 Plasma Desktop

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

To: broulik, #plasma, hein, dfaure
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28079: [keditfiletype] Prevent removing the "main" glob pattern for mime types

[David Faure]

dfaure added a comment.


  I wrote QMimeDatabase, and my only intent was to move the main glob to be 
first. Not to prevent people from removing it. So this is a Qt bug.

REPOSITORY
  R126 KDE CLI Utilities

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

To: ahmadsamir, #plasma, dfaure, davidedmundson, apol
Cc: kde-frameworks-devel, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

Re: 2 kirigami fixes for a point release

[David Faure]

On vendredi 20 mars 2020 12:16:09 CET David Edmundson wrote:
> >> > Kirigami seems to be rather unstable, I wonder if anything can be done
> >> > to
> >> > improve upon that [*].
> >> 
> >> One important thing seems to have been getting sloppy in those repos;
> >> mandatory code reviews.
> >> That's an easy thing to enforce, and we know it makes a huge difference
> >> to
> >> code.
> >> 
> >> Even if no-one comments, the extra delay of it running on your own
> >> system delivers a lot.
> >
> >I fully agree.
> >
> >(CC'ing the developer lists)
> 
> The situation with code reviews has not changed.
> 
>  git log --grep Differential --invert-grep  --since 1.month
> 
> In Kirigami is extremely disappointing, and not something we see in
> other frameworks.

Thanks for noticing (and for the nice git command line!).

I'm going to ask more directly than the other David dares doing...

@Marco, can you please start using phabricator for all your commits, so that 
they get reviewed?

-- 
David Faure, fa...@kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5

D27954: appstreamrunner: Port to KApplicationTrader

[David Faure]

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

INLINE COMMENTS

> appstreamrunner.cpp:102
> +return true;
> +} else {
> +const auto renamedFrom = 
> service->property("X-Flatpak-RenamedFrom").toStringList();

For symmetry I would have removed the "else" altogether, there are many other 
cases of if()+return above already, no need to do things differently here.

REPOSITORY
  R120 Plasma Workspace

BRANCH
  master

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

To: apol, #plasma, dfaure
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27954: appstreamrunner: Port to KApplicationTrader

[David Faure]

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


  Cool to see KApplicationTrader being used :-)

INLINE COMMENTS

> appstreamrunner.cpp:96
> +
> +if (service->desktopEntryName() == componentId)
> +return true;

Note that the original trader query used =~ which meant "case insensitive 
comparison".
I have no idea if it was necessary though.

> appstreamrunner.cpp:102
> +return true;
> +} else if 
> (service->propertyNames().contains("X-Flatpak-RenamedFrom")) {
> +const auto renamedFrom = 
> service->property("X-Flatpak-RenamedFrom").toStringList();

This test seems slow and unnecessary.
IMHO you can remove it, and just query for that property, and if it's empty it 
won't contain anything so you'll move ahead to the final return false.

REPOSITORY
  R120 Plasma Workspace

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

To: apol, #plasma, dfaure
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

Re: 2 kirigami fixes for a point release

[David Faure]

On dimanche 16 février 2020 22:17:17 CET Albert Astals Cid wrote:
> This is their fault, they as a distribution have decided to support a random
> KDE Frameworks version for longer than we do support it, so they are the
> ones that should be the job of supporting it.
> 
> It's like you are trying to say we should be doing the distributions jobs,
> what we should be doing is doing our job which is making the best software
> we can, not spending time accomodating decisions that somebody else took
> for us, and since distributions often only bring us pain in the shape of
> not updating versions, etc. IMHO what we should be doing is moving away
> from distributions model and more into the snap/flatpak model in which we
> control what we give our users.
> 
> Sadly flatpak doesn't work for non applications and snap is
> Ubuntu-only-not-really-but-yes-really so for Plasma this doesn't really
> solver the problem so maybe we should just finally tell our users to start
> using the good distributions if they care about their user experience. By
> good meaning those with a rolling KDE software suite or those that actually
> do backport fixes to the version they have randomly decided to lock onto.

At the same time, we can only successfully convince distributions to upgrade 
to the monthly KF5 releases if they are stable and don't come with 
regressions. I believe this is true for most of the frameworks, but I'm not so 
sure about kirigami/qqc2-desktop-style, based on what I hear (not just the 
recent issue).

Before blaming distros, I believe we have our own backyard to take care of,
with for sure more systematic use of code reviews and possibly more automated 
testing, for those frameworks (for the latter I guess that QtQuick doesn't 
make it easy, but that's part of the problem...).

-- 
David Faure, fa...@kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5

Re: 2 kirigami fixes for a point release

[David Faure]

On jeudi 13 février 2020 09:46:20 CET David Edmundson wrote:
> > Kirigami seems to be rather unstable, I wonder if anything can be done to
> > improve upon that [*].
> 
> One important thing seems to have been getting sloppy in those repos;
> mandatory code reviews.
> That's an easy thing to enforce, and we know it makes a huge difference to
> code.
> 
> Even if no-one comments, the extra delay of it running on your own
> system delivers a lot.

I fully agree.

(CC'ing the developer lists)

-- 
David Faure, fa...@kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5

D24238: Port the pager applet away from QtWidgets

[David Faure]

dfaure added a comment.


  Ah, right. Thanks.

REPOSITORY
  R119 Plasma Desktop

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

To: davidedmundson, hein, apol, dfaure
Cc: ognarb, broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D24238: Port the pager applet away from QtWidgets

[David Faure]

dfaure added a comment.


  Fix pushed 
https://commits.kde.org/plasma-desktop/2b5e86323f180f0c51ef9af898a69a522bc379ad 
after the bug reporter confirmed it fixes the bug.
  
  Reopening this after it was committed is a bit weird. I can't close it, 
please do.

REPOSITORY
  R119 Plasma Desktop

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

To: dfaure, hein, apol
Cc: ognarb, broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D24238: Port the pager applet away from QtWidgets

[David Faure]

dfaure added a comment.


  Further research leads to this possible patch 
http://www.davidfaure.fr/2019/windowmodel.cpp.diff

REPOSITORY
  R119 Plasma Desktop

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

To: dfaure, hein, apol
Cc: ognarb, broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D24238: Port the pager applet away from QtWidgets

[David Faure]

dfaure added a comment.


  Oh I see.
  
  Is there a non-deprecated method to get the overall geometry of all screens 
combined, or do we need to iterate over the list of screens?
  Surprising that this doesn't seem to be anywhere in the API  !?

REPOSITORY
  R119 Plasma Desktop

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

To: dfaure, hein, apol
Cc: ognarb, broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D24238: Port the pager applet away from QtWidgets

[David Faure]

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 R119:674dd5f7e090: Port the pager applet away from QtWidgets 
(authored by dfaure).

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24238?vs=66877=70308

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

AFFECTED FILES
  applets/pager/plugin/pagermodel.cpp
  applets/pager/plugin/windowmodel.cpp

To: dfaure, hein, apol
Cc: broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25488: kwayland-integration: fix compilation with no-deprecated build of kwindowsystem

[David Faure]

dfaure added a comment.


  Thanks for the idea, done in 
https://commits.kde.org/kwayland-integration/3ce73d56ca5364242be3eb7cb1b5c4f5eaf47196

REPOSITORY
  R130 Frameworks integration plugin using KWayland

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

To: dfaure, kossebau, jriddell, davidedmundson
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25488: kwayland-integration: fix compilation with no-deprecated build of kwindowsystem

[David Faure]

dfaure closed this revision.

REPOSITORY
  R130 Frameworks integration plugin using KWayland

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

To: dfaure, kossebau, jriddell, davidedmundson
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25488: kwayland-integration: fix compilation with no-deprecated build of kwindowsystem

[David Faure]

dfaure created this revision.
dfaure added reviewers: kossebau, jriddell, davidedmundson.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
dfaure requested review of this revision.

REVISION SUMMARY
  I build kwindowsystem with -DEXCLUDE_DEPRECATED_BEFORE_AND_AT=5.62.0,
  and this broke here. Use the exact same condition as the one around
  those virtual methods in the base class.
  
  This required upgrading the KF5 requirement from 5.62 to 5.64,
  so no backport to Plasma/5.17 unfortunately.

REPOSITORY
  R130 Frameworks integration plugin using KWayland

BRANCH
  master

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

AFFECTED FILES
  CMakeLists.txt
  src/windowsystem/windowsystem.cpp
  src/windowsystem/windowsystem.h

To: dfaure, kossebau, jriddell, davidedmundson
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D24996: Create directory when saving the menu file

[David Faure]

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


  Thanks for the fix!

REPOSITORY
  R103 KMenu Editor

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

To: wbauer, dfaure
Cc: dfaure, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D13360: Touchpad KDED module: Convert to JSON metadata

[David Faure]

dfaure added a comment.


  I didn't fully follow this saga, but AFAICS this patch was reverted (commit 
3432c3342b1f801 
, 
bug 395622), so we still rely on desktop files to load the touchpad kded 
module. Can this patch be applied again now that commit f040cdb399b 
 
(X-KDE-PluginInfo-Name=touchpad) is in, or is something else missing?

REPOSITORY
  R119 Plasma Desktop

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

To: marten, #plasma, davidedmundson
Cc: dfaure, cfeck, kossebau, fvogt, romangg, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, 
alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, ahiemstra, mart

D24810: Ensure defintion of XDG_DATA_DIRS

[David Faure]

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


  Please make sure the first line of the commit log says CONFIG and not DATA, 
too (the phab title still says DATA).

INLINE COMMENTS

> startplasma.cpp:197
> +if (!qEnvironmentVariableIsSet("XDG_CONFIG_DIRS")) {
> +qputenv("XDG_CONFIG_DIRS", KDE_INSTALL_FULL_CONFDIR 
> ":/etc/xdg:/usr/local/etc/xdg");
> +}

Wait, where does /usr/local/etc/xdg come from? 
https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html 
says the default is only /etc/xdg.

REPOSITORY
  R120 Plasma Workspace

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

To: tcberner, #freebsd, dfaure, apol
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D24810: Ensure defintion of XDG_DATA_DIRS

[David Faure]

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


  Bug in the commit log: s/ also make XDG_DATA_DIRS/ also make XDG_CONFIG_DIRS/

INLINE COMMENTS

> startplasma.cpp:195
>  }
> +   // Additionally also set default value for XDG_CONFIG_DIRS which is not 
> set by default on FreeBSD.
> +   if (!qEnvironmentVariableIsSet("XDG_CONFIG_DIRS")) {

I see nothing FreeBSD specific about this, I suggest simplifying the comment.

REPOSITORY
  R120 Plasma Workspace

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

To: tcberner, #freebsd, dfaure, apol
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D24255: [startplasma] don't set QT_AUTO_SCREEN_SCALE_FACTOR with Qt >= 5.14

[David Faure]

dfaure closed this revision.

REPOSITORY
  R120 Plasma Workspace

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

To: dfaure, apol, davidedmundson
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D24255: [startplasma] don't set QT_AUTO_SCREEN_SCALE_FACTOR with Qt >= 5.14

[David Faure]

dfaure created this revision.
dfaure added a reviewer: apol.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
dfaure requested review of this revision.

REVISION SUMMARY
  It gives the following runtime warning:
  
Warning: QT_AUTO_SCREEN_SCALE_FACTOR is deprecated. Instead use:
   QT_ENABLE_HIGHDPI_SCALING to enable platform plugin controlled 
per-screen factors
  
  But we don't need to set QT_ENABLE_HIGHDPI_SCALING to 0.
  Just setting QT_SCREEN_SCALE_FACTORS is enough to control scaling, after
  my commit e018d11600bffc6 to qtbase.
  
  Kudos for porting startkde to be C++ code, it makes this commit much
  easier...

TEST PLAN
  Builds; not rebooted yet

REPOSITORY
  R120 Plasma Workspace

BRANCH
  Plasma/5.17

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

AFFECTED FILES
  startkde/startplasma.cpp

To: dfaure, apol
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D24238: Port the pager applet away from QtWidgets

[David Faure]

dfaure added inline comments.

INLINE COMMENTS

> broulik wrote in windowmodel.cpp:81
> Isn't that the combined geometry of all screens, not just a single screen?

Oh indeed. But then this was the wrong thing to use anyway. You don't want to 
center the window in relation to the combined geometry, as the documentation of 
QDesktopWidget rightfully says.

Not that I really understand this code, I admit.

REPOSITORY
  R119 Plasma Desktop

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

To: dfaure, hein, apol
Cc: broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, 
GB_2, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D24238: Port the pager applet away from QtWidgets

[David Faure]

dfaure created this revision.
dfaure added reviewers: hein, apol.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
dfaure requested review of this revision.

TEST PLAN
  Builds

REPOSITORY
  R119 Plasma Desktop

BRANCH
  Plasma/5.17

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

AFFECTED FILES
  applets/pager/plugin/pagermodel.cpp
  applets/pager/plugin/windowmodel.cpp

To: dfaure, hein, apol
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D24007: [QQC2 Desktop Style] Port away from deprecated methods in Qt 5.14

[David Faure]

dfaure closed this revision.

REPOSITORY
  R858 Qt Quick Controls 2: Desktop Style

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

To: dfaure, mart, davidedmundson
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D24007: [QQC2 Desktop Style] Port away from deprecated methods in Qt 5.14

[David Faure]

dfaure created this revision.
dfaure added a reviewer: mart.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
dfaure requested review of this revision.

REPOSITORY
  R858 Qt Quick Controls 2: Desktop Style

BRANCH
  master

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

AFFECTED FILES
  plugin/kquickstyleitem.cpp

To: dfaure, mart
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D23808: Don't create() windows that aren't native, upon receiving a palette change event.

[David Faure]

dfaure closed this revision.

REPOSITORY
  R135 Integration for Qt applications in Plasma

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

To: dfaure, carewolf, davidedmundson, broulik
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D23823: [KMenuEdit] port away from KStandardDirs

[David Faure]

dfaure closed this revision.

REPOSITORY
  R103 KMenu Editor

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

To: dfaure, mlaurent
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D23823: [KMenuEdit] port away from KStandardDirs

[David Faure]

dfaure created this revision.
dfaure added a reviewer: mlaurent.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
dfaure requested review of this revision.

TEST PLAN
  only did smoke testing (kmenuedit starts and displays the tree, clicking
  on one item shows details).

REPOSITORY
  R103 KMenu Editor

BRANCH
  master

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

AFFECTED FILES
  menufile.cpp
  menuinfo.cpp
  treeview.cpp

To: dfaure, mlaurent
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D23808: Don't create() windows that aren't native, upon receiving a palette change event.

[David Faure]

dfaure created this revision.
dfaure added reviewers: carewolf, davidedmundson, broulik.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
dfaure requested review of this revision.

TEST PLAN
  run kmail or falkon, then run kcmshell5 fonts, change font size
  
  ASSERT: "!d->offscreenWindow->handle()" in file 
/d/qt/5/kde/qtdeclarative/src/quickwidgets/qquickwidget.cpp, line 1014
  
  Apparently that QQuickWindow should not be create()d so calling winId()
  unconditionally here is a bad idea, skip non-native windows.

REPOSITORY
  R135 Integration for Qt applications in Plasma

BRANCH
  Plasma/5.16

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

AFFECTED FILES
  src/platformtheme/x11integration.cpp

To: dfaure, carewolf, davidedmundson, broulik
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D23198: Skip mime type check only for files on network mounts

[David Faure]

dfaure added a comment.


  My suggestion is (still) "improve KFileItem to enable SkipMimeTypeFromContent 
automatically if isSlow is true"
  [possibly with another flag if you think there are actually use cases for 
mime-magic over network mounts]
  
  Then all this code isn't needed.

REPOSITORY
  R119 Plasma Desktop

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

To: meven, #plasma, dfaure
Cc: davidedmundson, ngraham, broulik, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D23198: Skip mime type check only for files on network mounts

[David Faure]

dfaure added a comment.


  What I mean is:
  
  1. improve KFileItem::isSlow to use this Solid code
  2. improve KFileItem to enable SkipMimeTypeFromContent automatically if 
isSlow is true
  
  However maybe we don't want that in dolphin? Not sure. (In that case a new 
enum value can be added, to disable some features "if slow".)
  
  In any case I see no reason for this code to be in recentusagemodel, it's 
just one out of many KFileItem users, with similar needs as most others.
  
  (And all this is the reason I hate network mounts, and the much-requested 
fuse-kio integration... what looks like a local path ends up being unusable as 
a local path and needs special treatment anyway...)

REPOSITORY
  R119 Plasma Desktop

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

To: meven, #plasma, dfaure
Cc: davidedmundson, ngraham, broulik, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D23198: Skip mime type check only for files on network mounts

[David Faure]

dfaure added a comment.


  Why not do this in KFileItem::isSlow() itself?
  (and enable SkipMimeTypeFromContent when isSlow is true)

REPOSITORY
  R119 Plasma Desktop

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

To: meven, #plasma, dfaure
Cc: davidedmundson, ngraham, broulik, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D23228: Port away from deprecated KWindowSystem API

[David Faure]

dfaure added a comment.


  Thanks for the testing!

REPOSITORY
  R133 KScreenLocker

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

To: dfaure, zzag, graesslin, davidedmundson
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D23228: Port away from deprecated KWindowSystem API

[David Faure]

dfaure closed this revision.

REPOSITORY
  R133 KScreenLocker

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

To: dfaure, zzag, graesslin, davidedmundson
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D23334: Remove slideWindow(QWidget*) overload with recent KWindowSystem

[David Faure]

dfaure closed this revision.

REPOSITORY
  R130 Frameworks integration plugin using KWayland

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

To: dfaure, zzag
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D23334: Remove slideWindow(QWidget*) overload with recent KWindowSystem

[David Faure]

dfaure created this revision.
dfaure added a reviewer: zzag.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
dfaure requested review of this revision.

TEST PLAN
  Fixes the build after https://phabricator.kde.org/D23213

REPOSITORY
  R130 Frameworks integration plugin using KWayland

BRANCH
  Plasma/5.16

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

AFFECTED FILES
  src/windowsystem/windoweffects.cpp
  src/windowsystem/windoweffects.h

To: dfaure, zzag
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D23228: Port away from deprecated KWindowSystem API

[David Faure]

dfaure created this revision.
dfaure added reviewers: zzag, graesslin, davidedmundson.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
dfaure requested review of this revision.

REVISION SUMMARY
  Call the all-in-one method (which handles numeric keypad better),
  then split out modifiers and keycode.

TEST PLAN
  Builds, didn't test yet.

REPOSITORY
  R133 KScreenLocker

BRANCH
  Plasma/5.16

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

AFFECTED FILES
  globalaccel.cpp

To: dfaure, zzag, graesslin, davidedmundson
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

Re: Unified git commit message guideline

[David Faure]

Hello Roman,

On dimanche 11 août 2019 23:52:55 CEST Roman Gilg wrote:
> [1] https://www.conventionalcommits.org

I like the idea very much, the changelog does look a bit messy indeed.
It would allow me to filter out all style, ci, and test changes, which are not 
interesting to the user of the frameworks.

I'm missing a type for internal cleanups like porting away from deprecated 
methods or fixing a harmless compiler warning? OTOH "chore" isn't documented 
in the Angular convention so I don't know what it is. In any case, it sounds 
like we need to write down our own list of types, right?

I would also like to keep the "Test Plan" field from phab even after we move 
to gitlab, it pushes people to write down how they actually tested the change.

I think we should wait until after the switch to gitlab though, because it 
might have some influence on this. For instance with phab I tend to prefix the 
subject with the repo name so that on k-f-d we can see which framework it's 
about. But that looks a bit redundant in the git history of a given repo 
afterwards so if the emails from gitlab include the repo name automatically, 
we shouldn't do that in the git commit log itself.

-- 
David Faure, fa...@kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5

D19784: Avoid calling QT_LSTAT and accessing recent documents

[David Faure]

dfaure added a comment.


  See KFileItem::isSlow()

REPOSITORY
  R119 Plasma Desktop

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

To: hoffmannrobert, #frameworks, dfaure, #dolphin
Cc: meven, apol, ngraham, plasma-devel, LeGast00n, jraleigh, fbampaloukas, 
GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, mart

D22525: kioclient: Don't convert `:x:y` to `?line=x=y` for URLs starting with remote schemes.

[David Faure]

dfaure added a comment.


  Backported. 
https://commits.kde.org/kde-cli-tools/42ef318a9d1c454f96b60181d8231a59233720ea

REPOSITORY
  R126 KDE CLI Utilities

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

To: arrowd, #frameworks, dfaure
Cc: wbauer, kwrite-devel, dfaure, cfeck, plasma-devel, #frameworks, LeGast00n, 
jraleigh, fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D21517: Fix "Type error" when creating a TextField with focus: true

[David Faure]

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 R858:e419e9b42287: Fix Type error when creating a 
TextField with focus: true (authored by dfaure).
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.

REPOSITORY
  R858 Qt Quick Controls 2: Desktop Style

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21517?vs=62107=62111

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

AFFECTED FILES
  org.kde.desktop/private/MobileTextActionsToolBar.qml
  tests/LineEditWithClearButton.qml

To: dfaure, mart, davidedmundson, apol, broulik
Cc: plasma-devel, broulik, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, 
Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D22525: kioclient: Don't convert `:x:y` to `?line=x=y` for URLs starting with remote schemes.

[David Faure]

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


  OK, I'm wrong (not the first time this happens) :-)
  
  `kwrite /home/dfaure/.zshrc:20` works
  `kwrite sftp://localhost/home/dfaure/.zshrc:20` doesn't work indeed.
  
  The patch looks fine then.

REPOSITORY
  R126 KDE CLI Utilities

BRANCH
  master

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

To: arrowd, #frameworks, dfaure
Cc: kwrite-devel, dfaure, cfeck, plasma-devel, #frameworks, LeGast00n, 
jraleigh, fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D22525: kioclient: Don't convert `:x:y` to `?line=x=y` for URLs starting with remote schemes.

[David Faure]

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


  I suppose the kate developers like the fact that this currently works over 
FTP, SFTP, FISH, SMB, etc.
  So maybe only HTTP[S]/WEBDAV should be blacklisted (because there queries 
have a different meaning, one that we can't know client-side).

REPOSITORY
  R126 KDE CLI Utilities

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

To: arrowd, #frameworks, dfaure
Cc: dfaure, cfeck, plasma-devel, #frameworks, LeGast00n, jraleigh, 
fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D21514: MobileTextActionsToolBar: fix runtime warnings when controlRoot isn't set yet

[David Faure]

dfaure closed this revision.

REPOSITORY
  R858 Qt Quick Controls 2: Desktop Style

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

To: dfaure, mart, apol, broulik, ngraham
Cc: astippich, plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, 
Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D21959: Fix selectedNameFilter() multiple matches

[David Faure]

dfaure closed this revision.

REPOSITORY
  R135 Integration for Qt applications in Plasma

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

To: hoffmannrobert, #frameworks, apol, dfaure
Cc: michaelweghorn, plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D21959: Fix selectedNameFilter() multiple matches

[David Faure]

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


  +1 for the included unittest.

INLINE COMMENTS

> kdeplatformfiledialoghelper.cpp:80
>   */
> -static QString kde2QtFilter(const QStringList , const QString )
> +static QString kde2QtFilter(const QStringList , const QString , 
> const QString )
>  {

(hehe I keep thinking this is about KDE-2 stuff... what a dinosaur I am)

> kdeplatformfiledialoghelper.cpp:90
> +(QLatin1Char(')') == (*it)[pos + kde.length()] || 
> QLatin1Char(' ') == (*it)[pos + kde.length()]) &&
> +(filterText.isEmpty() || (!filterText.isEmpty() && 
> it->startsWith(filterText {
>  return *it;

the `!filterText.isEmpty() &&` part is redundant.
In this part of the condition, we know it's not empty, otherwise shortcut 
evaluation happened after isEmpty returns true.

  blue or (not blue and green)

is really the same as

  blue or green

REPOSITORY
  R135 Integration for Qt applications in Plasma

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

To: hoffmannrobert, #frameworks, apol, dfaure
Cc: michaelweghorn, plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D17372: [componentchooser KCM] Make KIO browser option the fallback only and remove from the UI

[David Faure]

dfaure added a comment.


  Writing into the user's home dir is a "hack".
  
  There are better ways for distros to set defaults (such as the global 
mimeapps.list) but it seems KIO ignores that... (to be checked...)

REPOSITORY
  R119 Plasma Desktop

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

To: ngraham, #plasma, #vdg, #frameworks, dfaure
Cc: GB_2, dfaure, abetts, davidedmundson, plasma-devel, jraleigh, ragreen, 
Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, 
mart

D17372: [componentchooser KCM] Make KIO browser option the fallback only and remove from the UI

[David Faure]

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


  I agree that we shouldn't remove the underlying feature, in fact it's still 
used on other protocols than http.
  
  But we have to ensure the default behaviour matches the KCM and vice-versa, 
for http.

INLINE COMMENTS

> componentchooserbrowser.cpp:68
> +if (browserCombo->count() > 0) {
> +const QString  = 
> browserCombo->currentData().toString();
> +m_browserService = KService::serviceByStorageId(storageId);

This will lead to rather confusing user experience, btw.

On a freshly installed system [without distro hacks], BrowserApplication is 
empty.
In practice that means KRun::setEnableExternalBrowser will pick the entry 
x-scheme-handler/https in the user's mimeapps.list, if any. And if that isn't 
set either, then well, the current behaviour in KRun, AFAICS, *will* be to use 
KIO to find out the mimetype.

Then the user opens this KCM, and it says "your default browser is firefox" (or 
whichever is first in the list). "What?" says the user... "that's not what I'm 
experiencing..."

If you want to change the default behaviour, you need to actually change the 
default behaviour (in KRun), not just what the KCM shows by default.

The default behaviour of KRun and the default settings shown by this KCM have 
to match.

This investigation shows that this is already buggy before this patch (the 
fallback to x-scheme-handler/https in the user's mimeapps.list is missing 
here), but this patch makes it much worse, as is.

REPOSITORY
  R119 Plasma Desktop

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

To: ngraham, #plasma, #vdg, #frameworks, dfaure
Cc: GB_2, dfaure, abetts, davidedmundson, plasma-devel, jraleigh, ragreen, 
Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, 
mart

D21514: MobileTextActionsToolBar: fix runtime warnings when controlRoot isn't set yet

[David Faure]

dfaure created this revision.
dfaure added reviewers: mart, apol, broulik.
Herald added a project: Plasma.
dfaure requested review of this revision.

REVISION SUMMARY
  This file checks for controlRoot being undefined, everywhere else.

TEST PLAN
  `qmlscene tests/testComboBox.qml` showed
  MobileTextActionsToolBar.qml:62: TypeError: Cannot read property 
'selectedText' of null
  MobileTextActionsToolBar.qml:70: TypeError: Cannot read property 
'selectedText' of null
  MobileTextActionsToolBar.qml:78: TypeError: Cannot read property 'canPaste' 
of null

REPOSITORY
  R858 Qt Quick Controls 2: Desktop Style

BRANCH
  2019_fix_MobileTextActionsToolBar_warnings

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

AFFECTED FILES
  org.kde.desktop/private/MobileTextActionsToolBar.qml

To: dfaure, mart, apol, broulik
Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D18296: Add support for passing cursor information via URL parameters when running kioclient exec.

[David Faure]

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

INLINE COMMENTS

> urlinfo.h:76
> + *   - make relative paths absolute using the current working 
> directory
> + *   - prefer local file, if in doubt!
> + */

remove old comment

REPOSITORY
  R126 KDE CLI Utilities

BRANCH
  cursor

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

To: arrowd, #plasma, #ktexteditor, broulik, #frameworks, pino, cfeck, dfaure, 
elvisangelaccio
Cc: apol, cullmann, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart

D18296: Add support for passing cursor information via URL parameters when running kioclient exec.

[David Faure]

dfaure added inline comments.

INLINE COMMENTS

> arrowd wrote in urlinfo.h:39
> > const QString &
> 
> There is `path.chop(match.capturedLength());`, which requires non-const 
> `QString`.
> 
> > And what if it's a URL? At this point this string is pathOrUrl.
> 
> Well, `if (QFile::exists(path))` will return false in this case, and `url` 
> would get populated by `url = QUrl::fromUserInput()`. What's wrong with that?

I'm suggesting to at least rename the argument to pathOrUrl to be clear about 
what it contains.
A path is not a URL.

I see, about the `chop()`. It is customary, however, to make a local copy where 
needed. For instance this will avoid the copy when the file exists (and we get 
to the early return, before the copy that you'll need only further down).

> arrowd wrote in urlinfo.h:77
> It is not about creating missing files, but reaction to user typos. If I try 
> to open `fiel.txt` instead of a `file.txt`, I want to get a "no such file or 
> directory error message" instead of popping browser trying to open 
> "http://fiel.txt;.

But then you can't do `kde-open5 www.google.fr` anymore, right?

I see what you mean with typo handling, but there is no perfect solution. 
Either we treat typos as URLs (but it means we also treat actual URLs as such), 
or we treat everything non-existing as a local file (breaking any use of short 
URLs). The latter is OK for kwrite, but not for the more general purpose 
kioclient / kde-open5.

REPOSITORY
  R126 KDE CLI Utilities

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

To: arrowd, #plasma, #ktexteditor, broulik, #frameworks, pino, cfeck, dfaure, 
elvisangelaccio
Cc: apol, cullmann, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart

D18296: Add support for passing cursor information via URL parameters when running kioclient exec.

[David Faure]

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


  Is the makeURL function still used, or should it be removed now?

INLINE COMMENTS

> urlinfo.h:39
> + */
> +UrlInfo(QString path)
> +: line(0), column(0)

const QString &

And what if it's a URL? At this point this string is pathOrUrl.

> urlinfo.h:77
> + */
> +url = QUrl::fromUserInput(path, QDir::currentPath(), 
> QUrl::AssumeLocalFile);
> +

I'm not sure about AssumeLocalFile, in the context of kde-open.
This is about opening existing files, not creating new files.
So it should be removed.

REPOSITORY
  R126 KDE CLI Utilities

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

To: arrowd, #plasma, #ktexteditor, broulik, #frameworks, pino, cfeck, dfaure, 
elvisangelaccio
Cc: apol, cullmann, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart

D19784: Avoid calling QT_LSTAT and accessing recent documents

[David Faure]

This revision was automatically updated to reflect the committed changes.
Closed by commit R119:408f03ca989e: Avoid calling QT_LSTAT and accessing recent 
documents (authored by hoffmannrobert, committed by dfaure).

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D19784?vs=55115=55142

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

AFFECTED FILES
  applets/kicker/plugin/recentusagemodel.cpp

To: hoffmannrobert, #frameworks, dfaure, #dolphin
Cc: apol, ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart

D19784: Avoid calling QT_LSTAT and accessing recent documents

[David Faure]

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


  This needs a KIO version ifdef for KIO >= 5.57.
  
  `#if KIO_VERSION >= QT_VERSION_CHECK(5,57,0)`

REPOSITORY
  R119 Plasma Desktop

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

To: hoffmannrobert, #frameworks, dfaure, #dolphin
Cc: apol, ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart

D19784: Avoid calling QT_LSTAT and accessing recent documents

[David Faure]

dfaure added a comment.


  > If they point to files on a network drive, and the network or the drive is 
not responding
  
  Well that's exactly the problem with network mounts, and the reason they are 
a sucky technical solution.
  KIO's async jobs never have that problem.
  
  You will never be able to remove all uses of synchronous local file APIs in 
all of Qt and KDE-made software -- or IMHO any other large toolkit or 
application.
  
  At best, the kernel should offer better solutions for users to get rid of 
non-responding mounts more easily.

REPOSITORY
  R119 Plasma Desktop

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

To: hoffmannrobert, #frameworks, dfaure, #dolphin
Cc: apol, ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart

D19784: Avoid calling QT_LSTAT and accessing recent documents

[David Faure]

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


  OK so this is about KFileItem::text() and KFileItem::iconName().
  
  Indeed this doesn't need the stat() done by KFileItem's init(). This means 
the right solution is indeed for KFileItem to do that stat() on demand, and 
this code doesn't need any changes.

REPOSITORY
  R119 Plasma Desktop

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

To: hoffmannrobert, #frameworks, dfaure, #dolphin
Cc: apol, ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart

D19500: [KDirModel] Fix job urls change signal connection

[David Faure]

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


  If there was a unittest for this code, it wouldn't remain broken for so 
long... Feel free to add one :-)

REPOSITORY
  R119 Plasma Desktop

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

To: broulik, #frameworks, dfaure, jtamate
Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D19784: Avoid calling QT_LSTAT and accessing recent documents

[David Faure]

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

INLINE COMMENTS

> recentusagemodel.cpp:261
>  
>  if (!url.isValid() || !(fileItem.isFile() || fileItem.isDir())) {
>  return QVariant();

How do you expect isFile() and isDir() to work without a QT_LSTAT call?

REPOSITORY
  R119 Plasma Desktop

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

To: hoffmannrobert, #frameworks, dfaure, #dolphin
Cc: apol, ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart

D19588: [Notifications] Improve finished notification

[David Faure]

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

INLINE COMMENTS

> anthonyfieroni wrote in Jobs.qml:117-120
>   displayDestUrl = destUrl.replace(/^(file:\/{2})/, "")

All of this is wrong. Removing file:/// leaves something that is neither a 
correct path nor a correct URL.

You want destUrl.toString(QUrl::PreferLocalFile).

This code being javascript is no excuse -- move all this to C++ code :-)

> Jobs.qml:128
> +if (url[0] === "/") {
> +url = "file://" + url;
> +}

Nooo.
This is broken. Testcase: try a filename with a '#' in it.

If the input is indeed a "path or URL" (urgh, that's never good practice, other 
than for showing to the user), then the way to turn this into a URL is 
QUrl::fromUserInput().

REPOSITORY
  R120 Plasma Workspace

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

To: broulik, #plasma, #vdg, dfaure
Cc: anthonyfieroni, rooty, plasma-devel, jraleigh, GB_2, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15189: [KRun] Don’t follow redirection to speed up and avoid incorrect behavior

[David Faure]

dfaure added a comment.


#include 

#if KIO_VERSION >= QT_VERSION_CHECK(5,55,0)
...
#endif

REPOSITORY
  R126 KDE CLI Utilities

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

To: achauvel, #frameworks, dfaure, cfeck
Cc: plasma-devel, anthonyfieroni, ngraham, kde-frameworks-devel, jraleigh, 
GB_2, ragreen, Pitel, michaelh, ZrenBot, bruns, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart