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

[Ahmad Samir]

ahmadsamir added a comment.


  In D29385#671987 , @dfaure wrote:
  
  > That's the point, a NFS mount *is* a local URL, so we do use QFile for it. 
And then it takes forever because the kernel has to talk over a socket to 
answer us.
  >  Yes this is horrendous. I hate network mounts :-)
  
  
  I don't have *any* network mounts, never have, so I wasn't aware of that. 
Yep, that is horrendous.

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.


  That's the point, a NFS mount *is* a local URL, so we do use QFile for it. 
And then it takes forever because the kernel has to talk over a socket to 
answer us.
  Yes this is horrendous. I hate network mounts :-)

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

[Ahmad Samir]

ahmadsamir added a comment.


  In D29385#671980 , @dfaure wrote:
  
  > It's 3 times faster on my local SSD.
  >
  > Now think of a NFS mount on a server from another country
  
  
  I was thinking mostly of QFile when url.isLocalFile() is true, but yeah I see 
your point :)

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.


  It's 3 times faster on my local SSD.
  
  Now think of a NFS mount on a server from another country

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

[Ahmad Samir]

ahmadsamir added inline comments.

INLINE COMMENTS

> dfaure wrote in openurljob.cpp:261
> 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.]

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

I know this ship has sailed (well, sunk in this case :)), but if it's 3 times 
faster to use QFile, then is it really a "blocking I/O" operation? it's too 
fast to be "blocking"...

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

[Ahmad Samir]

ahmadsamir added a comment.


  In D29385#664552 , @dfaure wrote:
  
  > -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)?
  
  
  I vote:
  lineEdit->setDragEnabled(true); lineEdit->setClearButtonEnabled(true); 
lineEdit->setReadOnly(true);
  it's:
  
  - more readable, and easier to use (similar methods are used through out Qt 
code)
  - avoids the case you talked about, of the user setting nonsensical, from the 
code POV, flags.
  
  I was too slow working my way through this short story^W^W review, so I've 
gathered my other comments (mostly code style, docs changes...etc) in a diff, 
that I'll submit shortly.

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

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

[Kai Uwe Broulik]

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

REPOSITORY
  R241 KIO

BRANCH
  2020_05_openurljob

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

[Kai Uwe Broulik]

broulik added a comment.


  +1

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

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

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

[Alexander Saoutkin]

feverfew added a comment.


  In D29385#662435 , @dfaure wrote:
  
  > 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().
  
  
  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?

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

[Alexander Saoutkin]

feverfew added a comment.


  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?

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

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> broulik wrote in openurljob.cpp:274
> I know what you always say when I say what I always say but can we use a 
> mimetype job here? :)

Seems I forgot to remove this when I added the comment above instead :)

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

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

[Kai Uwe Broulik]

broulik added a comment.


  Cool!

INLINE COMMENTS

> openurljob.cpp:261
> +
> +void KIO::OpenUrlJobPrivate::determineLocalMimeType()
> +{

I know what you always say when I say what I always say, but why not just 
always stat/mimetype job?

> openurljob.cpp:274
> +QMimeDatabase db;
> +QMimeType mime = db.mimeTypeForUrl(m_url);
> +//qDebug() << "MIME TYPE is " << mime.name();

I know what you always say when I say what I always say but can we use a 
mimetype job here? :)

> openurljob.cpp:305
> +q->emitResult();
> +} else {
> +if (m_followRedirections) { // Update our URL in case of a 
> redirection

Using an early return here would make the code less nested

> openurljob.cpp:447
> +// X-KDE-LastOpenedWith holds the service desktop entry name that
> +// was should be preferred for opening this URL if possible.
> +// This is used by the Recent Documents menu for instance.

was or should be?

> openurljob.cpp:506
> +q->setError(KJob::UserDefinedError);
> +q->setErrorText(i18n("The file %1 is an executable 
> program. "
> + "For safety it will not be started.", 
> m_url.toDisplayString().toHtmlEscaped()));

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.

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

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

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

[Sune Vuorela]

svuorela added a comment.


  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.

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

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

[Friedrich W. H. Kossebau]

kossebau added a comment.


  Quick first nitpicks from initial scan what this patch is about, without 
having looked closer.

INLINE COMMENTS

> openurljobtest.cpp:108
> +{
> +std::for_each(m_filesToRemove.begin(), m_filesToRemove.end(), [](const 
> QString & f) {
> +QFile::remove(f);

why no simple range-based for loop?

> openurljob.h:73
> + */
> +void setRunFlags(ApplicationLauncherJob::RunFlags runFlags);
> +

Makes me wonder if those runflags should not be rather part of KIO namespace, 
to decouple classes here.

> openurljob.h:96
> + */
> +void setRunExecutables(bool allow);
> +

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.

> openurljob.h:127
> + */
> +void mimetypeFound(const QString );
> +

-> "mimeTypeFound"? would match other casing.

> openurljobhandlerinterface.cpp:34
> +
> +OpenUrlJobHandlerInterface::~OpenUrlJobHandlerInterface()
> +{

`= default;` instead?

> openurljobhandlerinterface.h:33
> +/**
> + * @brief The OpenUrlJobHandlerInterface class allows OpenUrlJob to
> + * prompt the user about which application to use to open URLs that do not

Please prepend a line

  * @class OpenUrlJobHandlerInterface openurljobhandlerinterface.h 


at the top, to trick doxygen into documenting the full CamelCase include.

> openurljobhandlerinterface.h:54
> + */
> +virtual ~OpenUrlJobHandlerInterface();
> +

override instead?

> openurljobhandlerinterface.h:84
> +private:
> +class Private;
> +Private *const d;

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.

> widgetsopenurljobhandler.h:45
> +private:
> +class Private;
> +Private *const d;

Do not use embedded Private class. Also pimp needed here, given not a public 
class?

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

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