<table><tr><td style="">sitter 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/D23579">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/D23579#inline-136522">View Inline</a><span style="color: #4b4d51; font-weight: bold;">dfaure</span> wrote in <span style="color: #4b4d51; font-weight: bold;">ftptest.cpp:186</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">what's bad quot?</p>
<p style="padding: 0; margin: 8px;">Is this test about a bug, or the job failing is what we want?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">That was meant to read quota. I've changed it to QVERIFY(!job->exec()); as per your earlier comment.</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/D23579#inline-136531">View Inline</a><span style="color: #4b4d51; font-weight: bold;">dfaure</span> wrote in <span style="color: #4b4d51; font-weight: bold;">ftp.cpp:696</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">The reuse of the <tt style="background: #ebebeb; font-size: 13px;">result</tt> var irks me a bit.<br />
const vars rock :-)</p>
<p style="padding: 0; margin: 8px;">Would it be a bad idea to make Result implicitly convertible to bool, so you can keep doing this inside the if? <tt style="background: #ebebeb; font-size: 13px;"> if (!ftpSendCmd(QByteArrayLiteral("PWD")) || (m_iRespType != 2)) {</tt> would still work fine then.</p>
<p style="padding: 0; margin: 8px;">This would simplify all cases where we test for failures, but ignore the actual error message on failure (because we'll provide a more high-level one, like here). In other cases, we indeed need a result var.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I know what you mean, I originally started out with consts but moved away because it was getting fairly repetitive and in the end the constness offers little, the objects are fairly "ephemeral" much like errno.</p>
<p style="padding: 0; margin: 8px;">The result handling is indeed very "wordy", but I think there is value to be had in the expressiveness there:<br />
A Result is not really a binary affair. Yes it is failure or success, but if it is a failure the developer <strong>must</strong> make a conscious choice to not use the context information of. If Result was implicitly usable as a bool we'd not force this conscious choice, opening us up for mistakes further down the line when everyone has forgotten if ftpSendCmd needs/deserves result handling... or worse yet, looking at old code it won't be obvious if the author ignored the result context by accident or intentionally.</p>
<p style="padding: 0; margin: 8px;">So, I would stay away from treating a Result like a bool.</p>
<p style="padding: 0; margin: 8px;">To get to the heart of the issue though: I actually think ftpSendCmd probably shouldn't even return a Result but rather a bool. Probably 99% of all callers entirely ignore the Result context and treat it as a bool, which seems like a strong indication that the return type is unnecessarily complex. Should I go ahead with making it bool?</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/D23579#inline-136537">View Inline</a><span style="color: #4b4d51; font-weight: bold;">dfaure</span> wrote in <span style="color: #4b4d51; font-weight: bold;">ftp.cpp:1602</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">What happened to those two lines of comments? Can I suggest to keep them?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Oh! Collateral damage from removing the nesting I guess. Comments are back now.</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/D23579#inline-136539">View Inline</a><span style="color: #4b4d51; font-weight: bold;">dfaure</span> wrote in <span style="color: #4b4d51; font-weight: bold;">ftp.cpp:1880</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Wasn't that just a way to propagate the error from the low-level method to this method? Your new mechanism replaces that, no?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I could have sworn there was code that differentiated between server and client. I can't find it anymore... maybe it was all a dream, or maybe I saw it in the history.</p>
<p style="padding: 0; margin: 8px;">Oh well, down with the useless enum!</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/D23579#inline-136546">View Inline</a><span style="color: #4b4d51; font-weight: bold;">dfaure</span> wrote in <span style="color: #4b4d51; font-weight: bold;">ftp.cpp:2363</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">optimistic but useless initializations, all code paths set (or ignore) result.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I do need to declare it though, and by design a Result cannot have a "null" state, so I either need to initialize a failure or pass here. Do you have a better suggestion?</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/D23579#inline-136550">View Inline</a><span style="color: #4b4d51; font-weight: bold;">dfaure</span> wrote in <span style="color: #4b4d51; font-weight: bold;">ftp.cpp:2596</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Yes, clearly. But the question is where to check it.</p>
<p style="padding: 0; margin: 8px;">Hmm, how does this method even get called, without an event loop? From <tt style="background: #ebebeb; font-size: 13px;">m_control->waitForReadyRead</tt>, maybe?</p>
<p style="padding: 0; margin: 8px;">The old code was broken here, it would for sure emit error+error or error+finished, since the main code had no idea that this slot emitted error already.</p>
<p style="padding: 0; margin: 8px;">Hmm. Shouldn't we do this connect *before* the waitForConnected in Ftp::synchronousConnectToHost? If a proxy exists, it will surely act at connection time?<br />
And then it becomes easier, the place where to check the errno-like member is just after the call to Ftp::synchronousConnectToHost...</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Since you mention it... where even is the eventloop for any of this?<br />
SlaveBase has no event processing, neither does the FTP slave. I get the distinct feeling that the entire auth code never worked, or at least not since kf5. To that end I'd actually be in favor of removing it since no one complained (or I cannot find any bug report anyway).</p>
<p style="padding: 0; margin: 8px;">From some limited testing your assessment would be mostly correct though. When trying to waitForConnected there'd be a ProxyAuthenticationRequiredError. Which we can handle somewhat awkwardly by querying the info from the user and then setting a newly "enhanced" proxy QNetworkProxy::setApplicationProxy before creating a new socket I guess.</p>
<p style="padding: 0; margin: 8px;">I'm not too excited about fixing proxy auth support no one apparently needs.</p>
<p style="padding: 0; margin: 8px;">Originally introduced without review as well:<br />
<a href="https://git.reviewboard.kde.org/r/102923/" class="remarkup-link" target="_blank" rel="noreferrer">https://git.reviewboard.kde.org/r/102923/</a></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/D23579#inline-136551">View Inline</a><span style="color: #4b4d51; font-weight: bold;">dfaure</span> wrote in <span style="color: #4b4d51; font-weight: bold;">ftp.cpp:2706</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">This looks wrong. openConnection() should emit connected() or error(), but never finished().</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">You may wish to take a look at <a href="https://phabricator.kde.org/D23537" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">D23537</a> ;)</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/D23579#inline-136526">View Inline</a><span style="color: #4b4d51; font-weight: bold;">dfaure</span> wrote in <span style="color: #4b4d51; font-weight: bold;">ftp.h:70</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">I think you can make all members const?</p>
<p style="padding: 0; margin: 8px;">(This will prevent bypassing the "factory methods"...)</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">It's a good idea. Would that work though?</p>
<p style="padding: 0; margin: 8px;">Currently the results rely on the implicit move operator when collecting the returned result</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);">result = ftpGet()</pre></div>
<p style="padding: 0; margin: 8px;">if the members are const we couldn't move/copy like that anymore.</p>
<p style="padding: 0; margin: 8px;">If we consider the mutability a problem I think I'd just make the members private and give them getters. I don't mind much either way.</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/D23579#inline-136528">View Inline</a><span style="color: #4b4d51; font-weight: bold;">dfaure</span> wrote in <span style="color: #4b4d51; font-weight: bold;">ftp.h:156</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Does this method serve any purpose?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Its sole purpose is to make it harder to call</p>
<p style="padding: 0; margin: 8px;"><tt style="background: #ebebeb; font-size: 13px;">q->finished()</tt> in the internal class (and that way bypass the result system). Same for error() above.</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/D23579#inline-136556">View Inline</a><span style="color: #4b4d51; font-weight: bold;">anthonyfieroni</span> wrote in <span style="color: #4b4d51; font-weight: bold;">ftp.h:167</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Should be hidden?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Does that make a useful difference for a plugin?</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/D23579">https://phabricator.kde.org/D23579</a></div></div><br /><div><strong>To: </strong>sitter, dfaure<br /><strong>Cc: </strong>anthonyfieroni, dfaure, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns<br /></div>