D8920: Fixes url navigation with relative links on KUrlNavigator
emateli abandoned this revision. emateli added a comment. Resolved in https://phabricator.kde.org/D9217 REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8920 To: emateli, #frameworks, dfaure Cc: #frameworks
D8920: Fixes url navigation with relative links on KUrlNavigator
dfaure added inline comments. INLINE COMMENTS > kurlnavigator.cpp:1009 > +// Always expand starting ~/ to $HOME. We want all inputs starting with > it to point home directory. > +// The reason this expansion is performed here, is that kshorturifilter > will not return the desired result > +// if said directory does not exist. 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. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8920 To: emateli, #frameworks, dfaure Cc: #frameworks
D8920: Fixes url navigation with relative links on KUrlNavigator
emateli updated this revision to Diff 23532. emateli added a comment. - ~/ at the start always resolves to $HOME REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8920?vs=22702=23532 BRANCH relative-files-v2 REVISION DETAIL https://phabricator.kde.org/D8920 AFFECTED FILES autotests/kurlnavigatortest.cpp src/filewidgets/kurlnavigator.cpp src/filewidgets/kurlnavigator.h src/widgets/kurifilter.cpp src/widgets/kurifilter.h To: emateli, #frameworks, dfaure Cc: #frameworks
D8920: Fixes url navigation with relative links on KUrlNavigator
dfaure added a comment.
Sounds to me like ~/foo should always mean $HOME/foo even if that doesn't
exist.
INLINE COMMENTS
> kurlnavigatortest.cpp:207
> +
> +QTest::newRow("relativeFile") << QStringLiteral(".some_dotfile") <<
> QUrl::fromUserInput(".some_dotfile", QLatin1String(""),
> QUrl::AssumeLocalFile);
> +QTest::newRow("relativeDir") << QStringLiteral("some_directory") <<
> QUrl::fromUserInput("some_directory", QLatin1String(""),
> QUrl::AssumeLocalFile);
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).
> kurifilter.cpp:270
>
> KUriFilterData::KUriFilterData(const QString )
> +: d(new KUriFilterDataPrivate(QUrl::fromUserInput(url,
> QLatin1String(""), QUrl::AssumeLocalFile), url))
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.
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D8920
To: emateli, #frameworks, dfaure
Cc: #frameworks
D8920: Fixes url navigation with relative links on KUrlNavigator
emateli added a comment. Ping. Thoughts on the last comment? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8920 To: emateli, #frameworks, dfaure Cc: #frameworks
D8920: Fixes url navigation with relative links on KUrlNavigator
emateli added a comment. @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. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8920 To: emateli, #frameworks, dfaure Cc: #frameworks
D8920: Fixes url navigation with relative links on KUrlNavigator
dfaure added a comment. 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, 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 ;-) REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8920 To: emateli, #frameworks, dfaure Cc: #frameworks
D8920: Fixes url navigation with relative links on KUrlNavigator
emateli updated this revision to Diff 22702. emateli added a comment. - remove QDir::homePath + minor code style REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8920?vs=22671=22702 BRANCH relative-files REVISION DETAIL https://phabricator.kde.org/D8920 AFFECTED FILES autotests/kurlnavigatortest.cpp src/filewidgets/kurlnavigator.cpp src/widgets/kurifilter.cpp To: emateli, #frameworks, dfaure Cc: #frameworks
D8920: Fixes url navigation with relative links on KUrlNavigator
dfaure requested changes to this revision. dfaure added a comment. This revision now requires changes to proceed. 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? INLINE COMMENTS > kurlnavigatortest.cpp:216 > > +m_navigator->setHomeUrl(QDir::homePath()); > m_navigator->setLocationUrl(QUrl()); Does this line have any effect? This is only for the "go home" functionality, says the API docs, not for resolving relative paths, right? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8920 To: emateli, #frameworks, dfaure Cc: #frameworks
D8920: Fixes url navigation with relative links on KUrlNavigator
elvisangelaccio added a reviewer: dfaure. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8920 To: emateli, #frameworks, dfaure Cc: #frameworks
D8920: Fixes url navigation with relative links on KUrlNavigator
emateli added a reviewer: Frameworks. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D8920 To: emateli, #frameworks Cc: #frameworks
D8920: Fixes url navigation with relative links on KUrlNavigator
emateli created this revision. Restricted Application added a project: Frameworks. Restricted Application added a subscriber: Frameworks. REVISION 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. REPOSITORY R241 KIO BRANCH relative-files REVISION DETAIL https://phabricator.kde.org/D8920 AFFECTED FILES autotests/kurlnavigatortest.cpp src/filewidgets/kurlnavigator.cpp src/widgets/kurifilter.cpp To: emateli Cc: #frameworks
