[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-panel-devel
Subject: Re: Review Request: Make DBusMenuQt optional
From: "Marc Mutz" <marc () kdab ! com>
Date: 2010-08-06 20:27:23
Message-ID: 20100806202723.19646.68177 () vidsolbach ! de
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
> On 2010-08-06 07:10:08, Aaron Seigo wrote:
> > personally, i'm fine with the patch, if only for correctness in terms o=
f "as few hard requirements as possible."
> > =
> > in practice, i find the usefulness of the patch to be dubious, but it c=
an't hurt to at least allow it to be optional :)
> =
> Ingo Kl=C3=B6cker wrote:
> 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 eve=
rything 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), b=
ut 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 b=
ecause otherwise they wouldn't have asked.
> =
> Aaron Seigo wrote:
> "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 pa=
tches. 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 clarific=
ation 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 no=
t 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 disappo=
inted when such patches are passed over because they are dubious. downstrea=
m can make incorrect judgments, too, and should take advantage of the revie=
w 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 exampl=
e of that: it's particularly useful on mobile when there is a dbus host, so=
mething that seems may have been overlooked by some. :)
> =
> "but apparently it does make sense for KDAB because otherwise they wo=
uldn't have asked."
> =
> that assumes KDAB (or any other group) is infallible, making every re=
quest sensible. i hope you agree that's an absurd idea. unfortunately in th=
is 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 parti=
cular review request than may have been needed, that's debatable (though no=
w with this reply i've certainly blown it ;). on the other hand, the origin=
al review request had desperately little information as to the motivations =
behind the patch, and so excessive discussion was almost inevitable. downst=
reams could learn from this: ensure that after going through the trouble of=
making a patch and posting it for review, spend a couple minutes explainin=
g the reasons for the change. it helps a lot.
> =
> so .. we all have improvements to be made here. but to keep perspecti=
ve, 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 boar=
d), they are paid attention to (sometimes too much, it seems? :), it was ac=
cepted for inclusion, knowledge was shared. so it could be worse ;)
Disclaimer: the patch came from me, not from KDAB. When upgrading from 4.4 =
to 4.5 I found it irritating that a new hard dependency had been introduced=
to what seemed at that time a minor feature and fixed the glitch</officesp=
ace>. That this patch is actually relevant to KDAB's mobile work occurred t=
o me only after posting the patch for review. I'd like to apologize for the=
lack of rationale in the original request (won't happen again) and the CO2=
emissions this thread has caused...
- Marc
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/4898/#review6833
-----------------------------------------------------------
On 2010-08-06 12:34:16, Marc Mutz wrote:
> =
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/4898/
> -----------------------------------------------------------
> =
> (Updated 2010-08-06 12:34:16)
> =
> =
> Review request for kdelibs, Plasma and Marco Martin.
> =
> =
> Summary
> -------
> =
> Make DBusMenuQt optional.
> =
> =
> Diffs
> -----
> =
> /branches/KDE/4.5/kdebase/workspace/CMakeLists.txt 1159836 =
> /branches/KDE/4.5/kdebase/workspace/ConfigureChecks.cmake 1159836 =
> /branches/KDE/4.5/kdebase/workspace/config-workspace.h.cmake 1159836 =
> /branches/KDE/4.5/kdebase/workspace/plasma/generic/dataengines/statusno=
tifieritem/CMakeLists.txt 1159836 =
> /branches/KDE/4.5/kdebase/workspace/plasma/generic/dataengines/statusno=
tifieritem/statusnotifieritemsource.cpp 1159836 =
> /branches/KDE/4.5/kdelibs/CMakeLists.txt 1159039 =
> /branches/KDE/4.5/kdelibs/ConfigureChecks.cmake 1159039 =
> /branches/KDE/4.5/kdelibs/config.h.cmake 1159039 =
> /branches/KDE/4.5/kdelibs/kdeui/CMakeLists.txt 1159039 =
> /branches/KDE/4.5/kdelibs/kdeui/notifications/kstatusnotifieritem.cpp 1=
159039 =
> =
> Diff: http://reviewboard.kde.org/r/4898/diff
> =
> =
> Testing
> -------
> =
> Compiles w/o DBusMenuQt present.
> =
> =
> Thanks,
> =
> Marc
> =
>
[Attachment #5 (text/html)]
<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>
<p>On August 6th, 2010, 6:38 p.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;">"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> </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;">Disclaimer: the patch \
came from me, not from KDAB. When upgrading from 4.4 to 4.5 I found it irritating \
that a new hard dependency had been introduced to what seemed at that time a minor \
feature and fixed the glitch</officespace>. That this patch is actually \
relevant to KDAB's mobile work occurred to me only after posting the patch for \
review. I'd like to apologize for the lack of rationale in the original request \
(won't happen again) and the CO2 emissions this thread has caused...</pre> <br />
<p>- Marc</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>
_______________________________________________
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic