<table><tr><td style="">hallas 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/D21760">View Revision</a></tr></table><br /><div><div><p><a href="https://phabricator.kde.org/p/dfaure/" style="
              border-color: #f1f7ff;
              color: #19558d;
              background-color: #f1f7ff;
                border: 1px solid transparent;
                border-radius: 3px;
                font-weight: bold;
                padding: 0 4px;">@dfaure</a>  - Overall, what do you think about the approach of subclassing KJob? Did it turn out like you had thought? And is this the solution we should go with, or was one of the other solutions better?</p></div></div><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/D21760#inline-133076">View Inline</a><span style="color: #4b4d51; font-weight: bold;">dfaure</span> wrote in <span style="color: #4b4d51; font-weight: bold;">klistopenfiles.cpp:29</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">I wonder if this actually needs to be a QObject, given that you use connect-to-pointer-to-member-function?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">The primary motivation was memory-management, but I could also just make the d-pointer in ListOpenFilesJob a QScopedPointer, would that be better?</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/D21760#inline-133077">View Inline</a><span style="color: #4b4d51; font-weight: bold;">dfaure</span> wrote in <span style="color: #4b4d51; font-weight: bold;">klistopenfiles.cpp:44</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">KIO::ERR_DOES_NOT_EXIST would fit well here, but that's in kio...</p>

<p style="padding: 0; margin: 8px;">I suggest adding enum { ERR_DOES_NOT_EXIST = KJob::UserDefinedError + 11 }<br />
in the header file for this job and using that here.</p>

<p style="padding: 0; margin: 8px;">And yes, you should do something 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);">setErrorText(i18n("Directory does not exist: %1", path));</pre></div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I have added the error enum entries, but I am a little hesitant to add to error texts since KCoreAddons doesn't today depend on KDE::I18n and this would introduce that dependency, do we really want that? When I look at the KIO classes then they usually set som non-translated string as the error text, would that be an option?</p></div></div></div></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/D21760">https://phabricator.kde.org/D21760</a></div></div><br /><div><strong>To: </strong>hallas, davidedmundson, broulik, Frameworks, dfaure, bruns, Plasma<br /><strong>Cc: </strong>meven, cfeck, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns<br /></div>