<table><tr><td style="">hallas added inline comments.
</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/D27152">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/D27152#inline-157698">View Inline</a><span style="color: #4b4d51; font-weight: bold;">bruns</span> wrote in <span style="color: #4b4d51; font-weight: bold;">fstabhandling.cpp:374</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Why is the id() a property of the entry now?</p>

<ul class="remarkup-list">
<li class="remarkup-list-item">it does not correspond to anything from the fstab/mtab</li>
<li class="remarkup-list-item">it is only used here to generate the key for the cache</li>
</ul>

<p style="padding: 0; margin: 8px;">Please move the id generation back here, eventually changing it from <tt style="background: #ebebeb; font-size: 13px;">_k_deviceNameForMountpoint(const QString &source, const QString &fstype,const QString &mountpoint)</tt> to <tt style="background: #ebebeb; font-size: 13px;">_k_deviceIdForFsentry(const FilesystemEntry&)</tt></p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I guess the main reason is that the id is a function of a <tt style="background: #ebebeb; font-size: 13px;">FilesystemEntry</tt>, meaning that the id is computed from the contents of it. You are absolutely right that it is not picked up directly from the fstab/mtab file, but it is computed from the contents. I think it should either be a member function of the <tt style="background: #ebebeb; font-size: 13px;">FilesystemEntry</tt> class or a static member function of some other class (taking the <tt style="background: #ebebeb; font-size: 13px;">FilesystemEntry</tt> as an input paramter) - it at least has to be in a place where it is possible to write a unit test for it's functionality. especially since the id() is used as part of the public API. Therefore I don't think we should make this an internal function in fstabhandling. Another reason is that, we might need the id in other places then the fstabhandling, at least we need it for the rest of the fstab handling refactor I posted in the other review.</p>

<p style="padding: 0; margin: 8px;">So long story short :)<br />
I think we should either keep it here or make it a public (static) method somewhere else - <a href="https://phabricator.kde.org/p/bruns/" style="
              border-color: #f1f7ff;
              color: #19558d;
              background-color: #f1f7ff;
                border: 1px solid transparent;
                border-radius: 3px;
                font-weight: bold;
                padding: 0 4px;">@bruns</a> what do you think?</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R245 Solid</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D27152">https://phabricator.kde.org/D27152</a></div></div><br /><div><strong>To: </strong>hallas, Frameworks, bruns, meven<br /><strong>Cc: </strong>kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns<br /></div>