kioclient: Don't convert `:x:y` to `?line=x&column=y` for URLs starting with remote schemes.
ClosedPublic

Authored by arrowd on Jul 18 2019, 9:03 AM.

Diff Detail

Repository
R126 KDE CLI Utilities
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 14075
Build 14093: arc lint + arc unit
arrowd created this revision.Jul 18 2019, 9:03 AM
Restricted Application added a project: Plasma. · View Herald TranscriptJul 18 2019, 9:03 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
arrowd requested review of this revision.Jul 18 2019, 9:03 AM
cfeck added a subscriber: cfeck.Jul 18 2019, 12:28 PM

Could we only apply the :xx check on the filename part, not on the server part? Someone might expect that http://path.to/file.txt:99 also works for remote files.

Could we only apply the :xx check on the filename part, not on the server part? Someone might expect that http://path.to/file.txt:99 also works for remote files.

I don't think that rewriting http://path.to/file.txt:99 to http://path.to/file.txt?line=99 is a good idea. I'd better not touch non`file://` URLs at all.

cfeck added a comment.Jul 18 2019, 7:50 PM

Oh, if the latter syntax also works, then you are right.

Oh, if the latter syntax also works, then you are right.

Not sure what you mean by "also works". Current code checks if the URL ends with :x:y and turns it into ?line=x&column=y. This is used to open local text files at the given cursor position. However, for URL's like http://localhost:3000 it is invalid to perform such transformation. My patch checks URL scheme and applies the transformation only if it is file://.

dfaure requested changes to this revision.Jul 20 2019, 10:03 AM
dfaure added a subscriber: dfaure.

I suppose the kate developers like the fact that this currently works over FTP, SFTP, FISH, SMB, etc.
So maybe only HTTP[S]/WEBDAV should be blacklisted (because there queries have a different meaning, one that we can't know client-side).

This revision now requires changes to proceed.Jul 20 2019, 10:03 AM

I suppose the kate developers like the fact that this currently works over FTP, SFTP, FISH, SMB, etc.
So maybe only HTTP[S]/WEBDAV should be blacklisted (because there queries have a different meaning, one that we can't know client-side).

I'm confused. I just tried SFTP and SMB without the patch and it also doesn't work. As I said, it is invalid to rewrite URLs for remote schemas.

dfaure accepted this revision.Jul 20 2019, 11:21 AM

OK, I'm wrong (not the first time this happens) :-)

kwrite /home/dfaure/.zshrc:20 works
kwrite sftp://localhost/home/dfaure/.zshrc:20 doesn't work indeed.

The patch looks fine then.

This revision is now accepted and ready to land.Jul 20 2019, 11:21 AM
This revision was automatically updated to reflect the committed changes.
wbauer added a subscriber: wbauer.Jul 20 2019, 5:29 PM

What about the stable 5.16 branch?
It has only be committed to master so far AFAICS, so the fix would only end up in Plasma 5.17 which is still 3 months away...

I'm not familiar with KDE release engineering, so I'm asking to do merging someone else.