dfaure requested changes to this revision. dfaure added a comment. This revision now requires changes to proceed.
Why is this not in the same commit as the related unittest, as is common practice? INLINE COMMENTS > kautosavefile.cpp:70 > const QString protocol(managedFile.scheme()); > - const QString path(managedFile.adjusted(QUrl::RemoveFilename | > QUrl::StripTrailingSlash).path()); > - QString name(managedFile.fileName()); > + const QString > path(QString::fromLatin1(QUrl::toPercentEncoding(managedFile.adjusted(QUrl::RemoveFilename > | QUrl::StripTrailingSlash).path()).constData())); > + QString > name(QString::fromLatin1(QUrl::toPercentEncoding(managedFile.fileName()).constData())); That's one long line, hard to read. Extract a `const QByteArray encodedPath = QUrl::toPercentEncoding(....)` ? [pre-existing: I would personally call this `QByteArray encodedDirectory` and `QString directory`, to my eyes path is a full path including filename. At least it's ambiguous, unlike directory] Also check if you can remove the .constData(). `QString::fromLatin1(const QByteArray &str)` was added in Qt 5.0, this code is probably older than that. > kautosavefile.cpp:71 > + const QString > path(QString::fromLatin1(QUrl::toPercentEncoding(managedFile.adjusted(QUrl::RemoveFilename > | QUrl::StripTrailingSlash).path()).constData())); > + QString > name(QString::fromLatin1(QUrl::toPercentEncoding(managedFile.fileName()).constData())); > and a `const QByteArray encodedFileName = ...`, for symmetry. Simpler than toPercentEncoding: use `QUrl::fileName(QUrl::FullyEncoded)`. However this wouldn't work for `encodedDirectory` above, because it wouldn't encode '/'. > kautosavefile.cpp:76 > // Subtract 1 for the _ char, 3 for the padding separator, 5 is for the > .lock > - int pathLengthLimit = FILENAME_MAX - NamePadding - name.size() - > protocol.size() - 9; > + int pathLengthLimit = NAME_MAX - NamePadding - name.size() - > protocol.size() - 9; > (while changing this line: prepend `const`) REPOSITORY R244 KCoreAddons REVISION DETAIL https://phabricator.kde.org/D24489 To: mardelle, #frameworks, dfaure, mpyne Cc: ahmadsamir, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
