On Tue, 2010-08-17 at 16:38 +0100, Robin Burchell wrote:
> Hi Jimmy (and other IVI folks),
>
> It's great to see you paid attention to my patches
> (http://lists.meego.com/pipermail/meego-dev/2010-August/004470.html for
> example),
> however, I have a few notes.
>
> 0) I'd like to be credited for work I do - it's fairly common
> practice, and it generally helps encourage people to do more of it in
> the future.
>
> This refers to ivihome commits a2f219cee109f2247aacd669c7243be4331fec68,
> c5c9eceda864a400418d71849cb01f07f3dd34e3, and commit
> d5c8e99be0d301c820e90bed7ba77ea2a101c45a from ividesktop.
>
> I understand that at least the ivihome project commit was modified from my
> submission, however, a "based on work by ____" isn't out of place there.
I am still new to accepting patches, so I'll make sure I keep this in
mind. A great thanks for the patch.
>
> 1) my patch notes that matchbox is also a runtime dependency, not just a
> build-time dependency. Is there any reason not to include this?
>
This is a documented in the packaging in the spec file, I'll add it to
the README.
> 2) there's a typo in the added 'running' section of README ('included' vs
> 'includes')
Fixed.
>
> 3) The project file now sets:
> INCLUDEPATH += $$PREFIX/include/libmb/
> .. and while this might sound like a good idea, it totally and utterly
> defeats the purpose of adding $$PREFIX in the first place, as now, if
> someone
> tries to use qmake PREFIX=/usr/local/, INCLUDEPATH will be incorrectly set,
> so it won't build at all. If it did, the install would work correctly,
> though. :)
>
> If libmb provides a pkg-config file (which I'm fairly sure it will), then
> the
> proper solution is to set CONFIG += link_pkgconfig, and PKGCONFIG += libmb
> (or similar
I am going to look at this further, probably using the PKGCONFIG values.
> ).
>
> 4) Not strictly a technical issue, but I personally disagree with shoving
> multiple, unrelated changes into a single commit. It tends to make
> collaboration and debugging very difficult.
>
Agree, I tried to split up the commits usually by features or fixes
added, they weren't intended to be unrelated changes:)
> Thanks for integrating!
>
> --
> Robin Burchell
> http://rburchell.com
_______________________________________________
MeeGo-dev mailing list
[email protected]
http://lists.meego.com/listinfo/meego-dev
