<table><tr><td style="">sitter created this revision.<br />Herald added a project: Frameworks.<br />Herald added a subscriber: kde-frameworks-devel.<br />sitter requested review of this revision.
</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/D23579">View Revision</a></tr></table><br /><div><strong>REVISION SUMMARY</strong><div><p>This system is a bit inspired by an earlier patch I threw together in<br />
a half hour to prevent the actual logic code from calling state-changing<br />
functions by splitting the interface class (implements SlaveBase) from<br />
the actual FTP client implementation (used by the interface class).<br />
The advantage here is that the future internal code cannot call error<br />
or finished directly (and cause state violations in SlaveBase - e.g.<br />
double-error call) because there is no trivial way to access them.</p>

<p>Instead the internals return a new Result class if they need to return<br />
anything. This is largely inspired by how Error handling works in Go.<br />
The caller of an internal function needs to check the result or explicitly<br />
disregard it. The caller may then decide to ignore an internal error result<br />
(e.g. chmod after upload failed), create its own higher level result, or<br />
forward it unchanged. On the interface level we then convert the result<br />
to appropriate state changes of the slave.<br />
This is assisted by Q_REQUIRED_RESULT so capable compilers will warn if<br />
a Result is not explicitly ignored as that'd be an indication of missing<br />
error handling in new code, again future proofing the slave.</p>

<p>All things put together this turned out really well I think.</p>

<p>This also adds a primitive test because it broke a million times while<br />
porting. Starts a simple ruby ftpd (with some monkey patching applied).</p>

<p>TODO:</p>

<ul class="remarkup-list">
<li class="remarkup-list-item">there are still a bunch of pending todos littered over the code</li>
<li class="remarkup-list-item">currently the internal class relies on helper functions to forward interface calls to the interface class, they can all be dropped in favor of directly calling <tt style="background: #ebebeb; font-size: 13px;">q->foo</tt>.</li>
<li class="remarkup-list-item">needs more test scenarios I'd imagine</li>
<li class="remarkup-list-item">there is a larger question if we still need the StatusCode enum, it was exclusively used to differentiate when the error is remote vs local so the correct file can be referenced (e.g. "/foo failed to read" vs "ftp://h/foo failed to read") but since we can now create construct the correct error strings even in very deep internal call chains this may no longer be necessary</li>
<li class="remarkup-list-item">I am wondering if maybe returning QScopedPointer<Result> would be nicer, currently there are a lot of useless copies around</li>
<li class="remarkup-list-item">Result::fail needs being made more explicit, all three arguments should always need providing and ideally be useful (i.e. QString() the errorString is fairly ugh</li>
<li class="remarkup-list-item">unrelated: fix unrelated warnings, there's a lot of unsigned<->signed coercion happening</li>
</ul></div></div><br /><div><strong>REPOSITORY</strong><div><div>R241 KIO</div></div></div><br /><div><strong>BRANCH</strong><div><div>ftp</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D23579">https://phabricator.kde.org/D23579</a></div></div><br /><div><strong>AFFECTED FILES</strong><div><div>autotests/CMakeLists.txt<br />
autotests/ftp/testCopy1<br />
autotests/ftp/testCopy2<br />
autotests/ftp/testOverwriteCopy1<br />
autotests/ftp/testOverwriteCopy2<br />
autotests/ftpd<br />
autotests/ftptest.cpp<br />
cmake/FindGem.cmake<br />
cmake/FindGem.cmake.in<br />
cmake/FindRubyExe.cmake<br />
src/ioslaves/ftp/ftp.cpp<br />
src/ioslaves/ftp/ftp.h</div></div></div><br /><div><strong>To: </strong>sitter<br /><strong>Cc: </strong>dfaure, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns<br /></div>