<div dir="ltr">On Wed, Sep 4, 2013 at 1:08 PM, Stephen Kelly <span dir="ltr"><<a href="mailto:steveire@gmail.com" target="_blank">steveire@gmail.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">


<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div>Martin Klapetek wrote:<br>
>> 1) It would be much better if you made several patches for things like<br>
>> this.<br>
>> One patch to remove the remove_definitions() calls, one patch to add<br>
>> Qt5::Gui to the link interface, one patch to add the needed<br>
>> find_dependency() calls, and then finally the patch moving the code<br>
>> without needing to change it. Reviewers should look for this stuff imo.<br>
>><br>
><br>
> Fair enough, though these changes were really small (no code needed to be<br>
> changed with removing definitions, one line for find_dependency() etc...).<br>
<br>
</div>Good patches are indivisible. That makes them small, which is actually a<br>
good thing, not a bad thing as you imply.<br></blockquote><div><br></div><div>I'm not implying it's a bad thing, merely that I don't think merging couple related one-liners into one bigger patch is that big problem. I agree however that the move could have been done separately and will do so next time.</div>


<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div>
><br>
>><br>
>> 2) Why did you add Qt5::Gui to the link interface? As far as I can see it<br>
>> does not need to be.<br>
>><br>
><br>
> Because KCrash needs qwindowdefs.h, which is in QtGui and afaiu the link<br>
> interface is what sets include paths now, so without Gui it wouldn't<br>
> build.<br>
<br>
</div>Actually it's the link implementation that provides the include paths.<br>
<br>
You put the Qt5::Gui in the LINK_PUBLIC section. It should be in the<br>
LINK_PRIVATE section.<br>
<br>
 <a href="http://community.kde.org/Frameworks/Epics/kdelibs_cleanups/link_private_details" target="_blank">http://community.kde.org/Frameworks/Epics/kdelibs_cleanups/link_private_details</a></blockquote><div><br></div><div>


Ok, will fix.</div><div><br></div><div>Cheers</div></div>-- <br><div><span style="color:rgb(102,102,102)">Martin Klapetek | KDE Developer</span></div>
</div></div>