sitter added inline comments.
INLINE COMMENTS
> desktopexecparser.cpp:314
> + org::kde::KIOFuse kiofuse_iface(QStringLiteral("org.kde.KIOFuse"),
> + QStringLiteral("/org/kde/KIOFuse"),
> + QDBusConnection::sessionBus());
align arguments with first argument. same in krun.cpp.
> desktopexecparser.cpp:316
> + QDBusConnection::sessionBus());
> + QList<QUrl> parsedUrls;
> + QVector<QPair<QDBusPendingReply<QString>, QUrl>> replies;
Do we need this? Seems to me you could just append to d->url directly.
> desktopexecparser.cpp:317
> + QList<QUrl> parsedUrls;
> + QVector<QPair<QDBusPendingReply<QString>, QUrl>> replies;
> if (!mx1.hasUrls) {
There's nothing wrong with this. But wouldn't using
`QHash<QDBusPendingReply<QString>, QUrl>` make for easier to read code all in
all since you don't have to mess with pairs?
Alternatively with a vector I'd still make a struct for the data
struct MountRequest { QDBusPendingReply<QString> reply, QUrl url };
QVector<MountRequest> requests;
...
requests.push_back({ mountUrl(url), url });
> desktopexecparser.cpp:319
> if (!mx1.hasUrls) {
> - Q_FOREACH (const QUrl &url, d->urls)
> + for(QUrl &url : d->urls) {
> if (!url.isLocalFile() && !hasSchemeHandler(url)) {
there should be a space after for.
what happened to the constness (also in the loop below)
> kiofuseinterface.h:28
> + */
> +class KIOCORE_EXPORT KIOFuseInterface: public QDBusAbstractInterface
> +{
I don't think this should be a public/exported class. It's purely for internal
use within KIO. It has no reason to become part of the ABI IMHO. To that end it
also shouldn't be part of ecm_generate_headers.
I suppose this generated file could also be replaced with the actual xml and
generated at build time instead?
Manually editing generated files is a bit meh in general.
To get the interface into both the core and widgets target I suppose you'd
simply add it to both source lists.
Does anyone else have an opinion on this?
> krun.cpp:598
> + QList<QUrl> parsedUrls;
> + for (QList<QUrl>::Iterator it = urls.begin(); it != urls.end();
> ++it) {
> + QUrl url = *it;
Is there a reason you are not using a range based for loop here? `for (const
QUrl &url : urls)`
> krun.cpp:605
> + if (KIO::DesktopExecParser::isProtocolInSupportedList(url,
> appSupportedProtocols)
> + && url.password().isEmpty())
> + continue;
That seems like a hack for a bug in VLC and also super opinionated and also
somewhat unrelated to fuse? If an application says it supports %u/%U and a
given protocol, we should expect it to be able to parse a valid rfc2396 URI I
would think.
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D23384
To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: sitter, davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, GB_2,
michaelh, bruns
