[kio] Fix create path
ClosedPublic

Authored by anthonyfieroni on Dec 11 2017, 7:44 PM.

Details

Summary

Basically the problem is .path() + / + .. result in //path/to should we create some generic solution like createPathFromUrl when we simplified it. Or more generic like a class to handle it with method like addPath.

Diff Detail

Repository
R241 KIO
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
anthonyfieroni created this revision.Dec 11 2017, 7:44 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptDec 11 2017, 7:44 PM
anthonyfieroni requested review of this revision.Dec 11 2017, 7:44 PM

Something like

QString makePath(QString path)
{
    return QDir::cleanPath(path);
}

QString makePath(Qstring path1, QString path2, ...)
{
    return makePath(path1 + '/' + path2, ...);
}

Yes, QUrl::addPath would really be convenient.
Meanwhile, make an addPath helper that just does if !endsWith('/') append('/')

Calling cleanPath sounds much much slower to me...

Helper function in all over places.

dfaure requested changes to this revision.Dec 12 2017, 10:12 PM

Thanks!

src/helpers.h
23 ↗(On Diff #23828)

This file should be called *_p.h to make it clear that it's not installed (and therefore not public API).

helpers is too generic though, let's at least call this something like pathhelpers_p.h

30 ↗(On Diff #23828)

const QString & to avoid one copy

I think the template below could also take const QString &T as input.

I'm also wondering about the name. The name addPath made sense in KUrl, but here we're not "adding one path to an object". This is more about concatenating paths. How about concatPaths() ?

35 ↗(On Diff #23828)

Fancy variadic template, but is it actually called with more than 2 args anywhere? :-)

If not I'd suggest to keep it a simple function with 2 args.

36 ↗(On Diff #23828)

is the enable_if needed? After all if someone calls this with int or whatever else, it will simply fail to compile, right?

This revision now requires changes to proceed.Dec 12 2017, 10:12 PM

Rename function, remove variadic template, it still not needed

anthonyfieroni added inline comments.Dec 13 2017, 7:32 AM
src/pathhelpers_p.h
32

To be absolutely sure we can add pedantic check

if (!path1.endsWith('/') && !path2.startsWith('/'))

?

dfaure requested changes to this revision.Dec 13 2017, 8:08 AM

Thanks.

src/core/copyjob.cpp
113

This one was called correctly IMHO (ok, I probably named it) because it adds one path (singular) to a URL :-)

(equivalent naming would be "concatUrlAndPath" but that's not great).

May I suggest reverting to the original name addPathToUrl?

src/pathhelpers_p.h
32

It seems to me that path2 cannot start with '/', by contract. It's supposed to be relative.
So I'd say we could even be foolish^H^H^H^Hbrave and make that

Q_ASSERT(!path2.startsWith('/'));
This revision now requires changes to proceed.Dec 13 2017, 8:08 AM
anthonyfieroni marked an inline comment as done.
anthonyfieroni retitled this revision from [filewidgets] Fix create path to [kio] Fix create path.
dfaure accepted this revision.Dec 14 2017, 7:53 AM

Perfect!

(until the first bug report, that is) :-)

This revision is now accepted and ready to land.Dec 14 2017, 7:53 AM
This revision was automatically updated to reflect the committed changes.
dfaure added inline comments.Dec 20 2017, 10:36 AM
src/pathhelpers_p.h
32

The assert hits during KDirModelTest, says CI. Can you take a look?

QFATAL : KDirModelTest::testRenameDirectory() ASSERT: "!path2.startsWith(QLatin1Char('/'))" in file /home/jenkins/workspace/Frameworks kio kf5-qt5 SUSEQt5.10/src/core/../pathhelpers_p.h, line 31

https://build.kde.org/view/Frameworks/job/Frameworks%20kio%20kf5-qt5%20SUSEQt5.10/39/testReport/(root)/TestSuite/kiowidgets_kdirmodeltest/