Differential D14682

Adding support for opening URLs with file scheme.
AbandonedPublic
Actions

Authored by garywang on Aug 8 2018, 4:01 AM.

Details

Reviewers
pino
Group Reviewers
Krita
Summary

In the current code base we can find Krita's desktop file are using %U / %u as argument in the .desktop's Exec key. While Krita doesn't support file:// scheme url as a launch argument,
for some file manager app like PCManFM (pcmanfm-qt, with LXDE or LXQT) and Deepin File Manager (dde-file-manager, with Deepin Desktop Envrioment), it will use a file:// scheme URL to replace the %u argument. Thus Krita can't open file correctly.

This patch is doing the work about adding support for file:// scheme URLs.

Test Plan

In the current code base we can find Krita's desktop file are using %U / %u as argument in the .desktop's Exec key. While Krita doesn't support file:// scheme url as a launch argument,
for some file manager app like PCManFM (pcmanfm-qt, with LXDE or LXQT) and Deepin File Manager (dde-file-manager, with Deepin Desktop Envrioment), it will use a file:// scheme URL to replace the %u argument. Thus Krita can't open file correctly.

According to https://standards.freedesktop.org/desktop-entry-spec/latest/ar01s07.html , Local files may either be passed as file: URLs or as file path, then probably Krita is
using the %u argument in the wrong way. A solution is using %f / %F instead of %u / %U.

There is a related bug in debian: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=905629 ,
and there is also a related commit to fix the bug: https://commits.kde.org/krita/c6c2368a3edb119a037065c19e9f504630e6c11f

It seems that commit only changed one of the .desktop files Krita is using, but actually we can simply fixing this issue by adding support to the file:// scheme. This patch is doing the work about adding support for file:// scheme URLs.

My first patch.. hope I didn't got the wrong place to submit a patch, should I create a bug first?

Diff Detail

Repository
R37 Krita
Lint
Lint Skipped
Unit
Unit Tests Skipped
garywang requested review of this revision.Aug 8 2018, 4:01 AM
garywang created this revision.
pino requested changes to this revision.Aug 8 2018, 5:11 AM
pino added a subscriber: pino.

I do not think this change is correct: %u/%U are not about "supporting the file:// scheme", but imply that the application can open any kind of URL, both local and remote ones. I also explained that in commit c6c2368a3edb119a037065c19e9f504630e6c11f, which will also do the job.

Supporting remote files is a much though job than this patch: other than properly handling arguments as URLs (and not as files) everywhere, then krita will need to use KIO to load remote files.

libs/ui/KisApplicationArguments.cpp
149–155

Since the URL comes from user input, then the right way to read it as QUrl is to use the QUrl::fromUserInput variant with the current directory. This way, relative file paths are handled correctly.

152

If you want the path of a local file, you must use toLocalFile(). The difference is relevant for Windows.

155

If filename is a remote URL, then this will turn into an empty string (since it is not a "file path").

This revision now requires changes to proceed.Aug 8 2018, 5:11 AM
garywang added a comment.EditedAug 8 2018, 5:21 AM
In D14682#305158, @pino wrote:

I also explained that in commit c6c2368a3edb119a037065c19e9f504630e6c11f, which will also do the job.

You are probably missing update other .desktop file, please take a look at /plugins/impex/kra/krita_kra.desktop and other files, we should replace all of them from %u to &f .

btw I've checked some file manager's behavior, when trying to launch Krita from right clicking from a remote path like smb://, it seems the file manager app will still using a local path like gvfs or kio which is mounted to local disk. Since Krita is not yet support other URL scheme, we only do file:// scheme support here.

Maybe we should do both, replace all %u with %f to make sure we only accepting local path, and also adding support to file:// scheme which is only used for local path?

I'll update the patch, Thank you for pointing out these issue :)

pino added a comment.Aug 8 2018, 5:29 AM
In D14682#305165, @garywang wrote:
In D14682#305158, @pino wrote:

I also explained that in commit c6c2368a3edb119a037065c19e9f504630e6c11f, which will also do the job.

You are probably missing update other .desktop file, please take a look at /plugins/impex/kra/krita_kra.desktop and other files, we should replace all of them from %u to &f .

Good catch, fixed now:

btw I've checked some file manager's behavior, when trying to launch Krita from right clicking from a remote path like smb://, it seems the file manager app will still using a local path like gvfs or kio which is mounted to local disk. Since Krita is not yet support other URL scheme, we only do file:// scheme support here.

Maybe we should do both, replace all %u with %f to make sure we only accepting local path, and also adding support to file:// scheme which is only used for local path?

Please try now with both c6c2368a3edb119a037065c19e9f504630e6c11f & 9d313acd4d15dd226d556d71d6d1a8956b24e6c5. Now file managers must not pass any URL to krita, but only paths to local files; if they do that, that is bug in the file manager.

garywang updated this revision to Diff 39283.Aug 8 2018, 5:47 AM

patchset1

Using QUrl::fromUserInput instead of using QUrl() directly.

Using QUrl::toLocalFile for the path.

garywang marked 2 inline comments as done.Aug 8 2018, 5:48 AM
garywang added inline comments.
libs/ui/KisApplicationArguments.cpp
155

I'm not sure about this, will a URL with file scheme be a remote URL?

pino requested changes to this revision.Aug 8 2018, 5:51 AM

Again, I do not think this approach is correct. After my changes to the desktop files, now file manager will not pass URLs anymore to krita.
As I said, supporting URLs requires much deeper changes.

In the meanwhile, please do test my patches.

libs/ui/KisApplicationArguments.cpp
155

If you pass ftp://host/path as argument, then your code will turn into an empty string (since absoluteFilePath cannot obviously resolve it).

This revision now requires changes to proceed.Aug 8 2018, 5:51 AM
garywang added a comment.Aug 8 2018, 6:00 AM
In D14682#305174, @pino wrote:

In the meanwhile, please do test my patches.

Your patches works, thank you for that <3. But this patch is not (exactly) for just solve that issue.

In D14682#305174, @pino wrote:

Again, I do not think this approach is correct. After my changes to the desktop files, now file manager will not pass URLs anymore to krita.
As I said, supporting URLs requires much deeper changes.

I'm just going to add support for URLs _only_ with a file:// scheme and just because it's for local path. URLs with other scheme which is not file:// scheme will still use the old way to load and finally failed to load. It will not change the fact Krita still requires a local path. Please see the title, I'm not planning to add support for all known URL schemes, but only the file:// scheme one.

pino added a comment.Aug 8 2018, 6:05 AM
In D14682#305178, @garywang wrote:
In D14682#305174, @pino wrote:

Again, I do not think this approach is correct. After my changes to the desktop files, now file manager will not pass URLs anymore to krita.
As I said, supporting URLs requires much deeper changes.

I'm just going to add support for URLs _only_ with a file:// scheme and just because it's for local path. URLs with other scheme which is not file:// scheme will still use the old way to load and finally failed to load. It will not change the fact Krita still requires a local path. Please see the title, I'm not planning to add support for all known URL schemes, but only the file:// scheme one.

What is the use cases of allowing file:// URLs, when any other URL is not allowed? Doing this seems to me an half-baked solution between the current situation (i.e. krita supporting only local files), and krita reading from any remote URL using KIO.

garywang added inline comments.Aug 8 2018, 6:05 AM
libs/ui/KisApplicationArguments.cpp
155

But the original code did the same thing, I did check the scheme is file scheme or not is just for that. This patch will only affect with who pass a file scheme URL as an argument, and keep other case use the original ways. When using a non-file scheme URL, if the original code will fall, this will fall, too.

pino added inline comments.Aug 8 2018, 6:07 AM
libs/ui/KisApplicationArguments.cpp
155

The original code does not handle URLs, but only local files, so using QDir is perfectly file.

You are working under the assumption that "file scheme URLs" and "other URLs" are two separates worlds -- in fact, they are both URLs, so they must be handled in the same way.

garywang abandoned this revision.Aug 8 2018, 6:12 AM
In D14682#305179, @pino wrote:

What is the use cases of allowing file:// URLs, when any other URL is not allowed? Doing this seems to me an half-baked solution between the current situation (i.e. krita supporting only local files), and krita reading from any remote URL using KIO.

Well, I think it can make the code more rubust for local file support.. Sorry about that.

Thank you anyway

Revision Contents
Changeset List

Diff 39283

View Options

libs/ui/KisApplicationArguments.cpp

Loading...