I believe most of your (@sitter) comments (apart from the misaligned & probably were caused by me forgetting to rebase), lmk if otherwise.

• Merge branch 'master' into arcpatch-D21795_1
• Update version

Ok, I see what's going on here. Earlier I mucked up the diff a bit and had to go back to different diff id and reapply my changes. In the process I forgot to rebase onto master. Once I do that David's copyright will be back in (and the event loop goes with it).

With a brief look the ideal scenario is to be able to use DesktopExecParser::resultingArguments, or a class that calls it KRun. That should do the conversion. I'm just not sure if you have the information required to use it.

• Fix the diff

Ok I've commandeered this on request, as we're so close to getting this done. I've simply addressed sitter's comments here seeming as they're simple enough to do so without understanding the code that well. I haven't actually tested this in any capacity, as again, I'm not too familiar with this code and what it's trying to accomplish. If someone could point me in the correct direction I'll test as well.

• add const &

• Rebase
• Respond to sitter's comments

• Remote unnecessary newline

• Actually fix the bug

Good stuff, I'll admit I kind of skimmed over the bits that were rewrites error(...); return; -> Result::fail(), I assume you've mapped correctly there. Only minor comments from me.

In D27291#609358, @dfaure wrote:

feverfew: you're probably looking at master while this is a patch for the 19.12 branch.
See https://phabricator.kde.org/D26358 which happened in master.

I guess that makes this commit ok for 19.12, but it has to be redone differently in master.

I'm a bit confused here, isn't the protocol file now a JSON file? I'm looking at the repo and it appears to be the case, so I'm not seeing how this diff is seeing this as a rename?

• Merge branch 'master' into arcpatch-D26191
• adhere to protcol -> json switch

Cleanup code according to comments

Fix introduction of BIC method

In D26148#585958, @dfaure wrote:

In D26148#585944, @ngraham wrote:

Thanks @dfaure! Would you be able to also review D26191?

Yes, once it has been ported to virtual_hook as well.

Fix introduction of BIC method

Add fact that protocol supports truncation

Add feature to determine if protocol supports truncation

Avoid code duplication

Avoid free(NULL)

In D26148#582365, @apol wrote:

It would be interesting to explain why it's important or how it's to be used in the commit message.

• Free unneeded struct pointer

Add unit test for truncation

fix indentation

• fix code style

In D23384#577174, @dfaure wrote:

For SMB shares I guess what can happen is that the URL doesn't have a username in it, but it still needs a password? IIRC there was no username in "old" samba (W95, W98, XP, ...?).
This might explain what you were saying about people having password problems even with empty userInfo() (which means username and password).
But we can hardly know, just by looking at a URL, if a password is needed... I guess most of time a password is actually needed? Maybe you're right after all, and we should always send SMB urls to kiofuse..... Dunno.

The current patch should be tested with latest kio-fuse master, as the blacklisting has been moved there to keep this patch clean.

On further looking, it seems like git grep doesn't really tell the full picture. It seems like resultingArguments is called before resolveURLs is, so I've simplified the diff as requested.

• Merge branch 'master' into arcpatch-D23384
• Address comments
• Remove unnecessary mount requests in krun
• Only use KIOFuse if password is empty

In D23384#572363, @dfaure wrote:

Why does KRun duplicate all of the (new) code from DesktopExecParser, when DesktopExecParser is actually a helper class for KRun?
I would expect it to have solved all this already, unless I'm missing something about the various code paths.
The code you changed in krun.cpp was supposed to simply resolve desktop:/foo to file:///bleh but everything else about %u/%f is done by DesktopExecParser.

"KeyChain" already has the "K" prefix ;) so Let's just drop the Q

What about "Baloo file search"?

Thanks for the reviews everyone! I'll update the diff and respond to the comments no later than Tuesday

Issues with KIOFuse itself should be opened up as an issue, or as a reply to the MR that is being tested; this differential only concerns communication between the KIOFuse mount and KIO itself.

don't send http(s) to kioexec

So before this patch we had the following behaviour:

ignore scheme handler

In D23384#571358, @fvogt wrote:

In D23384#571276, @ngraham wrote:

I'm afraid that even with that change, the issue is still present. I honestly don't think it would be the worst thing in the world if we always handed the kio-fuse paths to apps that don't use ioslaves.

It would be. I like to have links like http://kde.org opened properly in the web browser, ftp://some/where opened in an FTP client and so on...
Media players know more about the format and streaming it than kio-fuse ever could, so avoiding layers in between if possible is definitely an advantage.

Convert KIO URLs to KIOFuse URL is app is not KIO-enabled.

Chek for userInfo instead of just password

Check for password instead of authority

In D23384#570347, @ngraham wrote:

In D23384#570119, @feverfew wrote:

In D23384#570016, @ngraham wrote:

In D23384#569929, @feverfew wrote:

This isn't reaching KIOFuse at all. I believe this is related to this bug and that you've also blogged about:
https://bugs.kde.org/show_bug.cgi?id=330192
https://pointieststick.com/2018/01/17/videos-on-samba-shares/

This can be solved in this patch (and I did, although I removed it at Harald's request). I think I'll move it back in, and will leave it if it does solve this issue for you.

Great! Once it's back in, I'll re-test that.

Updated the diff. give it a go...
Also I assume that if it does work for you it resolves 330192?

Yay case #1 is fixed now! VLC and SMPlayer still unnecessarily download the URL locally like Totem does, but the large video file does at least open now when accesses from the non-FUSE location in Dolphin.

This seems like the best solution. From what I understand, not sending the credentials is a security feature, so doing it via KIOFuse is unavoidable

In D23384#570349, @ngraham wrote:

In D23384#570125, @dfaure wrote:

@ngraham AFAIK gnome has a trick where a fuse mount is created, its path is passed to the application being started, and the application, if it supports gvfs, re-translates that into a URL and uses that instead if it makes more sense. This way "dumb" apps get a local file (with all the limitations of doing synchronous I/O over the network) and network-transparent applications use URLs.
On the other hand, the KDE logic is "if the app takes %f and not %u in the Exec line, it doesn't support remote URLs, so we need to download the file first" (that's done by kioexec). If you see a "download first" check if kioexec is running. But if it's the app doing it, then I have no idea.

kioexec is not running during any of these lengthy downloads. Totem, VLC, and SMplayer all have %U in their desktop files, FWIW.

We may want to rethink this logic anyway. Any app that doesn't integrate with ioslaves should get the FUSE path rather than a locally-downloaded file, irrespective of whether or not its desktop file has %f or %u IMO. There are just too many disadvantages to the locally downloaded file approach, which is why we're doing this FUSE thing in the first place.

Perhaps the "lengthy local download first" issue is caused by not having https://invent.kde.org/kde/kio-fuse/merge_requests/2 yet.

In D23384#570016, @ngraham wrote:

In D23384#569929, @feverfew wrote:

This isn't reaching KIOFuse at all. I believe this is related to this bug and that you've also blogged about:
https://bugs.kde.org/show_bug.cgi?id=330192
https://pointieststick.com/2018/01/17/videos-on-samba-shares/

This can be solved in this patch (and I did, although I removed it at Harald's request). I think I'll move it back in, and will leave it if it does solve this issue for you.

Great! Once it's back in, I'll re-test that.

Updated the diff. give it a go...
Also I assume that if it does work for you it resolves 330192?

• Get rid of unnecessary copying of URLs
• Switch to interface generated at compile time
• Delete unused interfaces
• Merge branch 'master' into arcpatch-D23384
• remove kiofuseinterface from build
• Align arguments
• Only send original URLs to non-KIO apps if authority is empty

In D23384#569926, @ngraham wrote:

In D23384#569645, @fvogt wrote:

Issue #1:

That happens because the .desktop file sets X-KDE-Protocols=ftp,http,https,mms,rtmp,rtsp,sftp,smb:
https://code.videolan.org/videolan/vlc/blob/master/share/vlc.desktop.in#L124

I'm not sure about that. If I remove that line from VLC's installed desktop file, the problem still happens. And SMPlayer exhibits the same problem yet its desktop file doesn't define X-KDE-Protocols at all: https://sourceforge.net/p/smplayer/code/HEAD/tree/smplayer/trunk/smplayer.desktop

• Align arguments

• Get rid of unnecessary copying of URLs
• Switch to interface generated at compile time
• Delete unused interfaces
• Merge branch 'master' into arcpatch-D23384
• remove kiofuseinterface from build

• Align function arguments
• Remove hack
• Get rid of iterator

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

Don't use KIOFuse with mtp/gdrive

• Switch from KDED module to DBus service

On Linux appstream URLs don't work for me on Firefox Kubuntu 19.04. Should we really be using them if they're broken?

I won't review, but would like to note I think that with KIOFuse, in conjuction with fio we can do some really complex testing on KIO, correct me if I'm wrong.

@davidedmundson would be happy to work on this. Do you have the URL to the proposal on the mailing list?