<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://reviewboard.kde.org/r/4898/">http://reviewboard.kde.org/r/4898/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On August 6th, 2010, 7:10 a.m., <b>Aaron Seigo</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">personally, i'm fine with the patch, if only for correctness in terms of "as few hard requirements as possible."
in practice, i find the usefulness of the patch to be dubious, but it can't hurt to at least allow it to be optional :)</pre>
</blockquote>
<p>On August 6th, 2010, 5:24 p.m., <b>Ingo Klöcker</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">KDAB could have chosen to take the route that the KOffice guys took, i.e. create (fork) their own private version of kdelibs by throwing out everything that they do not need. They did not choose this route. Instead they chose to work directly with upstream. This requires upstream not to block patches for dubious reasons. We don't do so in this case (which is good), but we still try to argue why the patch makes no sense. It makes no sense to us (which is perfectly okay), but apparently it does make sense for KDAB because otherwise they wouldn't have asked.</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">"Instead they chose to work directly with upstream."
which is great.
what you do miss in your observations is that the cost of "going your own" is also huge, so it isn't just an act of pure altruism to upstream patches. it's not only "nice guy behaviour" it's also "limit my future costs" behaviour.
"but we still try to argue why the patch makes no sense"
i don't see much argument at all; what i see is requests for clarification and knowledge being shared. it was generally collegial and productive. it's a patch that affects a core component and it was offered with ~zero explanation of motivation/reason for making the change. as a result, i'm not overly surprised at the resulting discussion.
"This requires upstream not to block patches for dubious reasons."
it also requires downstream to not push dubious patches or be disappointed when such patches are passed over because they are dubious. downstream can make incorrect judgments, too, and should take advantage of the review they get when pushing patches upstream as a way to get sanity checks for free. the "this is useless on mobile" idea, for instance, is a great example of that: it's particularly useful on mobile when there is a dbus host, something that seems may have been overlooked by some. :)
"but apparently it does make sense for KDAB because otherwise they wouldn't have asked."
that assumes KDAB (or any other group) is infallible, making every request sensible. i hope you agree that's an absurd idea. unfortunately in this case, the review request didn't actually say why it made sense to KDAB, so discussion was necessary to determine if it was a sensible change or not.
i do agree with you that we (KDE) need to make upstreaming of changes as easy as possible. (one of the main reasons we've championed the use of things like review board, in fact.) there was more discussion on this particular review request than may have been needed, that's debatable (though now with this reply i've certainly blown it ;). on the other hand, the original review request had desperately little information as to the motivations behind the patch, and so excessive discussion was almost inevitable. downstreams could learn from this: ensure that after going through the trouble of making a patch and posting it for review, spend a couple minutes explaining the reasons for the change. it helps a lot.
so .. we all have improvements to be made here. but to keep perspective, we're also dissecting what could have been better in a process that has ultimately worked: there's a well known place to post patches (review board), they are paid attention to (sometimes too much, it seems? :), it was accepted for inclusion, knowledge was shared. so it could be worse ;)</pre>
<br />
<p>- Aaron</p>
<br />
<p>On August 6th, 2010, 12:34 p.m., Marc Mutz wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://reviewboard.kde.orgrb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for kdelibs, Plasma and Marco Martin.</div>
<div>By Marc Mutz.</div>
<p style="color: grey;"><i>Updated 2010-08-06 12:34:16</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Make DBusMenuQt optional.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Compiles w/o DBusMenuQt present.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/branches/KDE/4.5/kdebase/workspace/CMakeLists.txt <span style="color: grey">(1159836)</span></li>
<li>/branches/KDE/4.5/kdebase/workspace/ConfigureChecks.cmake <span style="color: grey">(1159836)</span></li>
<li>/branches/KDE/4.5/kdebase/workspace/config-workspace.h.cmake <span style="color: grey">(1159836)</span></li>
<li>/branches/KDE/4.5/kdebase/workspace/plasma/generic/dataengines/statusnotifieritem/CMakeLists.txt <span style="color: grey">(1159836)</span></li>
<li>/branches/KDE/4.5/kdebase/workspace/plasma/generic/dataengines/statusnotifieritem/statusnotifieritemsource.cpp <span style="color: grey">(1159836)</span></li>
<li>/branches/KDE/4.5/kdelibs/CMakeLists.txt <span style="color: grey">(1159039)</span></li>
<li>/branches/KDE/4.5/kdelibs/ConfigureChecks.cmake <span style="color: grey">(1159039)</span></li>
<li>/branches/KDE/4.5/kdelibs/config.h.cmake <span style="color: grey">(1159039)</span></li>
<li>/branches/KDE/4.5/kdelibs/kdeui/CMakeLists.txt <span style="color: grey">(1159039)</span></li>
<li>/branches/KDE/4.5/kdelibs/kdeui/notifications/kstatusnotifieritem.cpp <span style="color: grey">(1159039)</span></li>
</ul>
<p><a href="http://reviewboard.kde.org/r/4898/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>