<table><tr><td style="">mardelle added a comment.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D24489">View Revision</a></tr></table><br /><div><div><p>The unittest is in another diff because it's a different author I guess. I can process the comments but looking closer I found 2 other important issues in this class:</p>

<ol class="remarkup-list">
<li class="remarkup-list-item">When using KAutoSaveFile::staleFiles(filename) to check if there is an existing stale file for a document, we use the function KAutoSaveFile::extractManagedFilePath</li>
</ol>

<p>This function compares the orginal document path one reconstructed from the lock file name. However this logic seems flawed since the lock file name uses a trimmed path in some cases it is impossible to rebuild the original document's path from it.<br />
Instead I would propose to replace extractManagedFilePath with a new function, comparing filename, then paths like:</p>

<div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">bool staleMatchesManaged(const QString& staleFileName, const QString managedFile)
{
    const QStringRef sep = staleFileName.rightRef(3);
    int sepPos = staleFileName.indexOf(sep);
    if (QFileInfo(managedFile).fileName() != staleFileName.leftRef(sepPos).toString()) {
        // File names don't match
        return false;
    }
    int pathPos = staleFileName.indexOf(QChar::fromLatin1('_'), sepPos);
    const QByteArray encodedPath = staleFileName.midRef(pathPos+1, staleFileName.length()-pathPos-1-KAutoSaveFilePrivate::NamePadding).toLatin1();
    const QString sourcePathStart = QUrl::fromPercentEncoding(encodedPath);
    return QFileInfo(managedFile).absolutePath().startsWith(sourcePathStart);
}</pre></div>



<ol class="remarkup-list" start="2">
<li class="remarkup-list-item">The QLockFile class used in KAutoSaveFile creates a temporary file to manage locking:</li>
</ol>

<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><p>QLockFile rmlock(d->fileName + QLatin1String(".rmlock"));</p></blockquote>

<p><a href="https://github.com/qt/qtbase/blob/dev/src/corelib/io/qlockfile.cpp#L259" class="remarkup-link" target="_blank" rel="noreferrer">https://github.com/qt/qtbase/blob/dev/src/corelib/io/qlockfile.cpp#L259</a></p>

<p>This means that in our situation where we have a filename that already has the maximum length, trying to lock will always fail, preventing usage of the autosave file.<br />
Easier solution is probably to make maximum filename in KAutoSave File::tempFileName() a bit shorter.</p>

<p>Thoughts? Should I prepare separate patches ? But changing the fileName encoding as suggested:</p>

<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><p>Simpler than toPercentEncoding: use QUrl::fileName(QUrl::FullyEncoded).</p></blockquote>

<p>Might break more the path recovery method extractManagedFilePath..</p></div></div><br /><div><strong>REPOSITORY</strong><div><div>R244 KCoreAddons</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D24489">https://phabricator.kde.org/D24489</a></div></div><br /><div><strong>To: </strong>mardelle, Frameworks, dfaure, mpyne<br /><strong>Cc: </strong>ahmadsamir, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns<br /></div>