D26557: Allow to handle apps with Terminal=True in their desktop file, handle their associated mimetype properly

[David Faure]

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


  Please add me as reviewer when touching my code in KIO. I almost didn't notice this one.

INLINE COMMENTS

> desktopexecparser.cpp:213
> +    // add x-scheme-handler/<protocol>
> +    for (const auto &mimeType : service.serviceTypes()) {
> +        if (mimeType.startsWith(QStringLiteral("x-scheme-handler/"))) {

Move result of method call into local const variable. The usual range-for-detaches problem.

Can you call mimeTypes() instead of serviceTypes() here? I'm trying to slowly split the two notions, after it all got mixed up long ago.

> desktopexecparser.cpp:214
> +    for (const auto &mimeType : service.serviceTypes()) {
> +        if (mimeType.startsWith(QStringLiteral("x-scheme-handler/"))) {
> +            supportedProtocols << mimeType.mid(17);

QLatin1String is enough here

> krun.cpp:101
> -{
> -    // We have up to two sources of data, for protocols not handled by kioslaves (so called "helper") :
> -    // 1) the exec line of the .protocol file, if there's one

OK I wasn't happy about this docu disappearing but actually it fits better into hasSchemeHandler() which I later on moved to KIO::DesktopExecParser.

Done in https://commits.kde.org/kio/45f5d79600809f4c153c7b39ad90652cb921875c
You can now remove it from here indeed :)

> krun.cpp:106
>      }
> -    Q_ASSERT(KProtocolInfo::isHelperProtocol(protocol));
> -    return KProtocolInfo::exec(protocol);
> +    return KService::Ptr();
>  }

Just remove the if then. This method becomes a one-liner.

> krun.cpp:956
> +            //  if there's one...
> +            if (runApplication(*service, QList<QUrl>() << d->m_strURL, d->m_window)) {
>                  d->m_bFinished = true;

You forgot to pass d->m_asn here (last argument of runApplication).

> krun.cpp:967
> +                // the scheme has no service or protocol associated
> +                // use url scheme associated protocol
> +                mimeTypeDetermined(KProtocolManager::defaultMimetype(d->m_strURL));

I think what you meant here was scheme-associated *mimetype* ?
or "default mimetype from the protocol file".

In a URL, scheme==protocol, but in KIO we're mostly using the word protocol to refer to those .protocol files. KProtocolInfo::exec() is also reading from the [scheme associated] protocol file so this comment is confusing.

(I wonder if this isn't dead code though - helper protocols don't set an empty exec line and a default mimetype, this would make no sense, only real ioslaves like kio_man set a default mimetype, text/html)

> krun.cpp:971
> +            } else {
> +                if ( run(exec, QList<QUrl>() << d->m_strURL, d->m_window, QString(), QString(), d->m_asn)) {
> +                    d->m_bFinished = true;

remove space before 'run'

REPOSITORY
  R241 KIO

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

To: meven, ervin, ngraham, #frameworks, dfaure
Cc: dfaure, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20200112/31cb39a3/attachment-0001.html>