D9290: [filewidgets] Fix create path
dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.
Thanks.
INLINE COMMENTS
> copyjob.cpp:113
>
> -static QUrl addPathToUrl(const QUrl , const QString )
> +static QUrl concatPathsToUrl(const QUrl , const QString )
> {
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?
> anthonyfieroni wrote in pathhelpers_p.h:31
> To be absolutely sure we can add pedantic check
>
> if (!path1.endsWith('/') && !path2.startsWith('/'))
>
> ?
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('/'));
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D9290
To: anthonyfieroni, #frameworks, dfaure, hein, aacid
D9290: [filewidgets] Fix create path
anthonyfieroni added inline comments.
INLINE COMMENTS
> pathhelpers_p.h:31
> +{
> +if (!path1.endsWith(QLatin1Char('/'))) {
> +return path1 + QLatin1Char('/') + path2;
To be absolutely sure we can add pedantic check
if (!path1.endsWith('/') && !path2.startsWith('/'))
?
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D9290
To: anthonyfieroni, #frameworks, dfaure, hein, aacid
D9290: [filewidgets] Fix create path
anthonyfieroni updated this revision to Diff 23843. anthonyfieroni added a comment. Rename function, remove variadic template, it still not needed REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D9290?vs=23828=23843 REVISION DETAIL https://phabricator.kde.org/D9290 AFFECTED FILES src/core/chmodjob.cpp src/core/copyjob.cpp src/core/deletejob.cpp src/core/forwardingslavebase.cpp src/core/kcoredirlister.cpp src/core/kfileitem.cpp src/core/listjob.cpp src/core/mkpathjob.cpp src/filewidgets/kdiroperator.cpp src/filewidgets/kfilewidget.cpp src/filewidgets/knewfilemenu.cpp src/filewidgets/kurlnavigatorbutton.cpp src/ioslaves/trash/kio_trash.cpp src/ioslaves/trash/tests/testtrash.cpp src/pathhelpers_p.h src/urifilters/ikws/searchproviderregistry.cpp src/urifilters/shorturi/kshorturifilter.cpp src/widgets/clipboardupdater.cpp src/widgets/kpropertiesdialog.cpp src/widgets/kurlcompletion.cpp src/widgets/kurlrequester.cpp src/widgets/paste.cpp src/widgets/renamedialog.cpp To: anthonyfieroni, #frameworks, dfaure, hein, aacid
D9290: [filewidgets] Fix create path
dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.
Thanks!
INLINE COMMENTS
> helpers.h:23
> +
> +#ifndef KIO_HELPERS_H
> +#define KIO_HELPERS_H
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
> helpers.h:30
> +inline
> +QString addPath(QString path)
> +{
const QString & to avoid one copy
I think the template below could also take const QString 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() ?
> helpers.h:35
> +
> +template + class = typename std::enable_if QString>::value>::type>
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.
> helpers.h:36
> +template + class = typename std::enable_if QString>::value>::type>
> +T addPath(T path, T1... pathN)
is the enable_if needed? After all if someone calls this with int or whatever
else, it will simply fail to compile, right?
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D9290
To: anthonyfieroni, #frameworks, dfaure, hein, aacid
D9290: [filewidgets] Fix create path
anthonyfieroni updated this revision to Diff 23828. anthonyfieroni added a comment. Helper function in all over places. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D9290?vs=23768=23828 REVISION DETAIL https://phabricator.kde.org/D9290 AFFECTED FILES src/core/chmodjob.cpp src/core/copyjob.cpp src/core/deletejob.cpp src/core/forwardingslavebase.cpp src/core/kcoredirlister.cpp src/core/kfileitem.cpp src/core/listjob.cpp src/core/mkpathjob.cpp src/filewidgets/kdiroperator.cpp src/filewidgets/kfilewidget.cpp src/filewidgets/knewfilemenu.cpp src/filewidgets/kurlnavigatorbutton.cpp src/helpers.h src/ioslaves/trash/kio_trash.cpp src/ioslaves/trash/tests/testtrash.cpp src/urifilters/ikws/searchproviderregistry.cpp src/urifilters/shorturi/kshorturifilter.cpp src/widgets/clipboardupdater.cpp src/widgets/kpropertiesdialog.cpp src/widgets/kurlcompletion.cpp src/widgets/kurlrequester.cpp src/widgets/paste.cpp src/widgets/renamedialog.cpp To: anthonyfieroni, #frameworks, dfaure, hein, aacid
D9290: [filewidgets] Fix create path
dfaure added a comment.
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...
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D9290
To: anthonyfieroni, #frameworks, dfaure, hein, aacid
D9290: [filewidgets] Fix create path
anthonyfieroni added a comment.
Something like
QString makePath(QString path)
{
return QDir::cleanPath(path);
}
QString makePath(Qstring path1, QString path2, ...)
{
return makePath(path1 + '/' + path2, ...);
}
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D9290
To: anthonyfieroni, #frameworks, dfaure, hein, aacid
D9290: [filewidgets] Fix create path
anthonyfieroni created this revision. anthonyfieroni added reviewers: Frameworks, dfaure, hein, aacid. Restricted Application added a project: Frameworks. REVISION 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. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D9290 AFFECTED FILES src/filewidgets/knewfilemenu.cpp To: anthonyfieroni, #frameworks, dfaure, hein, aacid
