<table><tr><td style="">dfaure marked 5 inline comments as done.<br />dfaure 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/D29385">View Revision</a></tr></table><br /><div><div><blockquote style="border-left: 3px solid #8C98B8;
color: #6B748C;
font-style: italic;
margin: 4px 0 12px 0;
padding: 8px 12px;
background-color: #F8F9FC;">
<div style="font-style: normal;
padding-bottom: 4px;">In <a href="https://phabricator.kde.org/D29385#662336" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">D29385#662336</a>, <a href="https://phabricator.kde.org/p/svuorela/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;">@svuorela</a> wrote:</div>
<div style="margin: 0;
padding: 0;
border: 0;
color: rgb(107, 116, 140);"><p>I've been looking at it for a while, and after trying to decipher the long lambdas, which might have been better as named functions, I think it is, beside Kossebau's comments, fine.</p></div>
</blockquote>
<p>Interesting comment. I thought it was actually more readable to keep that code together, rather than having to jump around in the file to find the right slots.</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/D29385#inline-168049">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kossebau</span> wrote in <span style="color: #4b4d51; font-weight: bold;">openurljobtest.cpp:108</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">why no simple range-based for loop?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Good point, copy/pasted from another (old) test.</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/D29385#inline-168050">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kossebau</span> wrote in <span style="color: #4b4d51; font-weight: bold;">openurljob.h:73</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Makes me wonder if those runflags should not be rather part of KIO namespace, to decouple classes here.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Interesting point, let's discuss it quickly (because if we want to change that, I need to redo the kio RC1 tag for 5.70, which has ApplicationLauncherJob).</p>
<p style="padding: 0; margin: 8px;">But I'm not sure we should do that, anyway.</p>
<p style="padding: 0; margin: 8px;">On hindsight, I would rather change this one to be setTemporaryFiles(bool b);</p>
<p style="padding: 0; margin: 8px;">I don't like all the flag-conversion hell that we end up with otherwise. Stuff 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);">RunFlags flags = tempFile ? KRun::DeleteTemporaryFiles : RunFlags();
if (runExecutables) {
flags |= KRun::RunExecutables;
}</pre></div>
<p style="padding: 0; margin: 8px;">The input data, all the way up, is for sure not in the form of a set of job flags.<br />
So I find it more readable to have code like setSomething(conditionForSomething)<br />
than to have a bunch of flag manipulation code.</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/D29385#inline-168051">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kossebau</span> wrote in <span style="color: #4b4d51; font-weight: bold;">openurljob.h:96</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Could this and the following bool flags be turned into some flags instead? Just wondering, not looked further.</p>
<p style="padding: 0; margin: 8px;">Also wondering if there should not be some getter as well, when using a flagset that would be just a single method/symbol.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">With this one in particular, since it's on by default, we'd have to either have DefaultRunFlags that includes RunExecutables --- or we'd have to make the flag DoNotRunExecutables.<br />
I don't like either....</p>
<p style="padding: 0; margin: 8px;">Technically yes it could all be flags, but I'd much rather have independent setters.</p>
<p style="padding: 0; margin: 8px;">The TempFile flag is a good example why: if it's shared with ApplicationLauncherJob by being in the KIO namespace, does that mean we also put all of those in there? But then people can write things that don't make sense like ApplicationLauncherJob *job = ...; job->setFlags(DoNotRunExecutables). What? That flag doesn't apply to that job. If there's one thing that job does, it's to run executables :-) [but via desktop files, while that flag is actually about what OpenUrlJob should do when opening an executable directly].</p>
<p style="padding: 0; margin: 8px;">Sharing flags between jobs who need a different set, is a problem. It ends up with docu like "only these flags make sense here". Not great.</p>
<p style="padding: 0; margin: 8px;">I don't see a use case for getters, but we can certainly add them (either now or as needed).</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/D29385#inline-168052">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kossebau</span> wrote in <span style="color: #4b4d51; font-weight: bold;">openurljob.h:127</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">-> "mimeTypeFound"? would match other casing.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Well spotted, thanks.</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/D29385#inline-168053">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kossebau</span> wrote in <span style="color: #4b4d51; font-weight: bold;">openurljobhandlerinterface.h:33</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Please prepend a line</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);">* @class OpenUrlJobHandlerInterface openurljobhandlerinterface.h <KIO/OpenUrlJobHandlerInterface></pre></div>
<p style="padding: 0; margin: 8px;">at the top, to trick doxygen into documenting the full CamelCase include.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Ah! I was wondering what that was about...</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/D29385#inline-168057">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kossebau</span> wrote in <span style="color: #4b4d51; font-weight: bold;">openurljobhandlerinterface.h:84</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Prevent use of nested Private class here, declare a class OpenUrlJobHandlerInterfacePrivate outside, also use QScopedPointer for consistency and preparedness in case there ever is an actual object.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">LOL I was the one arguing against nested Private classes recently. Looks like I forgot that when copy/pasting here. Thanks!</p>
<p style="padding: 0; margin: 8px;">Hmm note that using QScopedPointer requires actually defining (in the .cpp) a dummy private class, even if there's no "new" anywhere (generated code wants to delete it).</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/D29385">https://phabricator.kde.org/D29385</a></div></div><br /><div><strong>To: </strong>dfaure, ahmadsamir, broulik, meven, kossebau, davidedmundson, nicolasfella, svuorela<br /><strong>Cc: </strong>kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns<br /></div>