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

Authored by feverfew on Aug 23 2019, 6:09 PM.

Details

Summary

This patch is required to provide seamless integration with KIOFuse and KIO.

The patch attempts to convert URLs that applications will not understand
natively into local paths based on a KIOFuse mount. If successful, the URL
passed to the application is changed to its location within the KIOFuse mount.
If it is not successful, the URL is not changed.

If KIOFuse is not installed, then kioexec is simply used.

Test Plan

Compile and in the build directory run source prefix.sh, so that applications
using KIO will use this patch.

Next build this: https://invent.kde.org/asaoutkin/kio-fuse/tree/DbusConversion

For the integration to work the KIOFuse service needs to be installed
OR one can start the kio-fuse process.

Once you have either of those one can simply browse to any non-local location in
say, Dolphin, where upon opening a URL in a non-KIO enabled app, it should open
in the KIOFuse mount, except for MTP/Gdrive URLs - those will use KIOExec.

Diff Detail

Repository
R241 KIO
Branch
KDEDModule (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 18450
Build 18468: arc lint + arc unit
feverfew created this revision.Aug 23 2019, 6:09 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptAug 23 2019, 6:09 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
feverfew requested review of this revision.Aug 23 2019, 6:09 PM

How does kio-fuse know when to unmount?

src/widgets/krun.cpp
619

Generally QDBusInterface is a class to be avoided.

I would recommend copy/pasting your XML file and then using the generated interface from that.

QDBusInterface sucks as it makes a blocking call introspecting (which isn't a huge problem considering we're going to block anyway!) but also leads to going crazy figuring out typos in methods/arguments.

623

We rarely check lastError(), that's for if you use the lower level API calls.

639

All DBus replies are a union of their success value and an error.

Returning an error reply when mounting fails would allow us to provide a more helpful message back to the user

feverfew updated this revision to Diff 68931.Mon, Oct 28, 10:49 PM
  • Switch from KDED module to DBus service
feverfew updated this revision to Diff 69201.Sat, Nov 2, 9:43 PM

Don't use KIOFuse with mtp/gdrive

feverfew edited the summary of this revision. (Show Details)Sat, Nov 2, 10:17 PM
feverfew edited the test plan for this revision. (Show Details)
feverfew edited reviewers, added: davidedmundson, dfaure, ngraham; removed: chinmoyr.

@dfaure Just to give you a bit of context. KIOFuse is about to enter KDE Review, but at the same time, KIOFuse is most useful when integrated to KIO via this patch. So we'd like to get this reviewed early to save a bit of effort; instead of getting it passed KDE Review and then having to wait for approval for this patch, we'd like to do both concurrently, and simply merge this once KIOFuse is officially a KDE project.

@ngraham can do the QA side of the review to save some time, but you are of course welcome to test this yourself!

sitter added a subscriber: sitter.Wed, Nov 6, 2:04 PM
sitter added inline comments.
src/core/desktopexecparser.cpp
314

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

316

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

317

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 });
319

there should be a space after for.

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

src/core/kiofuseinterface.h
28

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?

src/widgets/krun.cpp
598

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

605

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.