D23384: [WIP] Adding support for mounting KIOFuse URLs for applications that don't use KIO

[Harald Sitter]

sitter added inline comments.

INLINE COMMENTS

> desktopexecparser.cpp:314
> +    org::kde::KIOFuse kiofuse_iface(QStringLiteral("org.kde.KIOFuse"),
> +                                QStringLiteral("/org/kde/KIOFuse"),
> +                                QDBusConnection::sessionBus());

align arguments with first argument. same in krun.cpp.

> desktopexecparser.cpp:316
> +                                QDBusConnection::sessionBus());
> +    QList<QUrl> parsedUrls;
> +    QVector<QPair<QDBusPendingReply<QString>, QUrl>> replies;

Do we need this? Seems to me you could just append to d->url directly.

> desktopexecparser.cpp:317
> +    QList<QUrl> parsedUrls;
> +    QVector<QPair<QDBusPendingReply<QString>, QUrl>> replies;
>      if (!mx1.hasUrls) {

There's nothing wrong with this. But wouldn't using `QHash<QDBusPendingReply<QString>, QUrl>` make for easier to read code all in all since you don't have to mess with pairs?

Alternatively with a vector I'd still make a struct for the data

  struct MountRequest { QDBusPendingReply<QString> reply, QUrl url };
  QVector<MountRequest> requests;
  ...
  requests.push_back({ mountUrl(url), url });

> desktopexecparser.cpp:319
>      if (!mx1.hasUrls) {
> -        Q_FOREACH (const QUrl &url, d->urls)
> +        for(QUrl &url : d->urls) {
>              if (!url.isLocalFile() && !hasSchemeHandler(url)) {

there should be a space after for.

what happened to the constness (also in the loop below)

> kiofuseinterface.h:28
> + */
> +class KIOCORE_EXPORT KIOFuseInterface: public QDBusAbstractInterface
> +{

I don't think this should be a public/exported class. It's purely for internal use within KIO. It has no reason to become part of the ABI IMHO. To that end it also shouldn't be part of ecm_generate_headers.

I suppose this generated file could also be replaced with the actual xml and generated at build time instead?
Manually editing generated files is a bit meh in general.

To get the interface into both the core and widgets target I suppose you'd simply add it to both source lists.

Does anyone else have an opinion on this?

> krun.cpp:598
> +        QList<QUrl> parsedUrls;
> +        for (QList<QUrl>::Iterator it = urls.begin(); it != urls.end(); ++it) {
> +            QUrl url = *it;

Is there a reason you are not using a range based for loop here? `for (const QUrl &url : urls)`

> krun.cpp:605
> +            if (KIO::DesktopExecParser::isProtocolInSupportedList(url, appSupportedProtocols)
> +                && url.password().isEmpty())
> +                continue;

That seems like a hack for a bug in VLC and also super opinionated and also somewhat unrelated to fuse? If an application says it supports %u/%U and a given protocol, we should expect it to be able to parse a valid rfc2396 URI I would think.

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: sitter, davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, GB_2, michaelh, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20191106/6002930d/attachment-0001.html>