<table><tr><td style="">rjvbb 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/D4981" rel="noreferrer">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/D4981#94034" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;" rel="noreferrer">D4981#94034</a>, <a href="https://phabricator.kde.org/p/kfunk/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;" rel="noreferrer">@kfunk</a> wrote:</div>
<div style="margin: 0;
padding: 0;
border: 0;
color: rgb(107, 116, 140);"><p>I also don't like that additional combo box there. We already have way too many buttons in that bar. This feature is useful for around 0.01% of our users...</p></div>
</blockquote>
<p>That's a bit of a bold statement, remember that the main reason for implementing the feature now is uploading useful patches to Phabricator. Even if we assume that part of the current users of the ReviewBoard plugin will adapt to Phab's different approach and upload locally committed patches (once that' possible from within KDevelop) another part of that population will continue to work with diffs made from uncommitted local changes. That much is more or less clear from upstream exchanges on the topic (I myself will probably use both modes).</p>
<p>Either way, do you have a better location where the control can still be accessed and it's clear the thing even exists? If there are too many buttons one should maybe reconsider the 4 left-most buttons. I've never once even had the idea of using those, and I use the patchreview toolview a lot.</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/D4981#inline-20399" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kfunk</span> wrote in <span style="color: #4b4d51; font-weight: bold;">perforceplugin.h:91</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Ditch the <tt style="background: #ebebeb; font-size: 13px;">const</tt>. Not needed for POD types.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I see why it's not needed (doh), but what does POD stand for?</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/D4981#inline-20398" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kfunk</span> wrote in <span style="color: #4b4d51; font-weight: bold;">kdevsvnplugin.cpp:240</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Why does this store <tt style="background: #ebebeb; font-size: 13px;">contextLines</tt> into a member? That's totally cumbersome from a API POV.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I could indeed just as well have passed on the parameter to diff2() as an additional argument.</p>
<p style="padding: 0; margin: 8px;">I take it then that you wouldn't be in favour of making the contextLines parameter a member variable of the base class, with a setter function so that it doesn't have to be passed around regardless of whether an implementation actually support it?</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/D4981#inline-20402" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kfunk</span> wrote in <span style="color: #4b4d51; font-weight: bold;">ipatchsource.h:53</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">I really don't like this API, but can't think of anything better right now.</p>
<p style="padding: 0; margin: 8px;">If at all, this needs better API documentation, with named arguments in the signature + <tt style="background: #ebebeb; font-size: 13px;">@param</tt> in doxygen, etc.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">How about the alternative <tt style="background: #ebebeb; font-size: 13px;">virtual void update(int contextLines=-1) = 0;</tt>?</p>
<p style="padding: 0; margin: 8px;">Or else, make contextLines a member variable with a setter.</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/D4981#inline-20403" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kfunk</span> wrote in <span style="color: #4b4d51; font-weight: bold;">vcsdiffpatchsources.h:45</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Still, odd API. <tt style="background: #ebebeb; font-size: 13px;">lines</tt> is ambiguous, too, should be <tt style="background: #ebebeb; font-size: 13px;">contextLines</tt> if at all.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Here too one could make the parameter a member variable with setter method instead of an argument. It probably makes even more sense here than elsewhere.</p></div></div></div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D4981" rel="noreferrer">https://phabricator.kde.org/D4981</a></div></div><br /><div><strong>To: </strong>rjvbb, KDevelop, kfunk<br /><strong>Cc: </strong>kfunk, apol, kdevelop-devel, KDevelop, Pilzschaf, akshaydeo, surgenight, arrowdodger<br /></div>