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.
Details
- Reviewers
dfaure hein aacid - Group Reviewers
Frameworks - Commits
- R241:c72f8e4b0ca0: Fix creation of paths
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.
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...
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? |
src/pathhelpers_p.h | ||
---|---|---|
32 | To be absolutely sure we can add pedantic check if (!path1.endsWith('/') && !path2.startsWith('/')) ? |
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. Q_ASSERT(!path2.startsWith('/')); |
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 |