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
