D29397: KPreviewJob : Support for DeviceRatioPixel

[David Faure]

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

INLINE COMMENTS

> previewjob.cpp:173
> +
> +void PreviewJob::setDefaultDevicePixelRatio(qreal defaultDevicePixelRatio) {
> +s_defaultDevicePixelRatio = defaultDevicePixelRatio;

coding style: '{' on separate line

> thumbcreator.cpp:52
> +
> +bool ThumbCreatorV3::create(const QString , int width, int height, 
> QImage ) {
> +return createV3(path, width, height, img, 1);

'{' on its own line for methods

> thumbcreator.h:215
> + * @param img The QImage to store the preview in.
> + * @param devicePixelRatio
> + *

docu?

REPOSITORY
  R241 KIO

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

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

D29528: [OpenUrlJob] Improve comments/docs

[David Faure]

dfaure added a comment.


  Thanks!

INLINE COMMENTS

> openurljob.cpp:393
> +if (mime.isValid() && mimeName != m_mimeTypeName) {
> +m_mimeTypeName =mimeName;
>  }

missing space after '='

> openurljob.cpp:590
> +const QMimeType mimeType = db.mimeTypeForName(m_mimeTypeName);
> +if (isExecutableMime(mimeType) && handleExecutables(mimeType)) {
> +return;

That one was on purpose. I find this version less readable, mixing a test and 
an actual action (with error handling).

> openurljob.h:114
>   * Starts the job.
> - * You must call this, after having done all the setters.
> + * You must call this, after having called all the needed setters.
>   * This is a GUI job, never use exec(), it would block user interaction.

I think I used the sentence in the two LauncherJobs, feel free to make the same 
change here.

REPOSITORY
  R241 KIO

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

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

D19080: Make file overwrite a bit safer

[David Faure]

dfaure added inline comments.

INLINE COMMENTS

> dfaure wrote in jobtest.cpp:1759
> The other problems were fixed, but this test still fails randomly.
> 
> In fact, why are we checking that the dest file already started to be created 
> when totalSize is emitted?
> Surely copying involves looking at the src file first, emitting totalSize, 
> and *then* starting to create the destination, no?
> 
> Maybe it's not totalSize we should connect to, but more of a progress 
> signal

Done in https://commits.kde.org/kio/30d74b814f794e52fa4561181b7b864558144e3a

REPOSITORY
  R241 KIO

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

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

D29524: ECMGeneratePriFile: fix for ECM_MKSPECS_INSTALL_DIR being absolute

[David Faure]

dfaure closed this revision.

REPOSITORY
  R240 Extra CMake Modules

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

To: dfaure, cgiboudeaux, vatra, kfunk, apol, vkrause
Cc: kde-frameworks-devel, kde-buildsystem, LeGast00n, cblack, bencreasy, 
michaelh, ngraham, bruns

D19080: Make file overwrite a bit safer

[David Faure]

dfaure added inline comments.

INLINE COMMENTS

> jobtest.cpp:1759
> +Q_UNUSED(totalSize);
> +QCOMPARE(destFileExists,  QFile::exists(destPartFile));
> +});

The other problems were fixed, but this test still fails randomly.

In fact, why are we checking that the dest file already started to be created 
when totalSize is emitted?
Surely copying involves looking at the src file first, emitting totalSize, and 
*then* starting to create the destination, no?

Maybe it's not totalSize we should connect to, but more of a progress signal

REPOSITORY
  R241 KIO

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

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

D29524: ECMGeneratePriFile: fix for ECM_MKSPECS_INSTALL_DIR being absolute

[David Faure]

dfaure created this revision.
dfaure added reviewers: cgiboudeaux, vatra, kfunk, apol, vkrause.
Herald added projects: Frameworks, Build System.
Herald added subscribers: kde-buildsystem, kde-frameworks-devel.
dfaure requested review of this revision.

TEST PLAN
  works as before for the case where it's relative.

REPOSITORY
  R240 Extra CMake Modules

BRANCH
  master

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

AFFECTED FILES
  modules/ECMGeneratePriFile.cmake

To: dfaure, cgiboudeaux, vatra, kfunk, apol, vkrause
Cc: kde-frameworks-devel, kde-buildsystem, LeGast00n, cblack, bencreasy, 
michaelh, ngraham, bruns

D29485: [CopyJob] Check free space for remote urls before copying and other improvements

[David Faure]

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


  Good idea overall.

INLINE COMMENTS

> copyjob.cpp:430
> +if (!m_privilegeExecutionEnabled && !isWritable) {
> +// In copy-as mode, we want to check the directory to which 
> we're copying.
> +// The target file or directory does not exist yet.

Here you kept a comment that said "we want to check", but the check already 
happened.
I'd say just remove the two lines of comments.
The code is clearer without them.

> copyjob.cpp:433
> +const QUrl dest = m_asMethod ? 
> m_dest.adjusted(QUrl::RemoveFilename) : m_dest;
> +const QString path = dest.isLocalFile() ? dest.toLocalFile() 
> : dest.toString();
> +q->setError(ERR_WRITE_ACCESS_DENIED);

That is not a path, for remote URLs. I suggest using 
dest.toDisplayString(QUrl::PreferLocalFiles) and just inlining that in the 
setErrorText call.

> copyjob.cpp:444
>  
> -const QString sLocalPath = 
> entry.stringValue(KIO::UDSEntry::UDS_LOCAL_PATH);
> -if (!sLocalPath.isEmpty() && kio_resolve_local_urls) {
> -const QString fileName = m_dest.fileName();
> -m_dest = QUrl::fromLocalFile(sLocalPath);
> -if (m_asMethod) {
> -m_dest = addPathToUrl(m_dest, fileName);
> +if (m_dest.isLocalFile()) {
> +// Fast code path, if a dir is already in the kcoredirlister 
> cache and it was e.g.

I think this check is too early.

UDS_LOCAL_PATH is also set by kio_desktop and kio_remote, for instance.
It's the main point of that entry: to map URLs from 
kioslaves-that-wrap-the-local-file-system back to local paths.

AFAICS the old code would use UDS_LOCAL_PATH also for non-local-file URLs.

> copyjob.cpp:445
> +if (m_dest.isLocalFile()) {
> +// Fast code path, if a dir is already in the kcoredirlister 
> cache and it was e.g.
> +// renamed and got UDS_LOCAL_PATH set

I completely fail to see the relation between this comment and the code.

"If a dir is already in kcoredirlister" is only relevant when calling 
KCoreDirLister::cachedItemForUrl, but that's not done here.

> copyjob.cpp:460
>  }
> -qCDebug(KIO_COPYJOB_DEBUG) << "Setting m_dest to the local 
> path:" << sLocalPath;
> -if (isGlobalDest) {
> -m_globalDest = m_dest;
> +
> +const QUrl dest = m_asMethod ? 
> m_dest.adjusted(QUrl::RemoveFilename) : m_dest;

The check for m_dest.isLocalFile() goes here.

Given line 451, maybe it wasn't local initially, and it is now.

> copyjob.cpp:461
> +
> +const QUrl dest = m_asMethod ? 
> m_dest.adjusted(QUrl::RemoveFilename) : m_dest;
> +const QString path = dest.toLocalFile();

Now *this* is where the comment about "we want to check..." belongs :-)

The explanation of why we go "up" in case of copyAs, in order not to confuse 
KDiskFreeSpaceInfo.

> copyjob.cpp:465
> +// Check available free space for local urls
> +KDiskFreeSpaceInfo freeSpaceInfo 
> =KDiskFreeSpaceInfo::freeSpaceInfo(path);
> +if (freeSpaceInfo.isValid()) {

missing space after =

> copyjob.cpp:478
>  
> +// Must do this here before statCurrentSrc() is called in the lambda 
> connected to
> +// FileSystemFreeSpaceJob below

The lambda is called after going back to the event loop, so this will have 
happened anyway, no?
I don't get it.

On the other hand I'm fine if this is done here, I'm just not sure why the 
comment says it has to be so.

(Easy solution is to remove that comment, especially given the suggestion below 
it won't even seem weird to do things in this order)

> copyjob.cpp:485
> +// TODO: find a way to report connection errors to the user
> +if (!m_dest.isLocalFile()) {
> +const QUrl dest = m_asMethod ? 
> m_dest.adjusted(QUrl::RemoveFilename) : m_dest;

Maybe you can move the if (m_dest.isLocalFile()) block here, and use `else`.
It'll be clearer that free space check happens in both cases.

> copyjob.cpp:486
> +if (!m_dest.isLocalFile()) {
> +const QUrl dest = m_asMethod ? 
> m_dest.adjusted(QUrl::RemoveFilename) : m_dest;
> +KIO::FileSystemFreeSpaceJob *spaceJob = 
> KIO::fileSystemFreeSpace(dest);

... and this line is the same in both code paths, so it could be extracted to 
before the if().

To avoid confusion between dest and m_dest, maybe rename this var to 
destToCheck ?

REPOSITORY
  R241 KIO

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

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

D29274: ECMGeneratePriFile: make the pri files relocatable

[David Faure]

dfaure closed this revision.

REPOSITORY
  R240 Extra CMake Modules

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

To: dfaure, vatra, kfunk, apol, vkrause
Cc: ablu, kossebau, kde-frameworks-devel, kde-buildsystem, LeGast00n, cblack, 
bencreasy, michaelh, ngraham, bruns

D29347: KAuthorized: export method to reload restrictions

[David Faure]

dfaure added a comment.


  Good idea, done in 
https://commits.kde.org/kconfig/8e0f84030cc1d1e517ca75311ed9850ce88fac41

REPOSITORY
  R237 KConfig

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

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

D29385: Introduce KIO::OpenUrlJob, a rewrite and replacement for KRun

[David Faure]

dfaure closed this revision.

REPOSITORY
  R241 KIO

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

To: dfaure, ahmadsamir, broulik, meven, kossebau, davidedmundson, nicolasfella, 
svuorela
Cc: feverfew, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29397: KPreviewJob : Support for DeviceRatioPixel

[David Faure]

dfaure added a comment.


  In D29397#664536 , @meven wrote:
  
  > In D29397#663800 , @dfaure wrote:
  >
  > > Oh, I thought it was sent as an int. But 8 is 
QImage::Format_ARGB8565_Premultiplied. Did you mean 0x80?
  >
  >
  > No I meant 8, since format is passed as quint8 (a single byte, 8-bit) and 
the format is forced to QImage::Format_ARGB32 "5", so this works.
  
  
  OK, this works today. But if one day we want to start actually using other 
image formats, we'll end up with a clash here.
  Why not use 0x80 in order to stay away from valid image format values? This 
seems safer to me, in the long run.

REPOSITORY
  R241 KIO

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

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

D29385: Introduce KIO::OpenUrlJob, a rewrite and replacement for KRun

[David Faure]

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


  -void KIO::OpenUrlJob::setRunFlags(KIO::ApplicationLauncherJob::RunFlags 
runFlags)
  +void KIO::OpenUrlJob::setDeleteTemporaryFile(bool b)
  
  The more I think about it, the least I like the use of flags here.
  
  1. they are from the wrong class as Kai-Uwe pointed out, but more importantly:
  2. the other bool setters here are for unrelated concerns, better keep them 
separate.
  
  Are we doing lineEdit->setDragEnabled(true); 
lineEdit->setClearButtonEnabled(true); lineEdit->setReadOnly(true);
  or are we doing lineEdit->setFlags(QLineEdit::DragEnabled | 
QLineEdit::ClearButtonEnabled | QLineEdit::ReadOnly)?

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29385?vs=81840=82058

BRANCH
  2020_05_openurljob

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

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/krununittest.cpp
  autotests/openurljobtest.cpp
  autotests/openurljobtest.h
  src/core/kurlauthorized.h
  src/gui/CMakeLists.txt
  src/gui/applicationlauncherjob.cpp
  src/gui/applicationlauncherjob.h
  src/gui/openurljob.cpp
  src/gui/openurljob.h
  src/gui/openurljobhandlerinterface.cpp
  src/gui/openurljobhandlerinterface.h
  src/widgets/CMakeLists.txt
  src/widgets/jobuidelegate.cpp
  src/widgets/kdesktopfileactions.cpp
  src/widgets/kopenwithdialog.cpp
  src/widgets/kopenwithdialog.h
  src/widgets/krun.cpp
  src/widgets/widgetsopenurljobhandler.cpp
  src/widgets/widgetsopenurljobhandler.h

To: dfaure, ahmadsamir, broulik, meven, kossebau, davidedmundson, nicolasfella, 
svuorela
Cc: feverfew, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29445: [KOpenWithDialog] When pointing at a non-executable file print more meaningful error

[David Faure]

dfaure added inline comments.

INLINE COMMENTS

> apol wrote in kopenwithdialog.cpp:1006
> This patch D29170  suggests that 
> findExecutable won't find non-executables.
> 
> Something's wrong.

Right, hence the isEmpty() here. This check passes if 1) it doesn't exist, or 
2) it's not executable.

REPOSITORY
  R241 KIO

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

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

D29397: KPreviewJob : Support for DeviceRatioPixel

[David Faure]

dfaure added a comment.


  Oh, I thought it was sent as an int. But 8 is 
QImage::Format_ARGB8565_Premultiplied. Did you mean 0x80?

INLINE COMMENTS

> thumbcreator.h:191
> + * @class ThumbCreatorV3 thumbcreator.h 
> + * @since 5.70
> + */

5.70 is tagged already

REPOSITORY
  R241 KIO

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

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

D29427: [KBookmarkMenu] Assign m_actionCollection early to prevent crash

[David Faure]

dfaure added a comment.


  Yep, doing it already.

REPOSITORY
  R294 KBookmarks

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

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

D29427: [KBookmarkMenu] Assign m_actionCollection early to prevent crash

[David Faure]

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

REPOSITORY
  R294 KBookmarks

BRANCH
  l-bookmarkmenu-assign-actioncoll (branched from master)

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

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

D29385: Introduce KIO::OpenUrlJob, a rewrite and replacement for KRun

[David Faure]

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


  Remove local-files fast path.
  
  This also showed a small mistake in the handling of error texts
  (this job isn't a KIO::Job so we need to call buildErrorString ourselves).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29385?vs=81830=81840

BRANCH
  2020_05_openurljob

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

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/krununittest.cpp
  autotests/openurljobtest.cpp
  autotests/openurljobtest.h
  src/core/kurlauthorized.h
  src/gui/CMakeLists.txt
  src/gui/applicationlauncherjob.cpp
  src/gui/applicationlauncherjob.h
  src/gui/openurljob.cpp
  src/gui/openurljob.h
  src/gui/openurljobhandlerinterface.cpp
  src/gui/openurljobhandlerinterface.h
  src/widgets/CMakeLists.txt
  src/widgets/jobuidelegate.cpp
  src/widgets/kdesktopfileactions.cpp
  src/widgets/kopenwithdialog.cpp
  src/widgets/kopenwithdialog.h
  src/widgets/krun.cpp
  src/widgets/widgetsopenurljobhandler.cpp
  src/widgets/widgetsopenurljobhandler.h

To: dfaure, ahmadsamir, broulik, meven, kossebau, davidedmundson, nicolasfella, 
svuorela
Cc: feverfew, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29397: KPreviewJob : Support for DeviceRatioPixel

[David Faure]

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

INLINE COMMENTS

> previewjob.cpp:717
> +qreal imgDevicePixelRatio;
> +str >> width >> height >> iFormat >> imgDevicePixelRatio;
>  QImage::Format format = static_cast(iFormat);

This here also breaks compatibility. Add a KF6 TODO to start the serialization 
with a version number.

Meanwhile a hack is needed, like `if (iFormat & 0x1000) { iFormat &= 0xFFF; str 
>> imgDevicePixelRatio; }`
and of course setting that 0x1000 flag in the slaves that have been updated to 
provide the pixelratio.

> thumbcreator.h:183
>  class KIOWIDGETS_DEPRECATED_VERSION(5, 0, "Use ThumbCreator")
>  KIOWIDGETS_EXPORT ThumbCreatorV2 : public ThumbCreator
>  {

You need to do like we once did: define an interface that inherits from 
ThumbCreator.
Call it V3

In the job, use dynamic_cast to test whether the object provided by the 
kioslave supports the V3 interface or not.

REPOSITORY
  R241 KIO

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

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

D29385: Introduce KIO::OpenUrlJob, a rewrite and replacement for KRun

[David Faure]

dfaure updated this revision to Diff 81830.
dfaure marked an inline comment as done.
dfaure added a comment.


  Multiple fixes thanks to review comments; didn't remove the fast path yet (a 
unittest needs fixup)

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29385?vs=81814=81830

BRANCH
  2020_05_openurljob

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

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/krununittest.cpp
  autotests/openurljobtest.cpp
  autotests/openurljobtest.h
  src/core/kurlauthorized.h
  src/gui/CMakeLists.txt
  src/gui/applicationlauncherjob.cpp
  src/gui/applicationlauncherjob.h
  src/gui/openurljob.cpp
  src/gui/openurljob.h
  src/gui/openurljobhandlerinterface.cpp
  src/gui/openurljobhandlerinterface.h
  src/widgets/CMakeLists.txt
  src/widgets/jobuidelegate.cpp
  src/widgets/kdesktopfileactions.cpp
  src/widgets/kopenwithdialog.cpp
  src/widgets/kopenwithdialog.h
  src/widgets/krun.cpp
  src/widgets/widgetsopenurljobhandler.cpp
  src/widgets/widgetsopenurljobhandler.h

To: dfaure, ahmadsamir, broulik, meven, kossebau, davidedmundson, nicolasfella, 
svuorela
Cc: feverfew, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29385: Introduce KIO::OpenUrlJob, a rewrite and replacement for KRun

[David Faure]

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

INLINE COMMENTS

> broulik wrote in openurljob.cpp:261
> I know what you always say when I say what I always say, but why not just 
> always stat/mimetype job?

LOL we're like an old couple, the explicit discussion doesn't actually need to 
happen anymore ;)

OK for everyone else, we're debating whether it's ok to use blocking local-file 
I/O like QFile or QMimeDatabase which reads from the file.

Of course less code paths is a good thing for maintenance, but it seems *so* 
overkill to make two calls to a kioslave just to find out the mimetype of a 
file My main counter argument is performance.

For the whole OpenUrlJob until the mimetype is found:

With KIO

  RESULT : OpenUrlJobTest::takeOverAfterMimeTypeFound():
   0.29 msecs per iteration (total: 75, iterations: 256)

With the local-file optimization

  RESULT : OpenUrlJobTest::takeOverAfterMimeTypeFound():
   0.0986 msecs per iteration (total: 101, iterations: 1024)

That's 3 times faster. Admittedly this is not the kind of things people do in a 
loop.

Well, OK, if nobody objects I can remove the local-files fast paths.
0.2ms is nothing when lharming 2018 PG Demi Lovato Ashley Tisdale Avril 
Lavigne-02052020.mpgaunching an app, or even when opening a URL in a browser.

[More context: QMimeDatabase *might* determine the mimetype from just the 
extension, in which case no I/O happens and we could do that here, or it might 
need to look into the contents of the file. We can ask it to not do that but 
then the mimetype determination will be less good, for some mimetypes; and we 
can't ask it if we would get better information by looking at content, so 
there's no way to split up the work between here and the kioslave. It's "quick 
search" vs "full search", not phase 1, phase 2.]

> broulik wrote in openurljob.cpp:447
> was or should be?

Hehe, see kdesktopfileactions.cpp:141 ;-)

> broulik wrote in openurljob.cpp:506
> While at it, can we clean up/unify those texts? Sometimes it puts the file on 
> a new line, sometimes it's bold, sometines in quotes, etc. Generally I 
> wouldn't really want any HTML formatting in there.

Completely agree. I removed most of the HTML formatting when grabbing the error 
messages from KRun, looks like I forgot this one.

So now it's quotes everywhere.

But indeed sometimes the filename is on its own line. Should I just use quotes 
everywhere?

I guess the problem is when the filename (with full path) is very long.

REPOSITORY
  R241 KIO

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

To: dfaure, ahmadsamir, broulik, meven, kossebau, davidedmundson, nicolasfella, 
svuorela
Cc: feverfew, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29385: Introduce KIO::OpenUrlJob, a rewrite and replacement for KRun

[David Faure]

dfaure added a comment.


  In D29385#662439 , @feverfew wrote:
  
  > Ahhh yes correct. Seeming as these jobs are now async (unlike KRun IIRC?), 
it isn't a problem that `resultingArguments` makes several blocking calls to 
the KIOFuse daemon?
  
  
  Some of KRun was async too ("new KRun" was async, the static methods like 
runUrl and others were not).
  The blocking calls in resultingArguments() don't make things worse for 
OpenUrlJob than it was for KRun, but of course the fact that the job is async 
is an opportunity to do everything async.
  The problem is that we can't get rid of KRun just yet
  
  Async on top of sync works -- but still blocks the GUI thread so it's not 
perfect. Sync on top of async requires awful eventloop hacks which are often 
the source of nasty bugs.
  
  Solution 1: we write an async version of DesktopExecParser, keeping the 
existing one for KRun.
  Solution 2: we wait until KRun is gone (KF6) to make DesktopExecParser async.
  
  (I'm skipping non-solutions like event loop hacks in KRun or using the fact 
that nobody seems to really care for the PIDs or "bool success" returned by 
KRun so we could just make KRun "fire and forget" the underlying jobs and 
always return success).
  
  Basically this means rewriting DesktopExecParser as a KJob. Are you up for 
it? ;-)
  I can provide guidance, but I really care about FUSE enough myself to test a 
rewrite if I'd have to write it.
  Or I write, you test ;)

REPOSITORY
  R241 KIO

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

To: dfaure, ahmadsamir, broulik, meven, kossebau, davidedmundson, nicolasfella, 
svuorela
Cc: feverfew, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29385: Introduce KIO::OpenUrlJob, a rewrite and replacement for KRun

[David Faure]

dfaure added a comment.


  In D29385#662422 , @feverfew wrote:
  
  > Quick question, how does this affect D23384 
? Previously KRun used 
KIO::DesktopExecParser::resultingArguments() which handled the conversion of 
URLs to local KIOFuse URLs if needed, but now I believe this new API doesn't?
  
  
  No worries, this should still happen.
  KRun is actually split into three classes: OpenUrlJob, CommandLauncherJob and 
ApplicationLauncherJob. The last two delegate the work to an internal class, 
KProcessRunner (like KRun did).
  So (to roll out the common case of a document-type file), once OpenUrlJob 
finds out which application to start, it calls ApplicationLauncherJob, which 
creates a KProcessRunner, which uses 
KIO::DesktopExecParser::resultingArguments().

REPOSITORY
  R241 KIO

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

To: dfaure, ahmadsamir, broulik, meven, kossebau, davidedmundson, nicolasfella, 
svuorela
Cc: feverfew, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29385: Introduce KIO::OpenUrlJob, a rewrite and replacement for KRun

[David Faure]

dfaure updated this revision to Diff 81814.
dfaure marked 2 inline comments as done.
dfaure added a comment.


  Adjusments based on Friedrich's initial review

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29385?vs=81802=81814

BRANCH
  2020_05_openurljob

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

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/krununittest.cpp
  autotests/openurljobtest.cpp
  autotests/openurljobtest.h
  src/core/kurlauthorized.h
  src/gui/CMakeLists.txt
  src/gui/applicationlauncherjob.cpp
  src/gui/applicationlauncherjob.h
  src/gui/openurljob.cpp
  src/gui/openurljob.h
  src/gui/openurljobhandlerinterface.cpp
  src/gui/openurljobhandlerinterface.h
  src/widgets/CMakeLists.txt
  src/widgets/jobuidelegate.cpp
  src/widgets/kdesktopfileactions.cpp
  src/widgets/kopenwithdialog.cpp
  src/widgets/kopenwithdialog.h
  src/widgets/krun.cpp
  src/widgets/widgetsopenurljobhandler.cpp
  src/widgets/widgetsopenurljobhandler.h

To: dfaure, ahmadsamir, broulik, meven, kossebau, davidedmundson, nicolasfella, 
svuorela
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29385: Introduce KIO::OpenUrlJob, a rewrite and replacement for KRun

[David Faure]

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


  In D29385#662336 , @svuorela wrote:
  
  > I've been looking at it for a while, and after trying to decipher the long 
lambdas, which might have been better as named functions, I think it is, beside 
Kossebau's comments, fine.
  
  
  Interesting comment. I thought it was actually more readable to keep that 
code together, rather than having to jump around in the file to find the right 
slots.

INLINE COMMENTS

> kossebau wrote in openurljobtest.cpp:108
> why no simple range-based for loop?

Good point, copy/pasted from another (old) test.

> kossebau wrote in openurljob.h:73
> Makes me wonder if those runflags should not be rather part of KIO namespace, 
> to decouple classes here.

Interesting point, let's discuss it quickly (because if we want to change that, 
I need to redo the kio RC1 tag for 5.70, which has ApplicationLauncherJob).

But I'm not sure we should do that, anyway.

On hindsight, I would rather change this one to be setTemporaryFiles(bool b);

I don't like all the flag-conversion hell that we end up with otherwise. Stuff 
like

  RunFlags flags = tempFile ? KRun::DeleteTemporaryFiles : RunFlags();
  if (runExecutables) {
  flags |= KRun::RunExecutables;
  }

The input data, all the way up, is for sure not in the form of a set of job 
flags.
So I find it more readable to have code like setSomething(conditionForSomething)
than to have a bunch of flag manipulation code.

> kossebau wrote in openurljob.h:96
> Could this and the following bool flags be turned into some flags instead? 
> Just wondering, not looked further.
> 
> Also wondering if there should not be some getter as well, when using a 
> flagset that would be just a single method/symbol.

With this one in particular, since it's on by default, we'd have to either have 
DefaultRunFlags that includes RunExecutables --- or we'd have to make the flag 
DoNotRunExecutables.
I don't like either

Technically yes it could all be flags, but I'd much rather have independent 
setters.

The TempFile flag is a good example why: if it's shared with 
ApplicationLauncherJob by being in the KIO namespace, does that mean we also 
put all of those in there? But then people can write things that don't make 
sense like ApplicationLauncherJob *job = ...; 
job->setFlags(DoNotRunExecutables). What? That flag doesn't apply to that job. 
If there's one thing that job does, it's to run executables :-)   [but via 
desktop files, while that flag is actually about what OpenUrlJob should do when 
opening an executable directly].

Sharing flags between jobs who need a different set, is a problem. It ends up 
with docu like "only these flags make sense here". Not great.

I don't see a use case for getters, but we can certainly add them (either now 
or as needed).

> kossebau wrote in openurljob.h:127
> -> "mimeTypeFound"? would match other casing.

Well spotted, thanks.

> kossebau wrote in openurljobhandlerinterface.h:33
> Please prepend a line
> 
>   * @class OpenUrlJobHandlerInterface openurljobhandlerinterface.h 
> 
> 
> at the top, to trick doxygen into documenting the full CamelCase include.

Ah! I was wondering what that was about...

> kossebau wrote in openurljobhandlerinterface.h:84
> Prevent use of nested Private class here, declare a class 
> OpenUrlJobHandlerInterfacePrivate outside, also use QScopedPointer for 
> consistency and preparedness in case there ever is an actual object.

LOL I was the one arguing against nested Private classes recently. Looks like I 
forgot that when copy/pasting here. Thanks!

Hmm note that using QScopedPointer requires actually defining (in the .cpp) a 
dummy private class, even if there's no "new" anywhere (generated code wants to 
delete it).

REPOSITORY
  R241 KIO

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

To: dfaure, ahmadsamir, broulik, meven, kossebau, davidedmundson, nicolasfella, 
svuorela
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

Re: Adding a patch to 5.70

[David Faure]

On Sunday, May 3, 2020 6:31:44 PM CEST Filip Fila wrote:
> Dear Frameworks maintainers,
> 
> Would it be possible to add this (https://phabricator.kde.org/D29352) patch
> to the 5.70 release?
> 
> The change concerns not breaking third-party Plasma themes, and as Nate
> explained "If this doesn't land in Frameworks 5.70, Plasma 5.19 users run
> the risk of being hit by it, as 5.70 is the frameworks dependency for that
> Plasma release."

OK, done:

plasma-framework v5.70.0-rc2
fe45d59e250d1c7f4579f54fec52437ebb0e5d03
cb8289d495e4df19056ce8814dacd8a0afe93bff1edb0352028c6b6e47364ba5  
sources/plasma-framework-5.70.0.tar.xz


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

D29385: Introduce KIO::OpenUrlJob, a rewrite and replacement for KRun

[David Faure]

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

REVISION SUMMARY
  - KRun::runUrl() is now new OpenUrlJob(url, mimeType).
  - new KRun() is now OpenUrlJob(url), which will first determine the mimeType.
  - the "open with" dialog is provided by KIOWidgets via JobUiDelegate and an 
interface defined in kiogui. When not using JobUiDelegate, we'll fall back to 
QDesktopServices::openUrl, which will call xdg-open, which will call kde-open5, 
which will prompt an open with dialog :-)
  - Running of scripts and executables is off by default, unlike in KRun, since 
this created a few security bugs in unsuspecting applications in the past. File 
managers should enable it, apps that can open attachments or similar things 
should leave it disabled.
  - Instead of BrowserRun/KonqRun deriving from KRun, they can use an 
OpenUrlJob as is, connect to mimeTypeFound and kill the job if they want to 
take over (to embed the document instead of launching an app)
  
  I tried to unittest OpenUrlJob as much as possible. The parts that are
  least unittests are those related to remote protocols though, since they
  require test servers.

TEST PLAN
  Mostly via unittests, for now

REPOSITORY
  R241 KIO

BRANCH
  2020_05_openurljob

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

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/krununittest.cpp
  autotests/openurljobtest.cpp
  autotests/openurljobtest.h
  src/core/kurlauthorized.h
  src/gui/CMakeLists.txt
  src/gui/applicationlauncherjob.cpp
  src/gui/applicationlauncherjob.h
  src/gui/openurljob.cpp
  src/gui/openurljob.h
  src/gui/openurljobhandlerinterface.cpp
  src/gui/openurljobhandlerinterface.h
  src/widgets/CMakeLists.txt
  src/widgets/jobuidelegate.cpp
  src/widgets/kdesktopfileactions.cpp
  src/widgets/kopenwithdialog.cpp
  src/widgets/kopenwithdialog.h
  src/widgets/krun.cpp
  src/widgets/widgetsopenurljobhandler.cpp
  src/widgets/widgetsopenurljobhandler.h

To: dfaure, ahmadsamir, broulik, meven, kossebau, davidedmundson, nicolasfella, 
svuorela
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29347: KAuthorized: export method to reload restrictions

[David Faure]

dfaure added inline comments.

INLINE COMMENTS

> kossebau wrote in kauthorized.cpp:247
> Please also add a note next to this that it is for unit test, so people are 
> not wondering about this random KCONFIGCORE_EXPORT and first have to research 
> commit history.

Good idea, done: 
https://commits.kde.org/kconfig/311e30857e33f9222b28b6d3b841ab6e61558237

REPOSITORY
  R237 KConfig

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

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

D29347: KAuthorized: export method to reload restrictions

[David Faure]

dfaure closed this revision.

REPOSITORY
  R237 KConfig

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

To: dfaure, aacid, apol, mdawson
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29371: KMainWindow: remove doc paragraph about KGlobal::ref usage

[David Faure]

dfaure added a comment.


  `src/kmainwindow_p.h:59:QEventLoopLocker locker;` so the refcounting 
effect is the same as documented, just with a different "backend".

REPOSITORY
  R263 KXmlGui

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

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

D29371: KMainWindow: remove doc paragraph about KGlobal::ref usage

[David Faure]

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


  Please don't remove this, but port it to QEventLoopLocker.

REPOSITORY
  R263 KXmlGui

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

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

D29347: KAuthorized: export method to reload restrictions

[David Faure]

dfaure added a comment.


  Yes declaring the function there, as in the code sample shown here.
  
  We do the same for internally-exported variables like KSERVICE_EXPORT int 
ksycoca_ms_between_checks;
  Qt does the same kind of things.
  
  A _p.h header would have to be installed, which we don't normally do.

REPOSITORY
  R237 KConfig

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

To: dfaure, aacid, apol, mdawson
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29347: KAuthorized: export method to reload restrictions

[David Faure]

dfaure edited the summary of this revision.

REPOSITORY
  R237 KConfig

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

To: dfaure, aacid, apol, mdawson
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29210: [NestedListHelper] Fix indentation of selection, add tests

[David Faure]

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

BRANCH
  change-indent (branched from master)

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

To: poboiko, #frameworks, dfaure, mlaurent
Cc: kde-frameworks-devel

D29361: KCrash: remove debug output which breaks unittests from using ~/.qttest/config for categorized logging

[David Faure]

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

REVISION SUMMARY
  kdebugsettings --test-mode allows to tune the categorized logging for 
unittests,
  but if a very early qCDebug happens, QLoggingRegistry::initializeRules is 
called
  before the unittest gets a chance to call 
QStandardPaths::setTestModeEnabled(true).
  
  This is obviously a bit fragile overall; should all unittests do that in a 
constructor function? A bit ugly...

REPOSITORY
  R285 KCrash

BRANCH
  master

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

AFFECTED FILES
  src/kcrash.cpp

To: dfaure, kossebau, davidedmundson
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29208: [NestedListHelper] Improve indentation code

[David Faure]

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

REPOSITORY
  R310 KTextWidgets

BRANCH
  unused (branched from master)

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

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

D29208: [NestedListHelper] Improve indentation code

[David Faure]

dfaure added inline comments.

INLINE COMMENTS

> poboiko wrote in nestedlisthelper.cpp:87
> That's the current block being checked, not the next one. I've just checked 
> to be sure, last block can be unindented :)
> 
> TBH, I don't really know if it's even possible for the current block to be 
> invalid (that would probably mean that `textCursor()` returned by `QTextEdit` 
> is invalid?). 
> I've just borrowed this particular check from the old code...

Oh. I see. Well, it would be much more readable to move that if() *before* the 
line that declares and sets nextBlock.

And yes I can't really see how this can happen, either.

> nestedlisthelper.cpp:93
>  }
> -
> -QTextCursor cursor = textEdit->textCursor();
> -bool handled = false;
> -
> -if (!cursor.hasSelection() && cursor.currentList()) {
> -
> -// Check if we're on the last list item.
> -// itemNumber is zero indexed
> -QTextBlock currentBlock = cursor.block();
> -if (cursor.currentList()->count() == 
> cursor.currentList()->itemNumber(currentBlock) + 1) {
> -// Last block in this list, but may have just gained another 
> list below.
> -if (currentBlock.next().textList()) {
> -reformatList();
> -}
> -
> -// No need to reformatList in this case. reformatList is slow.
> -if ((event->key() == Qt::Key_Return) || (event->key() == 
> Qt::Key_Backspace)) {
> -handled = true;
> -}
> -} else {
> -reformatList();
> -}
> +if (!nextBlock.textList()) {
> +return true;

nextBlock is only used here so I would move its definition to just before this 
line.

(same thing in the previous method, about prevBlock)

REPOSITORY
  R310 KTextWidgets

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

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

D29347: KAuthorized: export method to reload restrictions

[David Faure]

dfaure created this revision.
dfaure added reviewers: aacid, apol, mdawson.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
dfaure requested review of this revision.

REVISION SUMMARY
  This is useful for unittests. Example:
  
  KCONFIGCORE_EXPORT void reloadUrlActionRestrictions();
  
  void someTestMethod()
  {
  
KConfigGroup cg(KSharedConfig::openConfig(), "KDE URL Restrictions");
cg.writeEntry("rule_count", 1);
cg.writeEntry("rule_1", QStringList{"open", {}, {}, {}, "file", "", "", 
"false"});
cg.sync();
reloadUrlActionRestrictions();

// Some test for code that uses KUrlAuthorized

cg.deleteEntry("rule_1");
cg.deleteEntry("rule_count");
cg.sync();
reloadUrlActionRestrictions();
  
  }
  
  This is consistent with the fact that other functions used by
  KUrlAuthorized are defined here and exported internally.

TEST PLAN
  Used this in a KIO unittest I'm writing for the future OpenUrlJob

REPOSITORY
  R237 KConfig

BRANCH
  master

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

AFFECTED FILES
  src/core/kauthorized.cpp

To: dfaure, aacid, apol, mdawson
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29096: Prefix includes and libs dir with QT_SYSROOT

[David Faure]

dfaure added a comment.


  I'm using conan, not doing cross compilation.
  
  But with relative paths it's no problem, both use cases work.

REPOSITORY
  R240 Extra CMake Modules

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

To: ablu, #build_system, apol, vkrause, kfunk
Cc: dfaure, kossebau, kde-frameworks-devel, kde-buildsystem, LeGast00n, cblack, 
bencreasy, michaelh, ngraham, bruns

D29208: [NestedListHelper] Improve indentation code

[David Faure]

dfaure added inline comments.

INLINE COMMENTS

> nestedlisthelper.cpp:87
> +QTextBlock nextBlock = block.next();
> +if (!block.isValid()) {
> +return false;

So the last block cannot be unindented? How come?

REPOSITORY
  R310 KTextWidgets

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

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

Re: Having $(framework)_global.h headers (was: Re: building KF5 projects against a different Qt5 version [...])

[David Faure]

On lundi 27 avril 2020 14:28:42 CEST Friedrich W. H. Kossebau wrote:
> Am Sonntag, 26. April 2020, 16:12:31 CEST schrieb Friedrich W. H. Kossebau:
> > Am Sonntag, 26. April 2020, 15:46:35 CEST schrieb David Faure:
> > > On Sunday, April 26, 2020 3:21:34 PM CEST René J.V. Bertin wrote:
> > > > Talking about version test hacks (or not-so-hacks): why is it that
> > > > KFOO_VERSION isn't defined systematically when you include any header
> > > > of
> > > > the FOO KF5 framework? With Qt you never have to worry about
> > > > QT_VERSION
> > > > being defined.
> > > 
> > > Well, everything in Qt ends up including global.h
> > > There's no such central header in KF5 frameworks (which are more
> > > modular,
> > > by definition), so you need to include the framework_version.h header.
> > 
> > We could perhaps look into extending the export headers into some kind of
> > $
> > (framework)_global.h headers, like we are now doing already a bit for all
> > the deprecation definitions when using ecm_generate_export_header, where
> > all additional stuff is injected via the export headers.
> > 
> > I also found a bit annoying to have to add all the explicit includes of
> > framework_version.h every time one wanted to access KFOO_VERSION.
> > 
> > So far had only pondered the idea at that time, no yet investigated
> > further
> > given state of own TODO list :) Also low motivation given that there would
> > need to be a backward-compatible setup, and usually one uses that very
> > version header to be compatible to old versions, so there was no immediate
> > gain. But long time (at least KF6) I would also prefer to see the version
> > macros available by default.
> 
> And I meanwhile noticed that the actual version number of a library is
> already present in the export headers generated with
> ECMGenerateExportHeader, due to being a possible default number for all the
> version-dependent calculations for what to show warnings or which API to
> hide. Not accessible as macro though, so do not have hopes to "abuse" this
> :)
> 
> Given this kept following me into my shower thoughts this morning, guess I
> will soonish be working on some ECMGenerateGlobalHeader then, or at least
> explore more for now in some non-ECM projects the feasibility of semi-
> generation of such global header carrying all the stuff interesting with all
> the (public) API of a library. Ping me again in autumn about this.

I know it's not autumn yet, but isn't this what COMMON_HEADER in 
ecm_generate_headers already does, actually?

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

D29096: Prefix includes and libs dir with QT_SYSROOT

[David Faure]

dfaure added a comment.


  Thanks for the quick test!
  
  I'll indeed proceed with the other one, which is more generic (our use case 
isn't related to QT_SYSROOT).
  
  Actually: can you add your comment (that it works for you) in the other 
review request, so people know it's been tested by more than me, and discard 
this one?

REPOSITORY
  R240 Extra CMake Modules

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

To: ablu, #build_system, apol, vkrause, kfunk
Cc: dfaure, kossebau, kde-frameworks-devel, kde-buildsystem, LeGast00n, cblack, 
bencreasy, michaelh, ngraham, bruns

D29274: ECMGeneratePriFile: make the pri files relocatable

[David Faure]

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


  Fix error: regex "[^/]*" matched an empty string.

REPOSITORY
  R240 Extra CMake Modules

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29274?vs=81506=81519

BRANCH
  master

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

AFFECTED FILES
  modules/ECMGeneratePriFile.cmake

To: dfaure, vatra, kfunk, apol
Cc: ablu, kossebau, kde-frameworks-devel, kde-buildsystem, LeGast00n, cblack, 
bencreasy, michaelh, ngraham, bruns

D29096: Prefix includes and libs dir with QT_SYSROOT

[David Faure]

dfaure added a comment.


  Hi @ablu,
  
  Could you test if my patch in D29274  
solves your problem? By making these lines relative to the location of the .pri 
file, it should work very well in a sysroot context as well.

REPOSITORY
  R240 Extra CMake Modules

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

To: ablu, #build_system, apol, vkrause, kfunk
Cc: dfaure, kossebau, kde-frameworks-devel, kde-buildsystem, LeGast00n, cblack, 
bencreasy, michaelh, ngraham, bruns

D29274: ECMGeneratePriFile: make the pri files relocatable

[David Faure]

dfaure created this revision.
dfaure added reviewers: vatra, kfunk, apol.
Herald added projects: Frameworks, Build System.
Herald added subscribers: kde-buildsystem, kde-frameworks-devel.
dfaure requested review of this revision.

REVISION SUMMARY
  Instead of generating
  QT.KArchive.includes = /full/path/include/KF5/KArchive
  make it
  QT.KArchive.includes = $$PWD/../../include/KF5/KArchive
  
  This makes the whole install prefix relocatable after the fact,
  the includes will be found based on where the .pri file ended up.
  
  This is especially useful for Conan support, says Bogdan.

TEST PLAN
  After make install in ECM, cd karchive/examples/helloworld && qmake && make

REPOSITORY
  R240 Extra CMake Modules

BRANCH
  master

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

AFFECTED FILES
  modules/ECMGeneratePriFile.cmake

To: dfaure, vatra, kfunk, apol
Cc: kde-frameworks-devel, kde-buildsystem, LeGast00n, cblack, bencreasy, 
michaelh, ngraham, bruns

D29251: API dox: use ulong typedef with Q_PROPERTY(percent) to avoid doxygen bug

[David Faure]

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


  Yes indeed, for sure "int" would be enough.

REPOSITORY
  R244 KCoreAddons

BRANCH
  fixkjobpercentdocs

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

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

D29153: Move handling of untrusted programs to ApplicationLauncherJob.

[David Faure]

dfaure added inline comments.

INLINE COMMENTS

> broulik wrote in applicationlauncherjob.cpp:155
> With this it starts to look as hard to follow as KRun :)

Not even close :-)

(OpenUrlJob will be more complicated, somewhere in between this one and KRun...)

REPOSITORY
  R241 KIO

BRANCH
  2020_04_UntrustedProgramHandler

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

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

D29153: Move handling of untrusted programs to ApplicationLauncherJob.

[David Faure]

This revision was automatically updated to reflect the committed changes.
Closed by commit R241:f0ae038490e6: Move handling of untrusted programs to 
ApplicationLauncherJob. (authored by dfaure).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29153?vs=81173=81472

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

AFFECTED FILES
  autotests/applicationlauncherjobtest.cpp
  autotests/applicationlauncherjobtest.h
  src/core/CMakeLists.txt
  src/core/jobuidelegateextension.h
  src/core/untrustedprogramhandlerinterface.cpp
  src/core/untrustedprogramhandlerinterface.h
  src/gui/applicationlauncherjob.cpp
  src/gui/applicationlauncherjob.h
  src/widgets/CMakeLists.txt
  src/widgets/jobuidelegate.cpp
  src/widgets/krun.cpp
  src/widgets/krun_p.h
  src/widgets/widgetsuntrustedprogramhandler.cpp
  src/widgets/widgetsuntrustedprogramhandler.h
  tests/kruntest.cpp

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

D29153: Move handling of untrusted programs to ApplicationLauncherJob.

[David Faure]

dfaure added a comment.


  @broulik ping?

REPOSITORY
  R241 KIO

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

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

D29206: Move check for invalid service from KDesktopFileActions to ApplicationLauncherJob

[David Faure]

dfaure closed this revision.

REPOSITORY
  R241 KIO

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

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

D29170: Detect executables without +x permission in $PATH to improve error message

[David Faure]

dfaure closed this revision.

REPOSITORY
  R241 KIO

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

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

D29239: [KPlotting] foreach is gone, build with -DQT_NO_FOREACH

[David Faure]

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

REPOSITORY
  R277 KPlotting

BRANCH
  l-no-foreach (branched from master)

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

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

D29170: Detect executables without +x permission in $PATH to improve error message

[David Faure]

dfaure added a comment.


  In D29170#658605 , @ngraham wrote:
  
  > What does this mean for AppImages?
  
  
  They are not desktop files, they don't come into this code path (which takes 
a KService as input).
  
  Clicking on a +x AppImage asks for confirmation and runs it.

REPOSITORY
  R241 KIO

BRANCH
  2020_04_findExecutable

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

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

D29233: [Solid] Port foreach to range/index for

[David Faure]

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

REPOSITORY
  R245 Solid

BRANCH
  l-foreach-3 (branched from master)

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

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

D29229: [KPlotting] Port foreach (deprecated) to range for

[David Faure]

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

REPOSITORY
  R277 KPlotting

BRANCH
  l-foreach (branched from master)

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

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

D29221: [FakeCdrom] Add a new UnknownMediumType enumerator to MediumType

[David Faure]

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

REPOSITORY
  R245 Solid

BRANCH
  l-mediumtypes (branched from master)

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

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

D29170: Detect executables without +x permission in $PATH to improve error message

[David Faure]

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


  Remove unused QFileInfo include

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29170?vs=81376=81377

BRANCH
  2020_04_findExecutable

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

AFFECTED FILES
  autotests/applicationlauncherjobtest.cpp
  src/core/desktopexecparser.cpp
  src/gui/kprocessrunner.cpp

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

D29216: Remove dead code since KF5.0: mount/umount devices in their contextmenu

[David Faure]

dfaure closed this revision.

REPOSITORY
  R241 KIO

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

To: dfaure, apol, broulik
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29170: Detect executables without +x permission in $PATH to improve error message

[David Faure]

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


  Remove QDir::current

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29170?vs=81275=81376

BRANCH
  2020_04_findExecutable

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

AFFECTED FILES
  autotests/applicationlauncherjobtest.cpp
  src/core/desktopexecparser.cpp
  src/gui/kprocessrunner.cpp

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

D29170: Detect executables without +x permission in $PATH to improve error message

[David Faure]

dfaure added inline comments.

INLINE COMMENTS

> ahmadsamir wrote in kprocessrunner.cpp:65
> I guess there is no need to use QDir::current() any more.

Good point, removing.

REPOSITORY
  R241 KIO

BRANCH
  2020_04_findExecutable

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

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

D29206: Move check for invalid service from KDesktopFileActions to ApplicationLauncherJob

[David Faure]

dfaure added inline comments.

INLINE COMMENTS

> ahmadsamir wrote in kprocessrunner.cpp:55
> IMHO, the word "entry" here is confusing, the first thing that comes to mind 
> is that an entry (i.e. a line) in the file is invalid.

I see what you're saying.

It's the name of the spec, though.
https://specifications.freedesktop.org/desktop-entry-spec/latest/

Each file is an "entry for the desktop", I guess.

REPOSITORY
  R241 KIO

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

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

D29170: Detect executables without +x permission in $PATH to improve error message

[David Faure]

dfaure added a comment.


  In D29170#658058 , @meven wrote:
  
  > Is it not sufficient to fix bug 415567 ? In which case replace in commit 
comment CCBUG by BUG
  
  
  No, that bug has two issues. "Program not found" and "Missing lib" -- which 
is another issue, to be solved in a future commit.

REPOSITORY
  R241 KIO

BRANCH
  2020_04_findExecutable

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

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

D29219: [KFontChooser] Remove NoFixedCheckBox DisplayFlag, redundant

[David Faure]

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

REPOSITORY
  R236 KWidgetsAddons

BRANCH
  l-remove-redundant-displayflag (branched from master)

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

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

D29210: [NestedListHelper] Fix indentation of selection, add tests

[David Faure]

dfaure added inline comments.

INLINE COMMENTS

> nestedlisthelper.cpp:225
> +if (delta > 0) {
> +// No list, we're increasing indentation -> create a new one
> +listFmt.setStyle(QTextListFormat::ListDisc);

should the opposite happen? delete the list when decreasing indentation?

Or is that what the blkFmt.setObjectIndex(-1); is about?
A comment there would be useful...

To me setObjectIndex(-1) sounds like it could kill any kind of embedded object 
there but I might be wrong.

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

To: poboiko, #frameworks, dfaure, mlaurent
Cc: kde-frameworks-devel

Re: KF 5 & C++14?

[David Faure]

On Sunday, April 26, 2020 5:30:37 PM CEST Friedrich W. H. Kossebau wrote:
> Hi,
> 
> I just saw that at least kimageformats, knewstuff & kquickcharts all set
> this: set(CMAKE_CXX_STANDARD 14)
> set(CMAKE_CXX_STANDARD_REQUIRED ON)
> 
> Which ignores a bit that so far C++11 has been the minimum standard
> officially supported in/by KDE Frameworks (by mainly following what Qt 5
> supports).
> 
> Compare also the current text of the policy
> "Frameworks compiler requirements and C++11":
> --- 8< ---
> The following minimal compiler versions are supported by KDE Frameworks:
> * GCC 4.8
> * Clang 3.3
> * VS2013 (MSVC12)
> 
> This means all of the C++11 standards can be used.
> --- 8< ---
> * https://community.kde.org/Frameworks/
> Policies#Frameworks_compiler_requirements_and_C.2B.2B11
> 
> What to make of this? Might it be the time to raise the bars a bit, and how
> much? Surely the lower limit is what the oldest Qt version currently
> supported by KDE Frameworks claims to support:
> https://doc.qt.io/qt-5.12/supported-platforms.html
> 
> Should we go above this?
> Or should kimageformats, knewstuff & kquickcharts be fixed to use C++11 only
> again?
> 
> No own opinion, so far only stumbled over what seems a policy breakage.

The goal was to align with the requirements of the Qt version we require.

But it's hard to know if anyone is actually using gcc 4.8 to build the current 
version of KF5. One way to find out is to... do nothing. Leave the above as is 
and see if anyone actually complains that it doesn't match our promise.

This isn't however a green light for doing this everywhere, not until we wait 
multiple months for feedback. Otherwise the porting effort (down to C++11) 
will be huge.

My two cents,

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

D29216: Remove dead code since KF5.0: mount/umount devices in their contextmenu

[David Faure]

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

REVISION SUMMARY
  This code was disabled in preparation for KF 5.0 to remove kio's dependency 
on solid.
  This is ironic because later on kio_trash brought that dependency again. But 
anyway, this
  feature was still dead code for all of KF5 life time.
  
  The idea of going to /dev and right-click mount/unmount just seems very weird 
as a GUI feature.
  
  (Ignore the comment in the CMakeLists.txt, someone misunderstood what that 
code was about.
  No desktop files involved there, this was about block devices themselves)

TEST PLAN
  It builds ;)

REPOSITORY
  R241 KIO

BRANCH
  2020_04_remove_dead_solid_code

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

AFFECTED FILES
  src/widgets/CMakeLists.txt
  src/widgets/config-kiowidgets.h.cmake
  src/widgets/kdesktopfileactions.cpp
  src/widgets/kfileitemactions.cpp

To: dfaure, apol, broulik
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29206: Move check for invalid service from KDesktopFileActions to ApplicationLauncherJob

[David Faure]

dfaure updated this revision to Diff 81281.
dfaure retitled this revision from "Move check for invalid service from 
KDesktopFileActions to ApplicationLauncherJobTest" to "Move check for invalid 
service from KDesktopFileActions to ApplicationLauncherJob".
dfaure added a comment.


  fix wrong "Test" in commit log

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29206?vs=81250=81281

BRANCH
  2020_04_invalid_service

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

AFFECTED FILES
  autotests/applicationlauncherjobtest.cpp
  autotests/applicationlauncherjobtest.h
  src/gui/kprocessrunner.cpp
  src/widgets/kdesktopfileactions.cpp

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

D28760: KSettings::Dialog: avoid duplicate entries due cascading $XDG_DATA_DIRS

[David Faure]

dfaure added a reviewer: svuorela.

REPOSITORY
  R295 KCMUtils

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

To: dfaure, apol, broulik, davidedmundson, kossebau, svuorela
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29170: Detect executables without +x permission in $PATH to improve error message

[David Faure]

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


  Remove support for relative executables, as discussed

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29170?vs=81228=81275

BRANCH
  2020_04_findExecutable

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

AFFECTED FILES
  autotests/applicationlauncherjobtest.cpp
  src/core/desktopexecparser.cpp
  src/gui/kprocessrunner.cpp

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

Re: building KF5 projects against a different Qt5 version (than the one the KF5 frameworks were built against)

[David Faure]

On Sunday, April 26, 2020 5:50:40 PM CEST René J.V. Bertin wrote:
> On Sunday April 26 2020 16:58:10 David Faure wrote:
> >> As a side-note, I'd even argue that
> >> it would make sense to provide an all-inclusive header per framework,
> >> just
> >> like Apple's frameworks do.
> >
> >It's said to improve compilation times with precompiled headers, but in
> >practice nobody knows if the person compiling the code has that, so I fail
> >to see the point.
> 
> The possibility to use precompiled headers is only an argument here when
> increased compilation times are brought up as an argument against such a
> header. But that's not really an argument if you can continue to use just
> the headers you need. 

Well, then you never need all-inclusive headers.

> >Who needs all of ? Nobody.
> 
> I could just as well maintain that nobody needs any of that particular
> framework ;) 

OK now you make no sense at all anymore, since I can give you 500 proofs of 
the contrary: actual pieces of code that use some parts of KCoreAddons, and 
none of them which use all of it.

> But the question isn't who needs all of. People including
>  or (better example)  also rarely
> need all of those frameworks. It's just a convenience: no need to jump back
> and forth between your editing location and the file preamble (after
> deciding if you want the required additional header in the implementation
> file or in the corresponding header file), and no need to maintain a long
> list of include statements which ideally you should prune every time you
> stop using a certain class. (Just how often do people forget to remove the
> corresponding header?)

And then people commit code that uses "everything", slowing down compilation 
times. We don't accept it in KF5 code (nor in most of KDE apps/workspace 
AFAIK), I don't see why we should provide something that we forbid.

> I still call me seeing the warning a pure chance given the fact it's there.
> The fact of me seeing it if it were not there would also be a pure chance
> ... hopefully close enough to 0 ;)

-Werror=undef is your friend.

Actually I'll add it to my default set of flags, to see how much breaks ;-)

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

D29170: Detect executables without +x permission in $PATH to improve error message

[David Faure]

dfaure added a comment.


  In D29170#657450 , @ahmadsamir 
wrote:
  
  > I don't think you need to go out of your way to support custom setups, 
after all it's quite simple for the user to edit the .desktop file and specify 
the path to the executable. Even from the xdg thread, they were talking about 
installing a virtual machine guest stuff, which is something the user only has 
to do once, whereas, from my POV at least, a .desktop file is more for things 
your run frequently/on a regular basis.
  
  
  One use case could be running software on a USB key. The desktop file there 
doesn't know the full path, but a relative one.
  
  > It would simplify the code, and would be consistent with how a shell would 
run a program; indeed a binary in the current dir has to be explicitly prefixed 
with  ./
  
  I'm not sure if now you're advocating to support "./foo" or no support for 
relative executables at all.
  
  > That also means that the original code trying to find the executable in the 
current dir never really worked
  
  You keep reading it that way, and I keep saying that code was assuming 
"realExecutable" is an absolute path. As my (new) comment in the code says, it 
actually is one, if the program was found (and is executable).
  What happened with not-found-therefore-still-relative executable names in 
there was purely accidental (and fixed by this commit).
  
  > because "current dir" is most likely where the _KIO executable_ exists (I 
tested with qt-creator and QDir::current() is indeed where the compiled binary 
exists). So not that useful.
  
  Worse, if you start "dolphin /tmp" from your $HOME (using konsole), then 
QDir::curren() is $HOME, completely unrelated to what dolphin is showing.
  
  I got no reply on k-f-d but Albert Astals Cid on IRC was of the opinion of 
"stick to the spec, don't support relative executables in any way", which is a 
fair point.
  If you agree too I can just revert to not supporting relative paths at all. 
It all came from what I thought was a request from you in the first review, and 
that old request on xdg.

REPOSITORY
  R241 KIO

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

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

Re: building KF5 projects against a different Qt5 version (than the one the KF5 frameworks were built against)

[David Faure]

On Sunday, April 26, 2020 4:32:46 PM CEST René J.V. Bertin wrote:
> On Sunday April 26 2020 15:46:35 David Faure wrote:
> >Well, yeah, you can't have it all.
> 
> I suppose that the appropriate ECM could provide a function that returns the
> path to the "official" Qt headers (or an expression that evaluates to that
> path) unless something like QT_HEADER_PATH is defined. Idem for linker
> paths. Of course then individual frameworks would still accept to use those
> functions.

None of that would work with imported targets.

> >Or, you can, but you need two full builds.
> 
> Yeah, I don't think that what I was trying is important enough to warrant
> the investment that comes with having another build...

Your choice.

> >There's no such central header in KF5 frameworks (which are more modular,
> >by definition), so you need to include the framework_version.h header.
>
> Don't you think it'd make sense to expect the version to be defined if you
> include the KFoo header if framework Foo provides one (I count 17 that do,
> among the 60+ frameworks I installed)? 

No. There's no "main header" concept. As you say, this would only work for 
17/60 frameworks anyway, not really a good solution.

> As a side-note, I'd even argue that
> it would make sense to provide an all-inclusive header per framework, just
> like Apple's frameworks do.

It's said to improve compilation times with precompiled headers, but in 
practice nobody knows if the person compiling the code has that, so I fail to 
see the point.

Who needs all of ? Nobody.

> >We have -Wundef by default.
> 
> Yes, in the compiler settings, which is why I caught the FOO_VERSION not
> defined thing purely by chance rather than because of unexpected behaviour
> at runtime. 

I wouldn't call this "purely by chance". I set -Wundef there for a reason :-)

> KDevelop doesn't include the flag in its options for the
> clang-based parser.

OK, talk to the KDevelop people then :-)

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

D29206: Move check for invalid service from KDesktopFileActions to ApplicationLauncherJobTest

[David Faure]

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


  remove redundant line

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29206?vs=81249=81250

BRANCH
  2020_04_invalid_service

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

AFFECTED FILES
  autotests/applicationlauncherjobtest.cpp
  autotests/applicationlauncherjobtest.h
  src/gui/kprocessrunner.cpp
  src/widgets/kdesktopfileactions.cpp

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

D29206: Move check for invalid service from KDesktopFileActions to ApplicationLauncherJobTest

[David Faure]

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

TEST PLAN
  New unittest passes

REPOSITORY
  R241 KIO

BRANCH
  2020_04_invalid_service

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

AFFECTED FILES
  autotests/applicationlauncherjobtest.cpp
  autotests/applicationlauncherjobtest.h
  src/gui/kprocessrunner.cpp
  src/widgets/kdesktopfileactions.cpp

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

D28264: KIO: rename ProcessLauncherJob to ApplicationLauncherJob

[David Faure]

dfaure added a task: T11549: KIO: remove unneeded QWidget dependencies to set 
parent windows or display errors.

REPOSITORY
  R241 KIO

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

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

D28020: New class ProcessLauncherJob in KIOGui

[David Faure]

dfaure added a task: T11549: KIO: remove unneeded QWidget dependencies to set 
parent windows or display errors.

REPOSITORY
  R241 KIO

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

To: dfaure, apol, davidedmundson, nicolasfella, vkrause, broulik
Cc: jbbgameich, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, 
bruns

D29153: Move handling of untrusted programs to ApplicationLauncherJob.

[David Faure]

dfaure added a task: T11549: KIO: remove unneeded QWidget dependencies to set 
parent windows or display errors.

REPOSITORY
  R241 KIO

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

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

Re: building KF5 projects against a different Qt5 version (than the one the KF5 frameworks were built against)

[David Faure]

On Sunday, April 26, 2020 3:21:34 PM CEST René J.V. Bertin wrote:
> On Sunday April 26 2020 14:12:01 David Faure wrote:
> >> (and possibly LD_PRELOAD).
> >
> >I don't see why you would set LD_PRELOAD in this configuration.
> 
> This may be necessary if even one of the multiple libraries that get loaded
> has an old-style link to a Qt library which makes it ignore
> LD_LIBRARY_PATH. I've seen that with libQt5QtCore (and also with libdbus).

This should only ever happen if RPATH is used instead of RUNPATH.
I haven't seen this in 15 years or so.

> >I think this is because the KF5 target brings its own include paths as part
> >of the imported target.
> 
> Probably, yes. And that's enough to stop the build I was attempting, so I
> cannot affirm nor infirm if something similar won't happen with linker
> paths.

Right. Those are most likely full paths too.

> >Build against the old Qt, run against the new Qt?
> >Then it's back to "only" LD_LIBRARY_PATH switcheroo which works well, while
> >at the buildsystem level it's all based upon the old Qt so no mixup there.
>
> Yeah, except that then you can't test those #if QT_VERSION hacks...

Well, yeah, you can't have it all.
Or, you can, but you need two full builds.
 
> Talking about version test hacks (or not-so-hacks): why is it that
> KFOO_VERSION isn't defined systematically when you include any header of
> the FOO KF5 framework? With Qt you never have to worry about QT_VERSION
> being defined. 

Well, everything in Qt ends up including global.h
There's no such central header in KF5 frameworks (which are more modular, by 
definition), so you need to include the framework_version.h header.

> This would be less of an issue if -Wundef was part of -Wall
> (or if IDE parsers always flagged undefined macros used in a preprocessor
> expression that isn't a simple #ifdef (or #if defined)...

We have -Wundef by default.

extra-cmake-modules/kde-modules/KDECompilerSettings.cmake:
367:set(_KDE_GCC_COMMON_WARNING_FLAGS "-Wall -Wextra -Wcast-align -Wchar-
subscripts -Wformat-security -Wno-long-long -Wpointer-arith -Wundef")

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

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

[David Faure]

dfaure added a comment.


  Ah! Now it actually makes sense to me. If we are changing what 
revertToDefault() does, then it makes sense to change the if() condition for 
calling it. Basically, now that it does the right thing in both cases (default 
from C++ and default from system file) it can be called in both cases, while 
before it was only called if there was no default from a system file. Right?
  
  I'm still wondering about this though: "If we're saving a value equal to the 
C++ default, then we have to write it out, otherwise upon reading the 
global-file default will interfer."
  Your unittest shows that revertToDefault() will lead to the global-file value 
being read.
  Doesn't this mean that this code is wrong then?
  
 if (value == "cppdefault") {
cg.revertToDefault(key);
} else {
cg.writeEntry(key, value);
}
  
  That's the code you're using everywhere, so that's actually the code that 
would be good to unittest ;-)
  Isn't this buggy when the value actually *is* cppdefault, and there is a 
system config file with another default value?
  
  > Indeed probably need to update configuration too.
  
  Did you mean documentation? I know this is all about configuration ;) but the 
bit that's missing is to update the documentation, and add a task to get rid of 
the hasDefault() everywhere before revertToDefault is called. Assuming I'm 
wrong above about there being a bug left :-)

INLINE COMMENTS

> kconfigtest.cpp:1991
> +KConfigGroup generalLocal(, "General");
> +// Check if we get global and not the default value from cpp 
> (defaultcpp) when reading data from restorerc
> +QCOMPARE(generalLocal.readEntry("testKeyDefault", "defaultcpp"), 
> "configdefault");

Bug in the comment, the file isn't called restorerc.

> kconfigtest.cpp:1998
> +local.reparseConfiguration();
> +// We expect to get configdefault value and configdefault not wrote to 
> local file (file will not exist)
> +QCOMPARE(generalLocal.readEntry("testKeyDefault", "defaultcpp"), 
> "configdefault");

s/wrote/written/

> kconfigtest.cpp:2002
> +
> +// Write a custom value, we expect this value to be wrote to local file
> +generalLocal.writeEntry("testKeyDefault", "custom");

s/wrote/written/

REPOSITORY
  R237 KConfig

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

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

D29170: Detect executables without +x permission in $PATH to improve error message

[David Faure]

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


  add missing braces

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29170?vs=81213=81228

BRANCH
  2020_04_findExecutable

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

AFFECTED FILES
  autotests/applicationlauncherjobtest.cpp
  src/core/desktopexecparser.cpp
  src/gui/kprocessrunner.cpp

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

D29170: Detect executables without +x permission in $PATH to improve error message

[David Faure]

dfaure added a comment.


  Meanwhile I sent an email to kde-frameworks-devel to get more input on the 
question.
  Supporting custom setups is a great feature for a powerful desktop 
environment.
  E.g. I deploy custom self-built apps for my users, so I use some of those 
custom KDE features. Not this particular one though because they like icons in 
the plasma panel which means the desktop file gets copied into a 
plasma-specific directory, unfortunately. But that means I at least do things 
like Exec[$e]=$HOME/.bin/foo so that it's the same for all users -- just to 
name one useful "custom setup" feature.

INLINE COMMENTS

> ahmadsamir wrote in desktopexecparser.cpp:465
> I think we should check for isExecutable() here too (this matches the 
> behaviour of QStandardPaths::findExecutable()).

But if we do that, a non-executable file gets ignored here and we don't get the 
warning that this commit is all about.

It would all be much simpler [for this custom purpose] if findExecutable didn't 
check the executable bit ;-) Then we'd have one full path to check ourselves. 
Instead we have to duplicate the PATH lookup. I'd like to avoid having to also 
duplicate the current-dir-of-desktop-file lookup and the libexec lookup.

REPOSITORY
  R241 KIO

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

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

Re: changing icon sizes no longer emits signal

[David Faure]

On Saturday, April 25, 2020 4:54:43 PM CEST Friedrich W. H. Kossebau wrote:
> Am Samstag, 25. April 2020, 15:10:37 CEST schrieb Martin Koller:
> > Hi,
> > 
> > in liquidshell I'm using
> > 
> >   connect(KIconLoader::global(), ::iconLoaderSettingsChanged,
> > 
> > this, ::adjustIconSize);
> > 
> > to be informed whenever the icon sizes get changed in systemsettings.
> > I don't know since when but at least in version
> > 
> > Operating System: openSUSE Tumbleweed 20200419
> > KDE Plasma Version: 5.18.4
> > KDE Frameworks Version: 5.69.0
> > Qt Version: 5.14.1
> > 
> > I no longer receive this signal.
> 
> I can confirm that changing icon sizes in Plasma Systemsettings and clicking
> apply seems to have no effect also for all other places which would adapt
> to those values, like KXmlGui-driven toolbars (which also by a quick check
> seems to connect to that signal). The changed sizes are only picked up by
> newly started instances of programs.
> I am also on openSUSE TW with same versions. Also not sure when I saw this
> working the last time.
> 
> Given the API dox of KIconLoader does not note that signal is deprecated, I
> would assume this signal is still to be used (and main.cpp of the icon kcm
> seems to call KIconLoader::emitChange(...), which should trigger the signal
> via D-Bus in all client side, from a peek view at code path.

The call to KIconLoader::emitChange is deeply nested in KDE4 compat stuff, 
with possible exit paths before.

A patch like the one attached seems to help, but someone who knows the KCM 
better (or has time to dig) should make this conditional on the user actually 
changing icon sizes, and only emit for the groups that have changed.

-- 
David Faure, fa...@kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5
diff --git i/kcms/icons/main.cpp w/kcms/icons/main.cpp
index 172d9bc18..4ab2f5150 100644
--- i/kcms/icons/main.cpp
+++ w/kcms/icons/main.cpp
@@ -148,7 +148,10 @@ void IconModule::save()
 
 processPendingDeletions();
 
-KIconLoader::global()->newIconLoader();
+// TODO do this only for the group(s) where icon sizes have changed
+for (int i = 0; i < KIconLoader::LastGroup; i++) {
+KIconLoader::emitChange(KIconLoader::Group(i));
+}
 }
 
 bool IconModule::isSaveNeeded() const
@@ -288,8 +291,6 @@ void IconModule::exportToKDE4()
 
 //message kde4 apps that icon theme is changed
 for (int i = 0; i < KIconLoader::LastGroup; i++) {
-KIconLoader::emitChange(KIconLoader::Group(i));
-
 QDBusMessage message = QDBusMessage::createSignal(QStringLiteral("/KGlobalSettings"),
   QStringLiteral("org.kde.KGlobalSettings"),
   QStringLiteral("notifyChange"));

Re: building KF5 projects against a different Qt5 version (than the one the KF5 frameworks were built against)

[David Faure]

On Saturday, April 25, 2020 2:23:34 PM CEST René J.V. Bertin wrote:
> Hi,
> 
> A priori Qt guarantees that you can run binaries against a different, newer
> Qt version than they were built against, as long as no private APIs are
> used. This also works if that newer Qt version is installed elsewhere,
> provided you set LD_LIBRARY_PATH correctly 

Yes.

> (and possibly LD_PRELOAD).

I don't see why you would set LD_PRELOAD in this configuration.

> I think this also means you can build code against such a newer (test) Qt
> install even if that code uses libraries which were built against the older
> Qt version (as long as no mixups occur at runtime; I've done this
> successfully with code that uses Phonon, for instance).

Yes when Qt itself is upgraded to a newer version.
Switching between two Qt prefixes at cmake time is another issue though,
which seems to be what you're experiencing.

> I'm not having a lot of luck building code that uses KF5 frameworks this
> way. Even after getting cmake to find the intended newer Qt version I still
> get header conflicts because something inserts unwanted header search
> paths.

I think this is because the KF5 target brings its own include paths as part of 
the imported target.

> I don't expect there to be official support for this kind of tricky things.
> Still, testing code against a new Qt version installed in parallel doesn't
> seem to be such an usual thing to do so if there is a trick to this I'd
> love to hear it.

Build against the old Qt, run against the new Qt?
Then it's back to "only" LD_LIBRARY_PATH switcheroo which works well, while at 
the buildsystem level it's all based upon the old Qt so no mixup there.

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

D28765: KSettings::Dialog: add support for KPluginInfos without a KService

[David Faure]

dfaure closed this revision.

REPOSITORY
  R295 KCMUtils

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

To: dfaure, pino, broulik, mart, davidedmundson, ngraham, svuorela
Cc: svuorela, cblack, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

RFC: relative executables in desktop files

[David Faure]

During the review of https://phabricator.kde.org/D29170 the following question 
surfaced again: should it be possible for a desktop file to refer to an 
executable that is in the "current directory", for some definition of that 
term. At least, outside of $PATH.

In my opinion, in a GUI program started graphically, the notion of "current 
dir" (QDir::currentPath()) has no meaning. The user can't see it and can't 
change it. When starting the program from the command line it does serve a 
purpose for command line arguments, but IMHO not after that (e.g. if you 
navigate to another dir in dolphin, QDir::currentPath() still points to the 
directory you started dolphin from).

There is however another "current directory" that might make more sense when 
starting a desktop file: the directory of the desktop file itself.

There are actual use cases for that, see this very old request on the XDG 
mailing-list: 
https://lists.freedesktop.org/archives/xdg/2011-April/011883.html

AFAICS this discussion has 3 possible outcomes:

* We do not support starting executables that are not in $PATH, end of story.
That was actually what I had in mind when writing the code initially, any use 
of API that also looked in the current directory (like QFileInfo::exists) was 
accidental. Unless I'm mistaken, this is how it's been until now. It's also 
what the XDG spec [1] says.

* We support launching executables relative to the desktop file, transparently. 
In the same directory, put a copy of dolphin called dolphin2, and a copy of 
org.kde.dolphin.desktop modified to say Exec=dolphin2, and clicking on the 
desktop file starts dolphin2. That's what my current revision of D29170 does.
I'm wondering if this is the right thing to do though.
After all, on the command line "foo" doesn't start a local executable called 
foo, only ./foo does that. Except for people who add "." to $PATH, but that's 
generally not recommended (security, accidental use of wrong binary).

* We could also adopt the above proposal from the xdg list, which is that 
Exec=foo only looks in $PATH, while Exec=./foo only looks in the directory of 
the desktop file.

(I'm purposefully excluding the 4th option, resolving relative to 
QDir::currentPath() which as explained at the top, would be nonsense IMHO)

Thoughts?

[1] 
https://specifications.freedesktop.org/desktop-entry-spec/desktop-entry-spec-latest.html#exec-variables

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

D29170: Detect executables without +x permission in $PATH to improve error message

[David Faure]

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


  Resolve relative executables using the directory of the .desktop file 
referring to them
  
  Not useful for /usr/share/applications stuff, but useful for custom setups
  where a custom desktop file refers to a local executable.
  
  Re-reading the thread 
https://lists.freedesktop.org/archives/xdg/2011-April/011883.html
  I'm wondering if this is the right thing to do though.
  
  After all, on the command line "foo" doesn't start a local executable called 
foo,
  only ./foo does that. I was always working under that assumption for Exec= as 
well
  (the fact that the current dir isn't part of the search path). Undecided.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29170?vs=81209=81213

BRANCH
  2020_04_findExecutable

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

AFFECTED FILES
  autotests/applicationlauncherjobtest.cpp
  src/core/desktopexecparser.cpp
  src/gui/kprocessrunner.cpp

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

D29170: Detect executables without +x permission in $PATH to improve error message

[David Faure]

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


  Better support for relative vs absolute executable names, with unittest.
  
  But I'll look into relative-to-desktop-file instead of 
relative-to-QDir::current.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29170?vs=81159=81209

BRANCH
  2020_04_findExecutable

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

AFFECTED FILES
  autotests/applicationlauncherjobtest.cpp
  src/gui/kprocessrunner.cpp

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

D29170: Detect executables without +x permission in $PATH to improve error message

[David Faure]

dfaure marked an inline comment as done.
dfaure added inline comments.

INLINE COMMENTS

> ahmadsamir wrote in kprocessrunner.cpp:53
> KProcessRunner tries finding the executable in the current dir too, so to be 
> precise in the reported error message maybe append currentDir.absolutePath() 
> to searchPaths?
> 
> Also KProcessRunner only checks that the executable exists in the current 
> dir, "!QFileInfo::exists(realExecutable)", it should also check that it's 
> executable. So that KProcessRunner checks the "exists and is +x" and here we 
> check "exists but not +x", if that makes sense.
> 
> I guess you'll have the add an ifdef kludge since QString::SkipEmptyParts is 
> deprecated in Qt 5.15 according to 
> https://lxr.kde.org/source/frameworks/frameworkintegration/src/kpackage-install-handlers/kns/main.cpp#0068
>  (I am still on 5.14).

In a GUI program started graphically, the notion of "current dir" means very 
little. You can't see it, you can't change it.
Granted, when starting it from the command line it does serve a purpose for 
command line arguments, but IMHO not after that.

What's more, here we're executing a KService i.e. usually a desktop file 
(unless it was constructed from an executable name, display name and icon).

Hmm OK I can make a testcase for it with a special case. A copy of dolphin's 
desktop file with Exec=dolphin2 and a copy of dolphin called dolphin2 in the 
same directory, and starting dolphin from that directory. Interesting, it leads 
to "execvp: Permission denied", that's a new one :)
(must be from QProcess).

My next path will fix that testcase, but it remains an unexpected corner case. 
The same dolphin started from $HOME and then navigating to that directory, 
cannot start that desktop file.
Maybe we want to look for executables relative to the desktop file, rather than 
from the hidden-to-the-user current directory... Actually I remember people 
asking for that to work, a VERY long time ago on the freedesktop xdg 
mailing-list. IIRC I even made it work back then.

Thanks for the SkipEmptyParts information and for noticing the weird if() -- 
fixed.

> ahmadsamir wrote in kprocessrunner.cpp:60
> Should be https://

I was counting on the redirection :-)

REPOSITORY
  R241 KIO

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

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

D29153: Move handling of untrusted programs to ApplicationLauncherJob.

[David Faure]

dfaure added a comment.


  > The question is how that will work in conjunction with 
KNotificationJobUiDelegate? In principle we could also make it emit a 
KNotification with buttons
  
  We would need a KIO::NotificationJobUiDelegate subclass of 
KNotificationJobUiDelegate in KIOGui, which also implements 
UntrustedProgramHandlerInterface.
  For KF5, its constructor would register itself using 
setDefaultUntrustedProgramHandler, and the destructor would use that same 
method to reset it to nullptr (to avoid leaving a dangling pointer).
  Alternatively it could do just like KIO::JobUiDelegate and register a 
singleton.
  
  In KF6, it'll inherit the handler interface and it'll just be autodetected by 
qobject_cast on the job's delegate (or we could even already do this as the 
primary way to find the handler, with fallback to checking the singleton --- 
this is all because I can't change what KIO::JobUiDelegate inherits from).

REPOSITORY
  R241 KIO

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

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

D29153: Move handling of untrusted programs to ApplicationLauncherJob.

[David Faure]

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


  Make UntrustedProgramHandlerInterface async.
  
  This required a nasty QEventLoop for KRun though, since it has a sync API.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29153?vs=81158=81173

BRANCH
  2020_04_UntrustedProgramHandler

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

AFFECTED FILES
  autotests/applicationlauncherjobtest.cpp
  autotests/applicationlauncherjobtest.h
  src/core/CMakeLists.txt
  src/core/jobuidelegateextension.h
  src/core/untrustedprogramhandlerinterface.cpp
  src/core/untrustedprogramhandlerinterface.h
  src/gui/applicationlauncherjob.cpp
  src/gui/applicationlauncherjob.h
  src/widgets/CMakeLists.txt
  src/widgets/jobuidelegate.cpp
  src/widgets/krun.cpp
  src/widgets/krun_p.h
  src/widgets/widgetsuntrustedprogramhandler.cpp
  src/widgets/widgetsuntrustedprogramhandler.h
  tests/kruntest.cpp

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

D28909: smb: port to Result system to force serialization of error/finish condition

[David Faure]

dfaure added inline comments.

INLINE COMMENTS

> sitter wrote in kio_smb.h:96
> Sure, if you think it's solid enough from an API POV.
> 
> I was thinking that we should amend the slavebase API for KF6 in general. 
> Instead of having error/finished/opened all functions on an API level should 
> return a Result and the slave loop would emit the relevant signal based on 
> the Result. IOW: what currently happens in the derived SlaveBases actually 
> ought to be KIO-internal.
> 
> That would then also allow us to get rid of the two-class split again. And 
> the "fronting" class is actually a much bigger concern than Result to me. The 
> call finalization logic is 100% code copy and so very easy to get wrong (e.g. 
> sftp's special() not finishing when in fact it should).

Excellent idea, but why wait for KF6?

We could implement a subclass of SlaveBase, in KIO, let's call it SlaveBaseV2, 
which

- Reimplements all the virtual methods from SlaveBase, in a private section
- Defines a new of virtual methods, called differently somehow
- Implements the signal emission based on the return value of those new virtual 
methods.

This allows for incremental porting of the slaves, instead of a big KF6 
breakage.
And the V2 class can be developed in a single slave first, before having to 
freeze its API.

REPOSITORY
  R320 KIO Extras

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

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

D27216: [KProcessRunner] Improve error messages on failure

[David Faure]

dfaure added a comment.


  Done in D29170 . Now we can start 
discussing the other part of the bug, executable doesn't launch due to missing 
libs.
  
  Ah BTW the unittest leads to "cp" failing because it deletes the temporary 
dir under cp's feet. This could be fixed, but in any case, we don't want to pop 
up an error message on every process that terminates with a non-0 exit code. 
Imagine launching firefox, using it for 2 hours, then it crashes, and you get - 
in addition to drkonqi - that messagebox saying "It terminated abnormally. For 
more details try running it from a terminal emulator (e.g. Konsole)"   I 
think at most we want to let the job run for 5 more seconds to handle "crash on 
startup", but not react on longer-running programs that terminate abnormally.

REPOSITORY
  R241 KIO

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

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

D29170: Detect executables without +x permission in $PATH to improve error message

[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.

REVISION SUMMARY
  QStandardPaths::findExecutable will not return to us a non-executable binary.
  So implement our own iteration over $PATH to detect such a case.
  Note: this doesn't handle the case where PATH isn't set at all 
(QStandardPaths implements a fallback)
  nor do we implement this for Windows (where chmod -x doesn't really exist as 
is). I think this is fine,
  in the worst case the user will get the other error message, program not 
found.

TEST PLAN
  'sudo chmod a-x /usr/bin/gwenview' then try opening a picture with gwenview 
from e.g. dolphin, see the error message
  
  CCBUG: 415567

REPOSITORY
  R241 KIO

BRANCH
  2020_04_findExecutable

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

AFFECTED FILES
  src/gui/kprocessrunner.cpp

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

D29153: Move handling of untrusted programs to ApplicationLauncherJob.

[David Faure]

dfaure added inline comments.

INLINE COMMENTS

> broulik wrote in untrustedprogramhandlerinterface.h:79
> I was wondering if this should be done async? Nested event loops are quite a 
> problem when QML is involved.

I don't see a nested event loop in makeServiceFileExecutable.

I guess your comment was for the main method, showUntrustedProgramWarning?

Indeed we could make that one async, if I turn this interface into a QObject 
and add a signal.
Can do.

This kind of turns it into a job, but not really, we don't need this bit to 
have its own delegate etc.
I think a signal is enough?

> broulik wrote in applicationlauncherjob.cpp:117
> You know I'm not a fan of jobs suddenly blocking on IO :)

You know I'm not a fan of network mounts, exactly for this reason

In my opinion it's a crazy requirement to say that we are not allowed to use 
QFile or QFileInfo anywhere anymore, because of network mounts. What's your 
suggestion? I'm not even aware of an asynchronous (but still portable) 
equivalent.

REPOSITORY
  R241 KIO

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

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

D27216: [KProcessRunner] Improve error messages on failure

[David Faure]

dfaure added a comment.


  Please ignore my previous comment. When launching gwenview by clicking on an 
image (and not on the gwenview executable itself), we indeed do end up with 
KProcessRunner failing to find the executable because of the missing -x.
  I'll implement the proper investigation and error message.

REPOSITORY
  R241 KIO

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

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

D29153: Move handling of untrusted programs to ApplicationLauncherJob.

[David Faure]

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


  Also set a KIO::JobUiDelegate in KRun itself (which simplifies its code)

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29153?vs=8=81158

BRANCH
  2020_04_UntrustedProgramHandler

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

AFFECTED FILES
  autotests/applicationlauncherjobtest.cpp
  autotests/applicationlauncherjobtest.h
  src/core/CMakeLists.txt
  src/core/jobuidelegateextension.h
  src/core/untrustedprogramhandlerinterface.cpp
  src/core/untrustedprogramhandlerinterface.h
  src/gui/applicationlauncherjob.cpp
  src/gui/applicationlauncherjob.h
  src/widgets/CMakeLists.txt
  src/widgets/jobuidelegate.cpp
  src/widgets/krun.cpp
  src/widgets/krun_p.h
  src/widgets/widgetsuntrustedprogramhandler.cpp
  src/widgets/widgetsuntrustedprogramhandler.h
  tests/kruntest.cpp

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

D27216: [KProcessRunner] Improve error messages on failure

[David Faure]

dfaure added a comment.


  Note that D29153  affects this since it 
will detect the lack of +x ahead of time in ApplicationLauncherJob and prompt 
the user [if the application uses KIOWidgets' JobUiDelegate -- oh I need to do 
this in KRun, will do right away].
  
  Technically this code path will still exist, but the (first) described 
testcase will not end up there anymore (once D29153 
 lands).

REPOSITORY
  R241 KIO

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

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

D28765: KSettings::Dialog: add support for KPluginInfos without a KService

[David Faure]

dfaure added a comment.


  Thanks for the review! I was getting desperate to get one ;-)

INLINE COMMENTS

> svuorela wrote in kcmoduleinfo.cpp:73
> At a later point, I*m not sure what the purpose is for these members are - 
> but that's probably for another changeset.

Right, I was wondering the same. We could just implement the getters so that 
they call these things directly.

But then I'm also wondering what's the purpose of KCModuleInfo at all and 
whether we could just use KPluginInfo directly instead -- but that's a KF6 
consideration, since it's part of the API for KCMultiDialog and others.

Well, we could add a addModule(KPluginInfo) overload if that's the direction 
we're going for; I just don't have full overview on the KCM stuff.

> svuorela wrote in kcmoduleinfo.h:131
> Can we mark it as deprecated?

It's complicated.

1. If you use the QString constructor, you know service() is usable. That's the 
case for all users of KCModuleInfo except KCModuleLoader. [Not that there are 
many]

2. Even KCModuleLoader calls service(), to test for noDisplay().

The whole concept of NoDisplay only makes sense for desktop files, unless we 
want to have that in JSON metadata as well. I suppose we do, but this currently 
seems to be completely missing (e.g. from KPluginMetaData, if we want this in 
all plugins, not just KCMs). We'd have to duplicate the logic currently in 
KService::noDisplay().

Any volunteers to look into this? :-)

REPOSITORY
  R295 KCMUtils

BRANCH
  master

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

To: dfaure, pino, broulik, mart, davidedmundson, ngraham, svuorela
Cc: svuorela, cblack, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D29153: Move handling of untrusted programs to ApplicationLauncherJob.

[David Faure]

dfaure added a reviewer: mdlubakowski.

REPOSITORY
  R241 KIO

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

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

D29153: Move handling of untrusted programs to ApplicationLauncherJob.

[David Faure]

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


  Fix error message box after the user clicks Cancel.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29153?vs=81108=8

BRANCH
  2020_04_UntrustedProgramHandler

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

AFFECTED FILES
  autotests/applicationlauncherjobtest.cpp
  autotests/applicationlauncherjobtest.h
  src/core/CMakeLists.txt
  src/core/jobuidelegateextension.h
  src/core/untrustedprogramhandlerinterface.cpp
  src/core/untrustedprogramhandlerinterface.h
  src/gui/applicationlauncherjob.cpp
  src/gui/applicationlauncherjob.h
  src/widgets/CMakeLists.txt
  src/widgets/jobuidelegate.cpp
  src/widgets/krun.cpp
  src/widgets/widgetsuntrustedprogramhandler.cpp
  src/widgets/widgetsuntrustedprogramhandler.h
  tests/kruntest.cpp

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

D29153: Move handling of untrusted programs to ApplicationLauncherJob.

[David Faure]

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


  improve docu, add test in kruntest

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29153?vs=81101=81108

BRANCH
  2020_04_UntrustedProgramHandler

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

AFFECTED FILES
  autotests/applicationlauncherjobtest.cpp
  autotests/applicationlauncherjobtest.h
  src/core/CMakeLists.txt
  src/core/jobuidelegateextension.h
  src/core/untrustedprogramhandlerinterface.cpp
  src/core/untrustedprogramhandlerinterface.h
  src/gui/applicationlauncherjob.cpp
  src/gui/applicationlauncherjob.h
  src/widgets/CMakeLists.txt
  src/widgets/jobuidelegate.cpp
  src/widgets/krun.cpp
  src/widgets/widgetsuntrustedprogramhandler.cpp
  src/widgets/widgetsuntrustedprogramhandler.h
  tests/kruntest.cpp

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