Review Request 112495: Move kcrash to tier2

[Martin Klapetek]

On Wed, Sep 4, 2013 at 1:08 PM, Stephen Kelly <steveire at gmail.com> wrote:

> Martin Klapetek wrote:
> >> 1) It would be much better if you made several patches for things like
> >> this.
> >> One patch to remove the remove_definitions() calls, one patch to add
> >> Qt5::Gui to the link interface, one patch to add the needed
> >> find_dependency() calls, and then finally the patch moving the code
> >> without needing to change it. Reviewers should look for this stuff imo.
> >>
> >
> > Fair enough, though these changes were really small (no code needed to be
> > changed with removing definitions, one line for find_dependency()
> etc...).
>
> Good patches are indivisible. That makes them small, which is actually a
> good thing, not a bad thing as you imply.
>

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.


> >
> >>
> >> 2) Why did you add Qt5::Gui to the link interface? As far as I can see
> it
> >> does not need to be.
> >>
> >
> > Because KCrash needs qwindowdefs.h, which is in QtGui and afaiu the link
> > interface is what sets include paths now, so without Gui it wouldn't
> > build.
>
> Actually it's the link implementation that provides the include paths.
>
> You put the Qt5::Gui in the LINK_PUBLIC section. It should be in the
> LINK_PRIVATE section.
>
>
> http://community.kde.org/Frameworks/Epics/kdelibs_cleanups/link_private_details


Ok, will fix.

Cheers
-- 
Martin Klapetek | KDE Developer
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20130904/a6995525/attachment.html>