Differential D11492

Prevent duplicate slashes in Recent Folders
ClosedPublic
Actions

Authored by rkflx on Mar 19 2018, 4:54 PM.

Details

Reviewers
muhlenpfordt
Group Reviewers
Gwenview
Commits
R260:79a537ee840c: Prevent duplicate slashes in Recent Folders
Summary

Sometimes the Recent Folders tab on the Start Page would
show directory entries with multiple / at the end. Clicking on those
folders works fine, but as the same folder could also be added without
two slashes, in some cases there were multiple entries for the same
location.

The duplicate slash is caused by a porting mistake in d760ced7cb5b,
which added / unconditionally while KUrl::AddTrailingSlash only
added / if there was none at the end. Performing this check manually
fixes the bug.

Note that the fix for Bug #312060 (differentiate between home and root
folder for remote hosts) and what was added in 7eb5472f6539 (no password
dialog right after startup) are still in effect as well, even after
porting away from KUrl.

As a side effect, visiting / does not result anymore in broken
Recent Folder entries (non-functional and showing an error icon).
KIO warnings are gone, too:

kf5.kio.core: Invalid URL: QUrl("")

CCBUG: 383850

Test Plan

Open folder, Go → Up, switch to Start Page and notice how
now the added recent folder only has a single slash at the end. In
particular, there is only a single entry for this location after
navigating back and forth again.

Start sshd, sftp://localhost still shows home folder,
sftp://localhost/ shows root dir. Killing kiod and restarting
Gwenview does not show a password prompt on the Start Page.

Visit /, switch back to Start Page and observe that the newly
added entry shows and works correctly.

Diff Detail

Repository
R260 Gwenview
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
rkflx requested review of this revision.Mar 19 2018, 4:54 PM
rkflx created this revision.
muhlenpfordt accepted this revision.Mar 20 2018, 8:04 AM
muhlenpfordt added a subscriber: muhlenpfordt.

Works as promised for local urls given as commandline argument or by navigating inside Gwenview. :)
I found only one way to add multiple recent folder entries with multiple slashes: commandline urls like sftp://server/ and e.g. sftp://server/// or http://...////. This is not handled by UrlUtils::fixUserEnteredUrl() inside MainWindow::setInitialUrl(). But I'm not sure if we should touch remote urls?

This revision is now accepted and ready to land.Mar 20 2018, 8:04 AM
Closed by commit R260:79a537ee840c: Prevent duplicate slashes in Recent Folders (authored by rkflx). · Explain WhyMar 21 2018, 1:02 AM
This revision was automatically updated to reflect the committed changes.
rkflx added a comment.Mar 21 2018, 1:05 AM

Thanks for reviewing both patches ;) (Any thoughts about D11495?)

In D11492#229705, @muhlenpfordt wrote:

Works as promised for local urls given as commandline argument or by navigating inside Gwenview. :)
I found only one way to add multiple recent folder entries with multiple slashes: commandline urls like sftp://server/ and e.g. sftp://server/// or http://...////. This is not handled by UrlUtils::fixUserEnteredUrl() inside MainWindow::setInitialUrl(). But I'm not sure if we should touch remote urls?

While there are conventions for remote URLs (e.g. meaning of / & ? etc.), in the end it's just a string that is sent in the request and the remote end could do anything with it. Thus we should not interfere with this, I guess.

Nevertheless, I could not get Gwenview to add more slashes than there were initially (which the bug is about). Only if I visit /// (which the remote server maps to /), enter a subfolder and go back up to / I see multiple entries (but not for every subfolder, which was the original problem). So in a sense Gwenview adds the original URL to the history instead of the redirect which is used internally afterwards and for all subsequent folders. I guess this behaviour makes sense, e.g. in case the redirect is to a different host you'd still want to see the original URL in your history.

Anyway, it's a good thing we double-checked that part.

Revision Contents
Changeset List

PathPackages
Mapp/gvcore.cpp (7 lines)

Diff 30079

View Options

app/gvcore.cpp

Loading...