Differential D1497

[kioslave/desktop] Ensure there are no multiple / when creating the file path
ClosedPublic
Actions

Authored by graesslin on Apr 26 2016, 2:44 PM.

Details

Reviewers
hein
dfaure
Group Reviewers
Plasma
Commits
R871:6b70416fac12: [kioslave/desktop] Ensure there are no multiple / when creating the file path
R120:6b70416fac12: [kioslave/desktop] Ensure there are no multiple / when creating the file path
Summary

Fixes failing test.

Diff Detail

Repository
R120 Plasma Workspace
Branch
fix-kioslave-desktop
Lint
No Linters Available
Unit
No Unit Test Coverage
graesslin updated this revision to Diff 3529.Apr 26 2016, 2:44 PM
graesslin retitled this revision from to [kioslave/desktop] Ensure there are no multiple / when creating the file path.
graesslin updated this object.
graesslin edited the test plan for this revision. (Show Details)
graesslin added a reviewer: Plasma.
Restricted Application added a project: Plasma. · View Herald TranscriptApr 26 2016, 2:44 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
hein accepted this revision.Apr 26 2016, 2:48 PM
hein added a reviewer: hein.
This revision is now accepted and ready to land.Apr 26 2016, 2:48 PM
graesslin added a reviewer: dfaure.Apr 26 2016, 2:54 PM

@dfaure I saw that you modified QUrl::setPath lately. Is this the correct approach here?

dfaure added inline comments.Apr 26 2016, 3:09 PM
kioslave/desktop/kio_desktop.cpp
161

Can the path really *not* start with a '/' in this code ?
From a QUrl point of view, it can happen with relative URLs ("desktop:foo.txt"), but we very rarely use that in KDE code, if at all (it's unusable without a reference directory).

So it seems to me that just removing the '/' would be enough in practice.

graesslin added inline comments.Apr 26 2016, 3:51 PM
kioslave/desktop/kio_desktop.cpp
161

So it seems to me that just removing the '/' would be enough in practice.

hmm, does that work also with older Qt (e.g. Qt 5.5)? Given that the test passes on Qt 5.5, but not on Qt 5.6, I fear that there is a difference.

dfaure edited edge metadata.Apr 26 2016, 7:31 PM

I'm pretty sure it should work. What changed in QUrl is that setPath used to normalize the path and doesn't do that anymore.
(qtbase commit 2e1de7f3c4ca). So in 5.5 the double slash got simplified to a single slash, while in 5.6 it stays and breaks the test.
Not putting a double slash in the first place will work with both Qt versions.

graesslin updated this revision to Diff 3534.Apr 27 2016, 6:12 AM
graesslin edited edge metadata.

Just drop the added / when constructing local path

Closed by commit R120:6b70416fac12: [kioslave/desktop] Ensure there are no multiple / when creating the file path (authored by graesslin). · Explain WhyApr 28 2016, 9:11 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mkioslave/desktop/kio_desktop.cpp (6 lines)

Diff 3529

View Options

kioslave/desktop/kio_desktop.cpp

Loading...