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

[Alexander Saoutkin]

feverfew updated this revision to Diff 71543.
feverfew added a comment.


  - fix code style

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23384?vs=71423=71543

BRANCH
  arcpatch-D23384

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

AFFECTED FILES
  src/core/CMakeLists.txt
  src/core/desktopexecparser.cpp
  src/core/org.kde.KIOFuse.VFS.xml

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: alexde, broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, 
LeGast00n, GB_2, michaelh, bruns

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

[Alexander Saoutkin]

feverfew added a comment.


  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.
  
  
  @ngraham could you confirm if you have the credential issue with the current 
state of this patch. If you do, I'll simplify the logic seeming as there's no 
chance I can be "smart" about this here.

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: alexde, broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, 
LeGast00n, GB_2, michaelh, bruns

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

[David Faure]

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


  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.

INLINE COMMENTS

> desktopexecparser.cpp:329
> +} else if (!appSupportedProtocols.contains(QLatin1String("KIO"))) {
> +for (int i = 0; i < d->urls.length(); ++i)  {
> +const QUrl url = d->urls.at(i);

s/length/count/

> desktopexecparser.cpp:337
> +// @see https://bugs.kde.org/show_bug.cgi?id=330192
> +if(!supported || (!url.userName().isEmpty() && 
> url.password().isEmpty())) {
> +requests.push_back({ kiofuse_iface.mountUrl(url.toString()), 
> i });

coding style: missing space after "if"

> desktopexecparser.cpp:374
> + d->urls[request.urlIndex] = 
> QUrl::fromLocalFile(request.reply.value());
> + }
> +

indentation of those last 5 lines seems wrong (2 spaces instead of 4?)

> feverfew wrote in desktopexecparser.cpp:323
> One can always remove/uninstall kiofuse if it doesn't work or if they don't 
> really want the feature?

OK, that's a possible solution indeed.

> feverfew wrote in desktopexecparser.cpp:356
> In these methods I have to return the URLs within the function, so blocking 
> is inevitable. If I could return the URLs via a signal then blocking could be 
> avoided.

I'll try to keep this in mind when coming back to my KJob-based rewrite of KRun.
I keep postponing it because people keep changing KRun :-)

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: alexde, broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, 
LeGast00n, GB_2, michaelh, bruns

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

[Alexander Saoutkin]

feverfew marked 9 inline comments as done.
feverfew added a comment.


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

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: alexde, broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, 
LeGast00n, GB_2, michaelh, bruns

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

[Alexander Saoutkin]

feverfew added a comment.


  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.

INLINE COMMENTS

> feverfew wrote in desktopexecparser.cpp:331
> From what others have tested `userInfo()` is always empty even when the URL 
> is actually a protected Samba share... This means I can't know if a certain 
> URL is likely to have a password or not, I just have to assume it does. I'll 
> check again just in case, as indeed it does seem odd, but that was my 
> conclusion.

From my own testing your intuition seems correct. I've now amended my diff 
appropriately

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: alexde, broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, 
LeGast00n, GB_2, michaelh, bruns

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

[Alexander Saoutkin]

feverfew updated this revision to Diff 71423.
feverfew added a comment.


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

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23384?vs=70866=71423

BRANCH
  arcpatch-D23384

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

AFFECTED FILES
  src/core/CMakeLists.txt
  src/core/desktopexecparser.cpp
  src/core/org.kde.KIOFuse.VFS.xml

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: alexde, broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, 
LeGast00n, GB_2, michaelh, bruns

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

[Alexander Saoutkin]

feverfew added inline comments.

INLINE COMMENTS

> dfaure wrote in desktopexecparser.cpp:323
> Could we have a kill switch for KIOFuse if it fails to work properly in some 
> release?
> 
> E.g. some `export KIO_FUSE=0` or `export KIO_FUSE_DISABLE=1`.
> Or a KConfig entry (use `KSharedConfig::openConfig()->group("KIO")` for best 
> performance and flexibility -- then it can be disabled for one calling 
> application or for all)

One can always remove/uninstall kiofuse if it doesn't work or if they don't 
really want the feature?

> dfaure wrote in desktopexecparser.cpp:331
> The comment says "in case there is a password", the code doesn't.
> Probably historical, please fix :)
> 
> On this topic I saw earlier comments in the merge request which I found 
> confusing.
> It's very rare for an application to have the password stored inside the URL 
> itself. That would mean the user typing it in clear in a location bar or in a 
> terminal, bad idea. Instead, the user typically types a URL like 
> ftp://user@host/ and the kioslave asks for the password, and stores it in 
> kpasswdserver.
> 
> Also you wrote "we strip out the stuff that's in userInfo()". (where 
> QUrl::userInfo is username+password). But surely, while we might strip out 
> passwords for security reasons, we never strip out the username, do we?

From what others have tested `userInfo()` is always empty even when the URL is 
actually a protected Samba share... This means I can't know if a certain URL is 
likely to have a password or not, I just have to assume it does. I'll check 
again just in case, as indeed it does seem odd, but that was my conclusion.

> broulik wrote in desktopexecparser.cpp:356
> Blocking IO is worse than blocking DBus, still not nice, especially given the 
> "job"-like nature of `KRun` I would expect it to be fully async.

In these methods I have to return the URLs within the function, so blocking is 
inevitable. If I could return the URLs via a signal then blocking could be 
avoided.

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: alexde, broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, 
LeGast00n, GB_2, michaelh, bruns

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

[Alexander Saoutkin]

feverfew added a comment.


  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.
  
  
  Using git grep it seems to me that `resolveURLs` is called in 
`KRun::runApplicationImpl` and `KRun::runService` but 
`KIO::DesktopExecParser::resultingArguments` isn't called in those two 
functions. If URL conversion isn't necessary in those functions then I'll 
happily get rid of the code.

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: alexde, broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, 
LeGast00n, GB_2, michaelh, bruns

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

[David Faure]

dfaure added a comment.


  In D23384#571455 , @sitter wrote:
  
  > The opposite extreme is to always pass when X-KDE-Protocols is set and 
assume that the applications are actually working correctly (e.g. vlc ought to 
talk to kiod/KPasswdServerClient to get credentials, otherwise its declaration 
of X-KDE-Protocols is incorrect and you are looking at a bug in vlc. at the 
very least it should throw up its own auth dialog if it doesn't know what to 
do).
  
  
  There's a mistake in this reasoning. The "KDE" in X-KDE-Protocols only means 
that this is a KDE-specific field in desktop files, it wasn't standardized.
  It doesn't however mean that these protocols are implemented using KIO.
  In fact KIO-based apps normally just set X-KDE-Protocols=KIO. Other values 
for that field (i.e. an actual list of protocols) *is* what other apps are 
supposed to use there.
  
  But yes, this doesn't solve the credentials issue.
  I like the suggestion in another comment to implement that in 
SecretService... (which could replace kpasswdserver then, at least for the 
storage part, only the GUI would remain).

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: alexde, broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, 
LeGast00n, GB_2, michaelh, bruns

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

[Alexander Saoutkin]

feverfew added a comment.


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

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: alexde, broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, 
LeGast00n, GB_2, michaelh, bruns

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

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> dfaure wrote in desktopexecparser.cpp:356
> This blocks.
> 
> I don't mind much myself, but I know some people had a mandate to remove as 
> many blocking calls as possible from KRun and related code. Or was that only 
> avoiding blocking I/O because of network mounts? [which this patch is all 
> about adding more of...]. @broulik ?

Blocking IO is worse than blocking DBus, still not nice, especially given the 
"job"-like nature of `KRun` I would expect it to be fully async.

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: alexde, broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, 
LeGast00n, GB_2, michaelh, bruns

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

[David Faure]

dfaure added a comment.


  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.

INLINE COMMENTS

> desktopexecparser.cpp:311
>  
>  // Check if we need kioexec
>  bool useKioexec = false;

`, or KIOFuse`

> desktopexecparser.cpp:317
> +struct MountRequest { QDBusPendingReply reply; int urlIndex; };
> +QVector requests;
>  if (!mx1.hasUrls) {

`requests.reserve(d->urls.count());`

> desktopexecparser.cpp:319
>  if (!mx1.hasUrls) {
> -for (const QUrl  : qAsConst(d->urls)) {
> +for (int i = 0; i < d->urls.length(); ++i)  {
> +const QUrl url = d->urls[i];

Minor: can you use count() (Qt-like) or size() (STL-like)? In 20 years I never 
saw code that uses length() for a vector or a list, it's just very unusual in 
Qt code :-)

[occurs twice]

> desktopexecparser.cpp:320
> +for (int i = 0; i < d->urls.length(); ++i)  {
> +const QUrl url = d->urls[i];
>  if (!url.isLocalFile() && !hasSchemeHandler(url)) {

Use d->urls.at(i) to avoid a detach given that d->urls isn't const here.

[occurs twice]

> desktopexecparser.cpp:323
> +// Lets try a KIOFuse mount instead.
> +requests.push_back({ kiofuse_iface.mountUrl(url.toString()), 
> i });
>  }

Could we have a kill switch for KIOFuse if it fails to work properly in some 
release?

E.g. some `export KIO_FUSE=0` or `export KIO_FUSE_DISABLE=1`.
Or a KConfig entry (use `KSharedConfig::openConfig()->group("KIO")` for best 
performance and flexibility -- then it can be disabled for one calling 
application or for all)

> desktopexecparser.cpp:330
> +// NOTE: Some non-KIO apps may support the URLs (e.g. VLC 
> supports smb://)
> +// but will struggle with passwords.
> +// Hence convert URL to KIOFuse equivalent in case there is a 
> password.

"Struggle" sounds like the apps are limited/buggy.

But if what you mean is that the password (e.g. stored in kpasswdserver after 
the user typed it in the KDE dialog asking for it) won't be available to 
non-KIO applications, that seems logical and I do accept that argument. It's 
just the writing that is surprising.

> desktopexecparser.cpp:331
> +// but will struggle with passwords.
> +// Hence convert URL to KIOFuse equivalent in case there is a 
> password.
> +// @see 
> https://pointieststick.com/2018/01/17/videos-on-samba-shares/

The comment says "in case there is a password", the code doesn't.
Probably historical, please fix :)

On this topic I saw earlier comments in the merge request which I found 
confusing.
It's very rare for an application to have the password stored inside the URL 
itself. That would mean the user typing it in clear in a location bar or in a 
terminal, bad idea. Instead, the user typically types a URL like 
ftp://user@host/ and the kioslave asks for the password, and stores it in 
kpasswdserver.

Also you wrote "we strip out the stuff that's in userInfo()". (where 
QUrl::userInfo is username+password). But surely, while we might strip out 
passwords for security reasons, we never strip out the username, do we?

> desktopexecparser.cpp:348
> +// Lets try a KIOFuse mount instead.
> +requests.push_back({ kiofuse_iface.mountUrl(url.toString()), 
> i });
>  }

This duplicates line 344.

`if (a) { if (!b) { foo } } else { foo }`
is equivalent to
`if (!a || !b) { foo }`

In your case `a` is `http || https` which makes this a big uglier, but you 
could use what other pieces of code do and write 
url.scheme().startsWith(QLatin1String("http")).

So this becomes `if (!url.scheme().startsWith(QLatin1String("http")) || 
!supportedProtocols(d->service).contains(url.scheme())) {`

> desktopexecparser.cpp:355
> +// Main thing that we want is to send the mount requests without 
> blocking.
> +for (auto request : requests) {
> +request.reply.waitForFinished();

`auto ` to avoid copying

> desktopexecparser.cpp:356
> +for (auto request : requests) {
> +request.reply.waitForFinished();
> +if (request.reply.isError()) {

This blocks.

I don't mind much myself, but I know some people had a mandate to remove as 
many blocking calls as possible from KRun and related code. Or was that only 
avoiding blocking I/O because of network mounts? [which this patch is all about 
adding more of...]. @broulik ?

> desktopexecparser.cpp:378
>  return result;
> +} else {
> +// At this point we know we're not using kioexec, so feel free to 
> replace

This nesting under `else` is unnecessary given the 

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

[Nathaniel Graham]

ngraham added a comment.


  In fact both issues I found seem to be caused by !2. See 
https://invent.kde.org/kde/kio-fuse/merge_requests/2#note_19771. We can 
continue the discussion there.

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: alexde, broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, 
LeGast00n, GB_2, michaelh, bruns

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

[Nathaniel Graham]

ngraham added a comment.


  Sure, I'll file bugs in 
https://bugs.kde.org/describecomponents.cgi?product=kiofuse.

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: alexde, broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, 
LeGast00n, GB_2, michaelh, bruns

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

[Nathaniel Graham]

ngraham added a comment.


  Anyway I guess this patch itself is fine now and the remaining issues are 
elsewhere. :)

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: alexde, broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, 
LeGast00n, GB_2, michaelh, bruns

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

[Alexander Saoutkin]

feverfew added a comment.


  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.
  
  In particular, if you could test writing with both master and FileJobCombo 
(!2, i.e. the seeking MR) and tell us what happens that would be great. What 
slave were you testing? Does this also occur if you mount something from the 
file slave (e.g. file:///home/nate/image.xcf); instructions for mounting 
without this differential are available in the README of the KIOFuse repo. A 
log for both branches would be great (as you did earlier following fvogt's 
instructions). A reply to all of this in the relevant issue tracker or MR would 
be great. Thanks.

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: alexde, broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, 
LeGast00n, GB_2, michaelh, bruns

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

[Nathaniel Graham]

ngraham added a comment.


  In D23384#572199 , @fvogt wrote:
  
  > Was that with or without the KIO::open merge request?
  
  
  With.

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: alexde, broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, 
LeGast00n, GB_2, michaelh, bruns

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

[Fabian Vogt]

fvogt added a comment.


  In D23384#572198 , @ngraham wrote:
  
  > I just tested writing today, for files opened in 3rd-party apps that get 
the FUSE mount path. Results:
  >  ...
  
  
  Was that with or without the KIO::open merge request?

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: alexde, broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, 
LeGast00n, GB_2, michaelh, bruns

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

[Nathaniel Graham]

ngraham added a comment.


  I just tested writing today, for files opened in 3rd-party apps that get the 
FUSE mount path. Results:
  
  - Gedit: everything works
  - Blender: everything works
  - Inkscape: though the file is very small, the save operation is somewhat 
slow, but everything works
  - GIMP, File > Save: save operation hangs forever
  - GIMP, file> File > Overwrite: save operation is slow but completes. 
However, the overwritten file is corrputed. Here's the corrupt file: F7799144: 
Fractured corrupted.zip 
  
  The slow speed is maybe something worth investigating, but these could all be 
app-specific issues (especially GIMP).

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: alexde, broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, 
LeGast00n, GB_2, michaelh, bruns

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

[Kai Uwe Broulik]

broulik added a comment.


  Can you please move the DBus XML file in a common location

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: alexde, broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, 
LeGast00n, GB_2, michaelh, bruns

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

[David Edmundson]

davidedmundson added a comment.


  If something is ready for full code review, can we remove "WIP:" from the 
title.

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: alexde, broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, 
LeGast00n, GB_2, michaelh, bruns

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

[David Faure]

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


  OK, please give me a day or two to review this, it shouldn't go in before 
Saturday anyway so we have one month of testing in git before release

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: alexde, broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, 
LeGast00n, GB_2, michaelh, bruns

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

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a comment.
This revision is now accepted and ready to land.


  All issues are fixed now for me! It works perfectly!

REPOSITORY
  R241 KIO

BRANCH
  arcpatch-D23384

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, 
GB_2, michaelh, bruns

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

[Alexander Saoutkin]

feverfew edited the summary of this revision.

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, 
GB_2, michaelh, bruns

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

[Alexander Saoutkin]

feverfew updated this revision to Diff 70866.
feverfew added a comment.


  don't send http(s) to kioexec

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23384?vs=70830=70866

BRANCH
  arcpatch-D23384

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

AFFECTED FILES
  src/core/CMakeLists.txt
  src/core/desktopexecparser.cpp
  src/core/org.kde.KIOFuse.VFS.xml
  src/widgets/CMakeLists.txt
  src/widgets/krun.cpp
  src/widgets/org.kde.KIOFuse.VFS.xml

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, 
GB_2, michaelh, bruns

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

[Nathaniel Graham]

ngraham added a comment.


  Thanks for that very clear and understandable synopsis. I'll let others 
figure out the best path forward. :)

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, 
GB_2, michaelh, bruns

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

[Alexander Saoutkin]

feverfew added a comment.


  So before this patch we had the following behaviour:
  
  1. If app is KIO-enabled, pass URLs unchanged.
  2. else for each URL, if app claims to support the protocol pass the original 
URL, otherwise change the URL to the corresponding KIOExec path.
  
  This causes a problem as noted in 330192 
 and this blog post 
. In particular, 
if a non-KIO claims to support a certain protocol (e.g., VLC supports smb), 
then we won't convert to a KIOExec path, but we strip out the stuff that's in 
`userInfo()` (credentials) before sending it over.
  
  What I previously tried to do was the following, in an attempt to close:
  
  1. If app is KIO-enabled, pass URLs unchanged.
  2. else for each URL, if app claims to support the protocol **AND**  
`userInfo()` is empty pass the original URL, otherwise change the URL to the 
corresponding KIOExec path.
  
  However, that doesn't really help as apparently even if, say, a samba share 
is password protected, at the point I'm checking the password in 
`resolveURLs()` or `resultingArguments()` (truth be told, I'm not sure which, 
if not both, codepaths are being hit in our current testing), `userInfo()` is 
always empty, so this optimisation isn't possible.
  
  So then one could just always direct non-KIO apps to use KIOFuse. Well that 
causes another problem, what about stuff like http(s)? It doesn't really make 
sense as a filesystem and we'd definitely like to forward it to the browser. So 
currently I can special case http/https. but is that the only one we want to 
special case?
  
  Ideally, I'd like to know if there is a place where `userInfo` is only empty 
when there is genuinely no user/pass combo needed to access the resource at the 
URL. If so, then it can actually be possible to use KIOFuse on apps that 
support a URL only if it needs to have credentials (which it won't have) such 
that 330192 can be closed, without needing any hacks.

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, 
GB_2, michaelh, bruns

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

[Nathaniel Graham]

ngraham added a comment.


  Okay, that fixed it for the case of opening files in non-KDE apps on my Samba 
share. Now they open from the FUSE path--all of them! So this is good.
  
  However now kioexec downloads http URLs opened from other apps instead of 
just passing them off to the browser. :(

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, 
GB_2, michaelh, bruns

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

[Alexander Saoutkin]

feverfew updated this revision to Diff 70830.
feverfew added a comment.


  ignore scheme handler

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23384?vs=70823=70830

BRANCH
  arcpatch-D23384

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

AFFECTED FILES
  src/core/CMakeLists.txt
  src/core/desktopexecparser.cpp
  src/core/org.kde.KIOFuse.VFS.xml
  src/widgets/CMakeLists.txt
  src/widgets/krun.cpp
  src/widgets/org.kde.KIOFuse.VFS.xml

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, 
GB_2, michaelh, bruns

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

[Nathaniel Graham]

ngraham added a comment.


  With the latest version of this patch, now every file always gets downloaded 
through kioexec on my password-protected Samba share; nothing ever gets the 
kio-fuse path. In fact the share never gets mounted at all when I try to open 
something on the share in a non-KDE app.

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, 
GB_2, michaelh, bruns

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

[Alexander Saoutkin]

feverfew added a comment.


  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.
  
  
  If the URL is protected then we have to pass it to KIOFuse as most apps don't 
know how to get the credentials from kpasswdserver. This is a bug that has 
always existed with KIOExec, where before this patch it would always pass on 
the original URL to non-KIO apps if they understood them, but would have the 
same issue with credentials being required. From what I understand, 
`userInfo()` always seems to be emtpy even if it is password protected, so 
there's nothing clever I can do where if it isn't password protected I can just 
pass it straight to the app. It looks like we'll simply have to send the 
KIOFuse URL to all non-KIO apps. Trying to optimise I think is out of scope for 
this patch. I've blacklisted http/https seeming as they don't make much sense 
as a filesystem.

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, 
GB_2, michaelh, bruns

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

[Alexander Saoutkin]

feverfew updated this revision to Diff 70823.
feverfew added a comment.


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

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23384?vs=70799=70823

BRANCH
  arcpatch-D23384

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

AFFECTED FILES
  src/core/CMakeLists.txt
  src/core/desktopexecparser.cpp
  src/core/org.kde.KIOFuse.VFS.xml
  src/widgets/CMakeLists.txt
  src/widgets/krun.cpp
  src/widgets/org.kde.KIOFuse.VFS.xml

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, 
GB_2, michaelh, bruns

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

[Harald Sitter]

sitter added a comment.


  I haven't read everything in great detail... but...
  
  Quick braindump of musing I did elsewhere: as far as credential hand-over is 
concerned this is likely a problem that needs a workaround for now as there is 
no clear cut solution that I am aware of. It may be worth talking to gnome 
about this and come up with an xdg standard for this.
  
  The trouble is: we can't pass credentials in the URL as that'd be an exec 
argument and that would by default leak into /proc where the credential is then 
world readable. What we need is an additional system to pass credentials.
  
  IMHO what likely is the smartest choice to is to have credentials stored in a 
running Secret Service API daemon and have clients then get the credentials out 
of there. Possibly with some additional ephemeral storage type (e.g. we cache 
credentials in kiod but store them in kwallet currently, that gives two points 
where we can leak secrets. if we were to move everything into the secret 
service we'd have a single service that needs securing). Just my opinion on the 
matter.
  
  Until we have a cross desktop spec defining the passing of credentials I 
imagine we can't really let applications handle URLs for which we have/need 
credentials and I suspect determining if we need credentials is also a bit 
tricky.
  The opposite extreme is to always pass when X-KDE-Protocols is set and assume 
that the applications are actually working correctly (e.g. vlc ought to talk to 
kiod/KPasswdServerClient to get credentials, otherwise its declaration of 
X-KDE-Protocols is incorrect and you are looking at a bug in vlc. at the very 
least it should throw up its own auth dialog if it doesn't know what to do).

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, 
GB_2, michaelh, bruns

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

[Fabian Vogt]

fvogt added a comment.


  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.

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, 
GB_2, michaelh, bruns

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

[Nathaniel Graham]

ngraham added a comment.


  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. My impression is that the few 
apps that have implemented their own samba clients and the like did so mostly 
to work around the lack of this feature.

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, 
GB_2, michaelh, bruns

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

[Alexander Saoutkin]

feverfew updated this revision to Diff 70799.
feverfew added a comment.


  Chek for userInfo instead of just password

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23384?vs=70773=70799

BRANCH
  arcpatch-D23384

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

AFFECTED FILES
  src/core/CMakeLists.txt
  src/core/desktopexecparser.cpp
  src/core/org.kde.KIOFuse.VFS.xml
  src/widgets/CMakeLists.txt
  src/widgets/krun.cpp
  src/widgets/org.kde.KIOFuse.VFS.xml

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, 
GB_2, michaelh, bruns

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

[Nathaniel Graham]

ngraham added a comment.


  This latest change made Totem work, but broke VLC and SMplayer for my 
password-protected Samba share. See the comment I left on 
https://invent.kde.org/kde/kio-fuse/merge_requests/2#note_19607.

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, 
GB_2, michaelh, bruns

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

[Alexander Saoutkin]

feverfew updated this revision to Diff 70773.
feverfew added a comment.


  Check for password instead of authority

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23384?vs=70661=70773

BRANCH
  arcpatch-D23384

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

AFFECTED FILES
  src/core/CMakeLists.txt
  src/core/desktopexecparser.cpp
  src/core/org.kde.KIOFuse.VFS.xml
  src/widgets/CMakeLists.txt
  src/widgets/krun.cpp
  src/widgets/org.kde.KIOFuse.VFS.xml

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, 
GB_2, michaelh, bruns

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

[Nathaniel Graham]

ngraham added a comment.


  @feverfew thanks for rebasing 
https://invent.kde.org/kde/kio-fuse/merge_requests/2. I've tested it and 
confirmed that it fixes case #2! We can continue that portion of the view in 
the comments there.

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, 
GB_2, michaelh, bruns

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

[Fabian Vogt]

fvogt added a comment.


  In D23384#570830 , @ngraham wrote:
  
  > In D23384#570735 , @fvogt wrote:
  >
  > > > Clicking a http link in my chat app now kiofuses it and then has the 
browser desperately try to open /run/user/1000/kio-fuse-bla/http/kde.org/ - it 
only gets the first character in the file, so I usually just see < for a HTML 
page
  > >
  > > Ignoring the elephant in the room which is that this diff forces 
everything through `mountUrl`, that's the expected behavior with a plain HTTP 
URL as the size isn't known until the file is downloaded. So `stat` reports a 
size of `1` until the file is actually opened.
  > >  This is unavoidable, otherwise every `ls` would trigger a download of 
all files. If handling this better is important, `HTTPProtocol::stat` could use 
a `HEAD` request to get the `Content-Length`, but that doesn't work in all 
cases either.
  >
  >
  > Well, we need to fix this or else it's a very serious regression that 
breaks a huge part of the desktop. Opening links in a web browser is pretty 
basic functionality.
  
  
  That issue is not in kio-fuse:
  
  > Ignoring the elephant in the room which is that this diff forces everything 
through `mountUrl`, ...

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, 
GB_2, michaelh, bruns

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

[Nathaniel Graham]

ngraham added a comment.


  In D23384#570735 , @fvogt wrote:
  
  > > Clicking a http link in my chat app now kiofuses it and then has the 
browser desperately try to open /run/user/1000/kio-fuse-bla/http/kde.org/ - it 
only gets the first character in the file, so I usually just see < for a HTML 
page
  >
  > Ignoring the elephant in the room which is that this diff forces everything 
through `mountUrl`, that's the expected behavior with a plain HTTP URL as the 
size isn't known until the file is downloaded. So `stat` reports a size of `1` 
until the file is actually opened.
  >  This is unavoidable, otherwise every `ls` would trigger a download of all 
files. If handling this better is important, `HTTPProtocol::stat` could use a 
`HEAD` request to get the `Content-Length`, but that doesn't work in all cases 
either.
  
  
  Well, we need to fix this or else it's a very serious regression that breaks 
a huge part of the desktop. Opening links in a web browser is pretty basic 
functionality.

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, 
GB_2, michaelh, bruns

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

[Fabian Vogt]

fvogt added a comment.


  In D23384#570331 , @ngraham wrote:
  
  > In D23384#570118 , @fvogt wrote:
  >
  > > Please try both of the following:
  >
  >
  > Done. Here are the log files:
  >
  > F7793378: kio-fuse.log 
  
  
  Just as expected:
  
unique: 4248, opcode: READ (15), nodeid: 13, insize: 80, pid: 24522
org.kde.kio.fuse: Read "Dean plays with Winnie.mov" 0 65536
org.kde.kio.fuse: Fetching cache for "Dean plays with Winnie.mov"
kf5.kio.core: KLocalSocket(0x212cfd0) Jumbo packet of 65534 bytes
kf5.kio.core: KLocalSocket(0x212cfd0) Jumbo packet of 65534 bytes
   unique: 4248, success, outsize: 65552
kf5.kio.core: KLocalSocket(0x212cfd0) Jumbo packet of 65534 bytes
unique: 4250, opcode: GETATTR (3), nodeid: 13, insize: 56, pid: 24522
   unique: 4250, success, outsize: 120
unique: 4252, opcode: READ (15), nodeid: 13, insize: 80, pid: 24522
org.kde.kio.fuse: Read "Dean plays with Winnie.mov" 153833472 36864
  
  It first reads the header and then jumps to the file's end to read a few KiB 
there.
  Nothing kio-fuse can do about that, except trying to use `KIO:open` in the 
future, but that has some other drawbacks...
  
  > F7793379: strace output.log 

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, 
GB_2, michaelh, bruns

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

[Fabian Vogt]

fvogt added a comment.


  > Clicking a http link in my chat app now kiofuses it and then has the 
browser desperately try to open /run/user/1000/kio-fuse-bla/http/kde.org/ - it 
only gets the first character in the file, so I usually just see < for a HTML 
page
  
  Ignoring the elephant in the room which is that this diff forces everything 
through `mountUrl`, that's the expected behavior with a plain HTTP URL as the 
size isn't known until the file is downloaded. So `stat` reports a size of `1` 
until the file is actually opened.
  This is unavoidable, otherwise every `ls` would trigger a download of all 
files. If handling this better is important, `HTTPProtocol::stat` could use a 
`HEAD` request to get the `Content-Length`, but that doesn't work in all cases 
either.

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, 
GB_2, michaelh, bruns

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

[Kai Uwe Broulik]

broulik added a comment.


  It appears to open everything remote through kiofuse.
  
  - Click a text file in `sftp://foo/bar` will kiofuse it and then open it in 
Kate
  - Clicking a http link in my chat app now kiofuses it and then has the 
browser desperately try to open `/run/user/1000/kio-fuse-bla/http/kde.org/` - 
it only gets the first character in the file, so I usually just see `<` for a 
HTML page
  - `xdg-open sftp://foo/bar` will kiofuse it and then open Dolphin pointing to 
`/run/user/1000/kio-fuse-bla/sftp/foo/bar` instead of opening it normally

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, 
GB_2, michaelh, bruns

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

[Alexander Saoutkin]

feverfew added a comment.


  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

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

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

[Alexander Saoutkin]

feverfew added a comment.


  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.
  
  
  Yes, that's precisely why. !2 
 implements seeking to 
any point in the file without having to download all of it. I'll rebase all of 
it such that you can test all of it easily tomorrow.

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

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

[Nathaniel Graham]

ngraham added a comment.


  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.

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

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

[Nathaniel Graham]

ngraham added a comment.


  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.
  
  > Also this patch doesn't seem to solve 398446? I'd downgrade that to a 
CCBUG. A fix can be done via the Solid repo though to display the mount in 
Places. I'll create a patch when appropriate.
  
  I guess it doesn't //fully// fix 398446. It mostly does since the open or 
save dialog from the app will still begin in the FUSE location so you can see 
nearby files or Save As to a new file in the same location. But if you navigate 
away from that location, it will be next to impossible for a regular user to 
find again, so we would want to expose the mount in the file dialog for 
completeness' sake, yeah.
  
  > That's just an annoying issue locally, which is resolved by that hack. On 
the system level this won't be a problem at all.
  
  Ok good.
  
  > Also, pls put debug output into an attachment in the future (and probably 
edit your comment above), it's now quite hard to scroll through this diff.
  
  Done, sorry about that.
  
  >>> I'm a bit confused. Which one of the three are you doing:
  >>> 
  >>> 1. Opening a KIO Url via Dolphin.
  >>> 2. Opening a KIOFuse mount local URL via Dolphin.
  >>> 3. Opening a KIOFuse URL via the media players file picker.
  >> 
  >> #2.
  > 
  > So as in, you're Dolphin is browsing inside 
`/run/user//kio-fuse-XX/some/location` and you open a file from there?
  
  Yes. testing further, I am able to reproduce this dolphin hang when I 
right-click > open in > Totem, but not in VLC or SMPlayer.

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

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

[Nathaniel Graham]

ngraham added a comment.


  In D23384#570118 , @fvogt wrote:
  
  > Please try both of the following:
  
  
  Done. Here are the log files:
  
  F7793378: kio-fuse.log 
  
  F7793379: strace output.log 

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

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

[David Faure]

dfaure added a comment.


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

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

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

[Alexander Saoutkin]

feverfew added a comment.


  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...
  
  >> You must be doing something wrong. FUSE provides output of all calls that 
it receives (and how kio-fuse responds). Let me write the instructions more 
verbosely for CLI to make sure no steps are missed:
  >>  `$ killall kio-fuse`
  >>  `$ export QT_LOGGING_RULES="*.debug=true"`
  >>  `$ kio-fuse $myFavLocation -d &> ~/kio-fuse-debug.txt` (I recommend 
`$myFabLocation` isn't empty if you haven't installed the 
`kio-fuse-tmpfiles.conf` exclusion file at the system level.)
  >> 
  >> Then open the file in the media player. Once your done simply attach 
`~/kio-fuse-debug.txt` here.
  >>  It's really important for us to know what kind of read requests are being 
called to help us diagnose the issue.
  > 
  > I think I found part of the problem. When I follow these instructions, the 
log file has only a single line:
  > 
  >   kio-fuse: command not found
  > 
  > 
  > This is because the path it is installed to (`~/kde/usr/lib64/libexec/`) is 
not in `$PATH`. When I add that location to `$PATH` and try again, the log file 
contains the following:
  > 
  > It should probably not assume that `/usr/lib64/libexec/` is in 
`$PATH`, at the minimum.
  
  That's just an annoying issue locally, which is resolved by that hack. On the 
system level this won't be a problem at all.
  
  Also, pls put debug output into an attachment in the future (and probably 
edit your comment above), it's now quite hard to scroll through this diff.
  
  > 
  > 
  >> I'm a bit confused. Which one of the three are you doing:
  >> 
  >> 1. Opening a KIO Url via Dolphin.
  >> 2. Opening a KIOFuse mount local URL via Dolphin.
  >> 3. Opening a KIOFuse URL via the media players file picker.
  > 
  > #2.
  
  So as in, you're Dolphin is browsing inside 
`/run/user//kio-fuse-XX/some/location` and you open a file from there?

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

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

[Fabian Vogt]

fvogt added a comment.


  Unfortunately the `kio-fuse -d` output is incomplete, probably because Qt was 
too smart and logged to the journal instead...
  It's visible that there are multiple processes reading the file, maybe 
thumbnailing is in progress?
  Can you try with thumbnails in dolphin disabled?
  
  Please try both of the following:
  
  Add this to kio-fuse and rebuild:
  
diff --git a/kiofusevfs.cpp b/kiofusevfs.cpp
index 7755b56..a3a4c72 100644
--- a/kiofusevfs.cpp
+++ b/kiofusevfs.cpp
@@ -859,6 +859,7 @@ void KIOFuseVFS::read(fuse_req_t req, fuse_ino_t ino, 
size_t size, off_t off, fu
return;
case KIOFuseNode::NodeType::RemoteFileNode:
{
+   qDebug(KIOFUSE_LOG) << "Read" << node->m_nodeName << off << 
size;
auto remoteNode = 
std::dynamic_pointer_cast(node);
that->awaitBytesAvailable(remoteNode, off + size, [=](int 
error) {
if(error != 0 && error != ESPIPE)
  
  then
  
QT_LOGGING_RULES=*.debug=true QT_FORCE_STDERR_LOGGING=1 
~/kde/usr/lib64/libexec/kio-fuse -d $yourfavlocation &>somelog.file
  
  and to get just the syscalls which totem makes:
  
strace -fefile totem $yourfavlocation/where/the/video.is

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

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

[Alexander Saoutkin]

feverfew updated this revision to Diff 70661.
feverfew added a comment.


  - 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

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23384?vs=70657=70661

BRANCH
  arcpatch-D23384

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

AFFECTED FILES
  src/core/CMakeLists.txt
  src/core/desktopexecparser.cpp
  src/core/org.kde.KIOFuse.VFS.xml
  src/widgets/CMakeLists.txt
  src/widgets/krun.cpp
  src/widgets/org.kde.KIOFuse.VFS.xml

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: sitter, davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, GB_2, 
michaelh, bruns

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

[Alexander Saoutkin]

feverfew reopened this revision.

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

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

[Alexander Saoutkin]

This revision was not accepted when it landed; it landed in state "Needs 
Revision".
This revision was automatically updated to reflect the committed changes.
Closed by commit R241:92c2886c3714: [WIP] Adding support for mounting KIOFuse 
URLs for applications that dont use… (authored by feverfew).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D23384?vs=69788=70657#toc

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23384?vs=69788=70657

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

AFFECTED FILES
  src/core/CMakeLists.txt
  src/core/desktopexecparser.cpp
  src/core/kiofuseinterface.cpp
  src/core/kiofuseinterface.h
  src/widgets/krun.cpp

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: sitter, davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, GB_2, 
michaelh, bruns

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

[Nathaniel Graham]

ngraham added a comment.


  I just found that Totem has a "Show Containing Folder" feature. If I use it, 
it shows me that the video file is in fact being opened from the FUSE mount 
path! So that's good. But it still insists on downloading the whole thing 
locally before it starts to play it. When Totem opens a video file from a 
Nautilus/GVfs-created FUSE path, it doesn't do this and starts playing the file 
instantly.

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

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

[Nathaniel Graham]

ngraham added a comment.


  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.
  
  > You must be doing something wrong. FUSE provides output of all calls that 
it receives (and how kio-fuse responds). Let me write the instructions more 
verbosely for CLI to make sure no steps are missed:
  >  `$ killall kio-fuse`
  >  `$ export QT_LOGGING_RULES="*.debug=true"`
  >  `$ kio-fuse $myFavLocation -d &> ~/kio-fuse-debug.txt` (I recommend 
`$myFabLocation` isn't empty if you haven't installed the 
`kio-fuse-tmpfiles.conf` exclusion file at the system level.)
  > 
  > Then open the file in the media player. Once your done simply attach 
`~/kio-fuse-debug.txt` here.
  >  It's really important for us to know what kind of read requests are being 
called to help us diagnose the issue.
  
  I think I found part of the problem. When I follow these instructions, the 
log file has only a single line:
  
kio-fuse: command not found
  
  This is because the path it is installed to (`~/kde/usr/lib64/libexec/`) is 
not in `$PATH`. When I add that location to `$PATH` and try again, the log file 
contains the following:
  
FUSE library version: 3.8.0
unique: 2, opcode: INIT (26), nodeid: 0, insize: 56, pid: 0
INIT: 7.31
flags=0x03fb
max_readahead=0x0002
   INIT: 7.31
   flags=0x00409021
   max_readahead=0x0002
   max_write=0x0010
   max_background=0
   congestion_threshold=0
   time_gran=10
   unique: 2, success, outsize: 80
unique: 4, opcode: ACCESS (34), nodeid: 1, insize: 48, pid: 7327
   unique: 4, error: -38 (Function not implemented), outsize: 16
unique: 6, opcode: LOOKUP (1), nodeid: 1, insize: 47, pid: 7327
   unique: 6, error: -2 (No such file or directory), outsize: 16
unique: 8, opcode: LOOKUP (1), nodeid: 1, insize: 52, pid: 7327
   unique: 8, error: -2 (No such file or directory), outsize: 16
unique: 10, opcode: LOOKUP (1), nodeid: 1, insize: 44, pid: 10077
   unique: 10, success, outsize: 144
unique: 12, opcode: LOOKUP (1), nodeid: 3, insize: 62, pid: 10077
   unique: 12, success, outsize: 144
unique: 14, opcode: LOOKUP (1), nodeid: 4, insize: 46, pid: 10077
   unique: 14, success, outsize: 144
unique: 16, opcode: LOOKUP (1), nodeid: 5, insize: 47, pid: 10077
   unique: 16, success, outsize: 144
unique: 18, opcode: LOOKUP (1), nodeid: 6, insize: 48, pid: 10077
   unique: 18, success, outsize: 144
unique: 20, opcode: LOOKUP (1), nodeid: 7, insize: 62, pid: 10077
   unique: 20, success, outsize: 144
unique: 22, opcode: OPEN (14), nodeid: 8, insize: 48, pid: 10077
   unique: 22, success, outsize: 32
unique: 24, opcode: FLUSH (25), nodeid: 8, insize: 64, pid: 10077
   unique: 24, success, outsize: 16
unique: 26, opcode: RELEASE (18), nodeid: 8, insize: 64, pid: 0
   unique: 26, success, outsize: 16
unique: 28, opcode: OPEN (14), nodeid: 8, insize: 48, pid: 10077
   unique: 28, success, outsize: 32
unique: 30, opcode: FLUSH (25), nodeid: 8, insize: 64, pid: 10077
   unique: 30, success, outsize: 16
unique: 32, opcode: RELEASE (18), nodeid: 8, insize: 64, pid: 0
   unique: 32, success, outsize: 16
unique: 34, opcode: OPEN (14), nodeid: 8, insize: 48, pid: 10077
   unique: 34, success, outsize: 32
unique: 36, opcode: GETATTR (3), nodeid: 1, insize: 56, pid: 10077
   unique: 36, success, outsize: 120
unique: 38, opcode: GETATTR (3), nodeid: 1, insize: 56, pid: 10084
   unique: 38, success, outsize: 120
unique: 40, opcode: READ (15), nodeid: 8, insize: 80, pid: 10088
   unique: 40, success, outsize: 16400
unique: 42, opcode: READ (15), nodeid: 8, insize: 80, pid: 10091
unique: 44, opcode: LOOKUP (1), nodeid: 1, insize: 44, pid: 31993
   unique: 44, success, outsize: 144
unique: 46, opcode: LOOKUP (1), nodeid: 3, insize: 62, pid: 31993
   unique: 46, success, outsize: 144
unique: 48, opcode: LOOKUP (1), nodeid: 4, insize: 46, pid: 31993
   unique: 48, success, outsize: 144
unique: 50, opcode: LOOKUP (1), nodeid: 5, insize: 47, pid: 31993
   unique: 50, success, outsize: 144
unique: 52, opcode: LOOKUP (1), nodeid: 6, insize: 48, pid: 31993
   unique: 52, success, outsize: 144
unique: 54, opcode: LOOKUP (1), nodeid: 7, insize: 62, pid: 31993
   unique: 54, success, outsize: 144
unique: 56, opcode: GETATTR (3), nodeid: 1, insize: 56, pid: 31993
   

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

[Alexander Saoutkin]

feverfew added a comment.


  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
  
  
  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.
  
  >> **Issue #2:**
  >> 
  >> The only explanation I have for that is that totem for some reason starts 
reading the file from the end.
  >>  If you start `kio-fuse -d` manually (kill the other kio-fuse process 
first) and then use totem again, what's the debug output
  > 
  > There is no debug output, and the whole file is downloaded locally as with 
the other apps. It seems like Totem is not getting the FUSE mount path; it gets 
an `smb://` URL and it or KIO downloads the file normally. Again, this happens 
even if I open the file from the FUSE mount path itself.
  > 
  > I can confirm that the FUSE path handoff is working properly. For example 
if I open `smb://gaston@living-room-pc/Users/Gaston/Desktop/Shep Face Code.txt` 
in Gedit, Gedit displays the file's path as follows: F7791491: 
Screenshot_20191130_133857.png 
  
  You must be doing something wrong. FUSE provides output of all calls that it 
receives (and how kio-fuse responds). Let me write the instructions more 
verbosely for CLI to make sure no steps are missed:
  `$ killall kio-fuse`
  `$ export QT_LOGGING_RULES="*.debug=true"
  `$ kio-fuse $myFavLocation -d &> ~/kio-fuse-debug.txt` (I recommend 
`$myFabLocation` isn't empty if you haven't installed the 
`kio-fuse-tmpfiles.conf` exclusion file at the system level.)
  
  Then open the file in the media player. Once your done simply attach 
`~/kio-fuse-debug.txt` here.
  It's really important for us to know what kind of read requests are being 
called to help us diagnose the issue.
  
  >> **Issue #3:**
  >> 
  >> Everything is in a separate process and async already. When/how does it 
happen?
  > 
  > When I open a large video file from the hidden mount path in any of the 
media player apps. KIO (or something) downloads the entire file, during which 
time Dolphin freezes. The moment the network activity ends because the file is 
finished downloading, Dolphin becomes responsive again. It's 100% reproducible 
for me.
  
  I'm a bit confused. Which one of the three are you doing:
  
  1. Opening a KIO Url via Dolphin.
  2. Opening a KIOFuse mount local URL via Dolphin.
  3. Opening a KIOFuse URL via the media players file picker.

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

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

[Nathaniel Graham]

ngraham added a comment.


  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
  
  > **Issue #2:**
  > 
  > The only explanation I have for that is that totem for some reason starts 
reading the file from the end.
  >  If you start `kio-fuse -d` manually (kill the other kio-fuse process 
first) and then use totem again, what's the debug output
  
  There is no debug output, and the whole file is downloaded locally as with 
the other apps. It seems like Totem is not getting the FUSE mount path; it gets 
an `smb://` URL and it or KIO downloads the file normally. Again, this happens 
even if I open the file from the FUSE mount path itself.
  
  I can confirm that the FUSE path handoff is working properly. For example if 
I open `smb://gaston@living-room-pc/Users/Gaston/Desktop/Shep Face Code.txt` in 
Gedit, Gedit displays the file's path as follows: F7791491: 
Screenshot_20191130_133857.png 
  
  > **Issue #3:**
  > 
  > Everything is in a separate process and async already. When/how does it 
happen?
  
  When I open a large video file from the hidden mount path in any of the media 
player apps. KIO (or something) downloads the entire file, during which time 
Dolphin freezes. The moment the network activity ends because the file is 
finished downloading, Dolphin becomes responsive again. It's 100% reproducible 
for me.

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

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

[Fabian Vogt]

fvogt added a comment.


  **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
  
  **Issue #2:**
  
  The only explanation I have for that is that totem for some reason starts 
reading the file from the end for some reason.
  If you start `kio-fuse -d` manually (kill the other kio-fuse process first) 
and then use totem again, what's the debug output?
  
  If totem behaves like this, only `KIO::open` support might help a bit, with 
the cost of increased latency.
  That needs support for something like `KIO::truncate` though, so might take a 
while.
  
  **Issue #3:**
  
  Everything is in a separate process and async already. When/how does it 
happen?

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

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

[Nathaniel Graham]

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


  I tested this today with a variety of files on a Windows Samba share with 
fairly slow 6 Mb/sec bandwidth and it's almost perfect for all files and use 
cases. However I ran into some issues with handling large video files on a 
Samba share. This is an important use case and supports the workflow of viewing 
or working on video files stored on a NAS or media PC. I tried opening a 
large-ish 800 Mb video file using VLC, Totem (a.k.a. GNOME Videos), and 
SMPlayer. Here are the results:
  
  - VLC receives a `smb://` URL and failes to open the file, displaying the 
following error message:F7790278: Screenshot_20191129_141256.png 

  - Totem launches immediately but displays nothing. Network traffic indicates 
that it is locally downloading the entire file to a cache somewhere. Once the 
network traffic ends, it begins playing the file.
  - SMPlayer receives a `smb://` URL and fails to open the file, displaying the 
following error message:F7790279: Screenshot_20191129_141354.png 

  
  Issue #1
  
  
  It looks like both VLC and SMPlayer are getting the `smb://` URL and failing 
to open it because they don't already have credentials for the share, which is 
stupid and probably their own faults for not at least prompting the user for it 
(VLC used to do this on other distros I've used, so that may be a regression in 
the app, or a packaging issue on openSUSE). But neither app uses KIO, so they 
should be getting the path to the FUSE mount instead.
  
  Issue #2
  
  
  Even when opening the file directly from the hidden KIO-FUSE mount, all 
tested apps download the file locally instead of just reading it and starting 
playback. When I use Nautilus to mount the same samba share and open the file 
in Totem, the app opens and playback begins almost instantly. So it looks like 
the entire file does not have to be downloaded first with the GNOME FUSE 
implementation, but something in our implementation is causing this to happen 
compared to theirs.
  
  This can be confirmed by navigating to the hidden FUSE mount paths generated 
by KIO-FUSE and GVFS.
  
  `totem 
/run/user/1000/gvfs/smb-share\:server\=living-room-pc\,share\=users\,user\=gaston/Gaston/Desktop/Starship\
 Troopers.avi` - Totem opens instantly and playback begins instantly
  
  `totem 
'/run/user/1000/kio-fuse-vCERHW/smb/gaston@living-room-pc/Users/Gaston/Desktop/Starship
 Troopers.avi'` - Totem opens instantly but downloads the entire file locally 
before starting playback
  
  The same behavior happens when using VLC or SMPlayer instead of Totem. So it 
seems like there is some issue with our KIO-FUSE mount that causes the app to 
not understand that it can skip the lengthy download. Or maybe KIO itself is 
still doing this, even though it's no longer necessary. Or maybe Totem, VLC, 
and SMPlayer have special logic to invoke the more desirable "play instantly" 
behavior for URLs opened with GVFS paths and will need to be patched to invoke 
the same behavior for our KIO-FUSE paths as well.
  
  Issue #3
  
  
  I feel like Dolphin should not hang when you open a large file from the FUSE 
mount path directly and a long download begins. This should happen in another 
process or thread so that Dolphin's main UI isn't blocked.
  
  ---
  
  Some of these issues may be out of scope for this patch but I thought I'd 
just put them here in this review, and we can figure out if they need to be 
addressed elsewhere.

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

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

[Nathaniel Graham]

ngraham edited the summary of this revision.

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

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

[Alexander Saoutkin]

feverfew edited the test plan for this revision.

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

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

[Harald Sitter]

sitter added inline comments.

INLINE COMMENTS

> feverfew wrote in krun.cpp:598
> That's within the same loop though?
> 
> I explicitly want to change the QUrl in that list, so that I can simply 
> return the same list without having to do a copy of it all.
> 
> I can't get this to work. I need to change the definition of the struct to be:
> 
> `struct MountRequest {QDBusReply reply; QUrl };`
> 
> This doesn't compile because "is implicitly deleted because the default 
> definition would be ill-formed"...

Can't reproduce with gcc.

  diff --git a/src/widgets/krun.cpp b/src/widgets/krun.cpp
  index c05875f7..2ea79257 100644
  --- a/src/widgets/krun.cpp
  +++ b/src/widgets/krun.cpp
  @@ -576,10 +576,9 @@ static QList resolveURLs(const QList &_urls, 
const KService &_servic
   org::kde::KIOFuse::VFS 
kiofuse_iface(QStringLiteral("org.kde.KIOFuse"),

QStringLiteral("/org/kde/KIOFuse"),
QDBusConnection::sessionBus());
  -struct MountRequest { QDBusPendingReply reply; int 
urlIndex; };
  +struct MountRequest { QDBusPendingReply reply; QUrl  };
   QVector requests;
  -for (int i = 0; i < urls.length(); ++i) {
  -const QUrl url = urls[i];
  +for (QUrl  : urls) {
   if (KIO::DesktopExecParser::isProtocolInSupportedList(url, 
appSupportedProtocols))
   continue;
   if (KProtocolInfo::protocolClass(url.scheme()) == 
QLatin1String(":local")) {
  @@ -588,7 +587,7 @@ static QList resolveURLs(const QList &_urls, 
const KService &_servic
   if (job->exec()) { // ## nasty nested event loop!
   const QUrl localURL = job->mostLocalUrl();
   if (localURL != url) {
  -urls[i] = localURL;
  +url = localURL;
   //qDebug() << "Changed to" << localURL;
   // KIOFuse doesn't work too well with mtp/gdrive.
   // @see https://invent.kde.org/kde/kio-fuse/issues/1 
(GDrive)
  @@ -598,11 +597,11 @@ static QList resolveURLs(const QList 
&_urls, const KService &_servic
   && url.scheme() != QLatin1String("gdrive")) {
   // Can't convert...
   // Lets try a KIOFuse mount instead.
  -requests.push_back({ 
kiofuse_iface.mountUrl(url.toString()), i });
  +requests.push_back({ 
kiofuse_iface.mountUrl(url.toString()), url });
   }
   }
   } else {
  -requests.push_back({ kiofuse_iface.mountUrl(url.toString()), 
i });
  +requests.push_back({ kiofuse_iface.mountUrl(url.toString()), 
url });
   }
   }
   // Doesn't matter that we've blocked here to process the replies.
  @@ -610,7 +609,7 @@ static QList resolveURLs(const QList &_urls, 
const KService &_servic
   for (auto request : requests) {
   request.reply.waitForFinished();
   if (!request.reply.isError()) {
  -urls[request.urlIndex] = 
QUrl::fromLocalFile(request.reply.value());
  +request.url = QUrl::fromLocalFile(request.reply.value());
   }
   }
   }

That said, I forgot about the second loop and I am not too sure that carrying 
the reference across two loops is all that nice to read anyway. Perhaps best 
wait for dfaure on this matter.

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

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

[Alexander Saoutkin]

feverfew added inline comments.

INLINE COMMENTS

> sitter wrote in krun.cpp:598
> So did the old code though. See old line 584.
> 
> It's iterating by-reference, so you can simply assign to it via
> 
> https://doc.qt.io/qt-5/qurl.html#operator-eq-1
> 
> to change the content of the QUrl object (not the instance of the object in 
> the list, but rather the content of that object).

That's within the same loop though?

I explicitly want to change the QUrl in that list, so that I can simply return 
the same list without having to do a copy of it all.

I can't get this to work. I need to change the definition of the struct to be:

`struct MountRequest {QDBusReply reply; QUrl };`

This doesn't compile because "is implicitly deleted because the default 
definition would be ill-formed"...

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

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

[Harald Sitter]

sitter added inline comments.

INLINE COMMENTS

> feverfew wrote in krun.cpp:598
> Yes, but note later I need to then change the values in the `QList` outside 
> of the for loop, hence why I store the index in a struct associated with the 
> reply. How would I do that easily with a range based for loop?

So did the old code though. See old line 584.

It's iterating by-reference, so you can simply assign to it via

https://doc.qt.io/qt-5/qurl.html#operator-eq-1

to change the content of the QUrl object (not the instance of the object in the 
list, but rather the content of that object).

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

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

[Alexander Saoutkin]

feverfew added inline comments.

INLINE COMMENTS

> sitter wrote in krun.cpp:583
> Coding style says we use curly braces even for single-line if statements I 
> think.

Yep you're right, will fix in a mo.

> sitter wrote in krun.cpp:598
> What I meant is you should literally iterate using
> 
>   for (QUrl  : urls) {
> 
> which is what the code did before but you changed it for some reason.

Yes, but note later I need to then change the values in the `QList` outside of 
the for loop, hence why I store the index in a struct associated with the 
reply. How would I do that easily with a range based for loop?

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

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

[Harald Sitter]

sitter added a comment.


  some style complaints. Looks great other than that 

INLINE COMMENTS

> krun.cpp:583
> +const QUrl url = urls[i];
> +if (KIO::DesktopExecParser::isProtocolInSupportedList(url, 
> appSupportedProtocols))
> +continue;

Coding style says we use curly braces even for single-line if statements I 
think.

> sitter wrote in krun.cpp:598
> Is there a reason you are not using a range based for loop here? `for (const 
> QUrl  : urls)`

What I meant is you should literally iterate using

  for (QUrl  : urls) {

which is what the code did before but you changed it for some reason.

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

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

[Alexander Saoutkin]

feverfew updated this revision to Diff 69788.
feverfew added a comment.


  - Align arguments

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23384?vs=69774=69788

BRANCH
  arcpatch-D23384

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

AFFECTED FILES
  src/core/CMakeLists.txt
  src/core/desktopexecparser.cpp
  src/core/org.kde.KIOFuse.VFS.xml
  src/widgets/CMakeLists.txt
  src/widgets/krun.cpp
  src/widgets/org.kde.KIOFuse.VFS.xml

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: sitter, davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, GB_2, 
michaelh, bruns

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

[Alexander Saoutkin]

feverfew marked 8 inline comments as done.
feverfew added inline comments.

INLINE COMMENTS

> sitter wrote in desktopexecparser.cpp:317
> There's nothing wrong with this. But wouldn't using 
> `QHash, 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 reply, QUrl url };
>   QVector requests;
>   ...
>   requests.push_back({ mountUrl(url), url });

Gone for the vector option.

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

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

[Alexander Saoutkin]

feverfew updated this revision to Diff 69774.
feverfew added a comment.


  - 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

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23384?vs=69769=69774

BRANCH
  arcpatch-D23384

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

AFFECTED FILES
  src/core/CMakeLists.txt
  src/core/desktopexecparser.cpp
  src/core/org.kde.KIOFuse.VFS.xml
  src/widgets/CMakeLists.txt
  src/widgets/krun.cpp
  src/widgets/org.kde.KIOFuse.VFS.xml

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: sitter, davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, GB_2, 
michaelh, bruns

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

[Alexander Saoutkin]

feverfew updated this revision to Diff 69769.
feverfew added a comment.


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

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23384?vs=69201=69769

BRANCH
  KDEDModule (branched from master)

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

AFFECTED FILES
  src/core/CMakeLists.txt
  src/core/desktopexecparser.cpp
  src/core/kiofuseinterface.cpp
  src/core/kiofuseinterface.h
  src/widgets/krun.cpp

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: sitter, davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, GB_2, 
michaelh, bruns

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 parsedUrls;
> +QVector, QUrl>> replies;

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

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

There's nothing wrong with this. But wouldn't using 
`QHash, 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 reply, QUrl url };
  QVector requests;
  ...
  requests.push_back({ mountUrl(url), url });

> desktopexecparser.cpp:319
>  if (!mx1.hasUrls) {
> -Q_FOREACH (const QUrl , d->urls)
> +for(QUrl  : 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 parsedUrls;
> +for (QList::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  : 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

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

[Alexander Saoutkin]

feverfew added a comment.


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

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, GB_2, michaelh, 
bruns

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

[Alexander Saoutkin]

feverfew edited the summary of this revision.
feverfew edited the test plan for this revision.
feverfew edited reviewers, added: davidedmundson, dfaure, ngraham; removed: 
chinmoyr.

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham, chinmoyr
Cc: davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, GB_2, michaelh, 
bruns

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

[Alexander Saoutkin]

feverfew updated this revision to Diff 69201.
feverfew added a comment.


  Don't use KIOFuse with mtp/gdrive

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23384?vs=68931=69201

BRANCH
  KDEDModule (branched from master)

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

AFFECTED FILES
  src/core/CMakeLists.txt
  src/core/desktopexecparser.cpp
  src/core/kiofuseinterface.cpp
  src/core/kiofuseinterface.h
  src/widgets/krun.cpp

To: feverfew, fvogt, chinmoyr
Cc: davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, GB_2, michaelh, 
bruns

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

[Alexander Saoutkin]

feverfew updated this revision to Diff 68931.
feverfew added a comment.


  - Switch from KDED module to DBus service

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23384?vs=64440=68931

BRANCH
  KDEDModule (branched from master)

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

AFFECTED FILES
  src/core/CMakeLists.txt
  src/core/desktopexecparser.cpp
  src/core/kiofuseinterface.cpp
  src/core/kiofuseinterface.h
  src/widgets/krun.cpp

To: feverfew, fvogt, chinmoyr
Cc: davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, GB_2, michaelh, 
bruns

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

[David Edmundson]

davidedmundson added a comment.


  How does kio-fuse know when to unmount?

INLINE COMMENTS

> krun.cpp:529
> +// Lets try a KIOFuse mount instead.
> +QDBusInterface 
> kiofused(QStringLiteral("org.kde.kded5"), 
> +
> QStringLiteral("/modules/kiofuse"), 

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.

> krun.cpp:533
> +
> QDBusConnection::sessionBus());
> +if (!kiofused.isValid() || 
> kiofused.lastError().isValid()) {
> +// Module isn't loaded...

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

> krun.cpp:556
> +// Empty string means mounting failed...
> +if(reply.isValid() && !reply.value().isEmpty()) {
> +*it = QUrl::fromLocalFile(reply.value());

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

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, chinmoyr
Cc: davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, GB_2, michaelh, 
bruns

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

[Alexander Saoutkin]

feverfew created this revision.
feverfew added reviewers: fvogt, chinmoyr.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
feverfew requested review of this revision.

TEST PLAN
  Make sure kded module is installed and loaded.
  Use dolphin and observe that when opening applications that don't support KIO,
  the resulting files are opened in a KIOFuse mount instead of opened via the 
  help of kioexec.

REPOSITORY
  R241 KIO

BRANCH
  KDEDModule (branched from master)

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

AFFECTED FILES
  src/core/desktopexecparser.cpp
  src/widgets/krun.cpp

To: feverfew, fvogt, chinmoyr
Cc: kde-frameworks-devel, ngraham, LeGast00n, GB_2, michaelh, bruns