<table><tr><td style="">dfaure requested changes to this revision.<br />dfaure added inline comments.<br />This revision now requires changes to proceed.
</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/D26407">View Revision</a></tr></table><br /><div><strong>INLINE COMMENTS</strong><div><div style="margin: 6px 0 12px 0;"><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D26407#inline-150041">View Inline</a><span style="color: #4b4d51; font-weight: bold;">meven</span> wrote in <span style="color: #4b4d51; font-weight: bold;">kfileitem.cpp:787</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">I meant this and this what KMountPoint does, better be precise here.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I see. Sorry, I hadn't realized that there is a Q_OS_ANDROID check in kmountpoint itself.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D26407#inline-150828">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kmountpoint.cpp:437</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">        <span class="n">QFileInfo</span> <span class="n">fileinfo</span><span class="p">(</span><span class="n">path</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">        <span style="color: #aa4000">if</span> <span class="p">(</span><span class="n">fileinfo</span><span class="p">.</span><span class="n">isSymLink</span><span class="p">())</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">            <span style="color: #aa4000">return</span> <span class="n">findByPath</span><span class="p">(</span><span class="n">fileinfo</span><span class="p">.</span><span class="n">symLinkTarget</span><span class="p">());</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Compared to canonicalPath, this only works if the very last component is a symlink.</p>

<p style="padding: 0; margin: 8px;">If I make /home/dfaure a symlink to /opt/dfaure (because I have more disk space there), then findByPath("/home/dfaure/Documents/foo.odt") used to return the mountpoint for /opt (which was correct) while now it will return the mount point for /home.<br />
isSymlink() will say false because foo.odt isn't a symlink.</p>

<p style="padding: 0; margin: 8px;">I don't see a perfect solution to this. The best we can do IMHO is make things work for local symlinked directories and for remote mounts, but we can't fully avoid slow calls in the case of symlinks-to-remote-mounts.</p>

<p style="padding: 0; margin: 8px;">So I would just call canonicalPath here, once we find out that the orig path isn't a slow mount. Yes it might point to a slow mount, but IMHO that's the user asking for (performance) trouble :)</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R241 KIO</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D26407">https://phabricator.kde.org/D26407</a></div></div><br /><div><strong>To: </strong>meven, Frameworks, ngraham, broulik, dfaure<br /><strong>Cc: </strong>anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns<br /></div>