Fixes url navigation with relative links on KUrlNavigator
AbandonedPublic

Authored by emateli on Nov 20 2017, 6:04 PM.

Details

Reviewers
dfaure
Group Reviewers
Frameworks
Summary

This patch aims to address an issue with KUrlNavigator using relative paths. Upon attempting to navigate using relative paths, due to the way QUrl::fromUserInput works, a relative symbol such as 'Documents' will be converted to http://Documents or .config will be converted to an empty QUrl. This patch uses the AssumeLocalFile flag along with the home directory as the relative link (which is where the suggestions are taken from), alongside that it adds two test cases to account for this behaviour. The suggestions would perhaps be more intuitive if they were from the current directory but I assume that's an issue to be discussed in another patch.

All tests currently pass, but someone with more knowledge of this should check if this patch potentially breaks something with KUriFilter::filterUri.

Test Plan

Open an application where KUrlNavigator is used. Eg.: Dolphin, Gwenview

  • Select the KUrlNavigator and clear the text
  • Type something with dot or a character that would otherwise show suggestions
  • Highlight one of the suggestions and activate it via enter

Expected: The selected directory should be browsed
Actual: Dolphin will show invalid protocol error for dotfiles or otherwise launch browser. Gwenview fails silently.

Diff Detail

Repository
R241 KIO
Branch
relative-files-v2
Lint
No Linters Available
Unit
No Unit Test Coverage
emateli created this revision.Nov 20 2017, 6:04 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptNov 20 2017, 6:04 PM
Restricted Application added a subscriber: Frameworks. · View Herald Transcript
dfaure requested changes to this revision.Nov 21 2017, 9:58 AM

I like the use of the 3-args QUrl::fromUserInput, I added it to Qt for such purposes.
I don't like the hardcoded QDir::homePath(), this has to be done better...
In KUrlNavigator it could be a setter. In KUriFilter, I would rather not do it there, but handle that in the caller, if possible?

autotests/kurlnavigatortest.cpp
218

Does this line have any effect? This is only for the "go home" functionality, says the API docs, not for resolving relative paths, right?

This revision now requires changes to proceed.Nov 21 2017, 9:58 AM
emateli updated this revision to Diff 22702.EditedNov 21 2017, 6:41 PM
  • remove QDir::homePath + minor code style

@dfaure : from what I can tell, KUrlCompletion and the QUrl constructor with an empty directory defaults to the working directory, which apparently is the home directory when installed. While running the tests it is the same as the test directory where the binary is located.

E: I personally prefer explicitly passing the home dir. Looks more readable and is immediately understood as to what the function call does.

dfaure added a comment.EditedNov 21 2017, 9:58 PM

Relative completions in KUrlCompletion defaults to $HOME but that can be configured with KUrlCompletion::setDir.
Maybe KUrlNavigator should get a setter too, so that apps can set a base directory that makes sense to them?
They have more context for setting a correct base dir (e.g. an image app would set the Pictures dir -- or the directory from which the current file was opened, dolphin can set the currently viewed directory, etc.)

Note: I am very much against using the working directory (as in QDir::currentPath()), because the concept of "working directory" makes little sense in most graphical applications (you don't see it, you can't change it...) and is just an artefact of how the application was launched. IMHO that's even worse than the home dir as base ;-)

@dfaure I've managed to take another look at this, added a directory property which controls the base dir. Works as it should, however there is one case where I'm not sure which way we want it and is probably just a matter of preference.

User types ~/some-dir which does not exist. The kshorturifilter plugin will expand that to /home/.../some-dir, check that it doesn't exist, and then decide to not return the expanded value, but rather the original value.
The result of this is that the KUrlNavigator will attempt to browse /home/.../~/some-dir instead of /home/.../some-dir which perhaps might be slightly confusing, this works as it should if the directory does exist though.
On the upside directories named ~ are browsable. Thoughts? Do we try to "fix" this or is the current behaviour okay.

Ping. Thoughts on the last comment?

Sounds to me like ~/foo should always mean $HOME/foo even if that doesn't exist.

autotests/kurlnavigatortest.cpp
207

I think this should have a clear expected value, not the same call as the implementation, which is then not testing the actual behavior; it's like checking that a==a, but not the value of a.

I mean, to me it's unclear what fromUserInput(3 args) does when the 2nd arg is QLatin1String("") (yes, even though I actually wrote that method!).
Why QLatin1String("")? Does it behave any different when called with QString() instead? In any case, these are questions for the implementation. Here it should be a clear hardcoded expected value (well using QDir::homePath and QUrl::fromLocalFile if necessary, of course).

src/widgets/kurifilter.cpp
275

I would very very much like that KUriFilterData is left untouched if possible. Is there no way do call this in the caller instead? (which would then call the QUrl method)

The whole point of KUriFilterData was to not have any string-url conversion logic itself, but leave that to the actual uri filters.

emateli updated this revision to Diff 23532.Dec 5 2017, 7:59 PM
  • ~/ at the start always resolves to $HOME
dfaure added inline comments.Dec 5 2017, 9:11 PM
src/filewidgets/kurlnavigator.cpp
1009

This is written like kshorturifilter is buggy...
I looked into it to see if there was a good reason why kshorturifilter returns Error in case of a non-existing local path, but in fact.... it doesn't really have to.
If the user types a non-existing path in a URL bar then opening that path will error anyway, there's no need to catch that at the kurifilter level.

I'm working on a fix for kshorturifilter.

emateli abandoned this revision.Dec 18 2017, 4:52 PM

Resolved in D9217