Review Request: Make DBusMenuQt optional

[Aaron Seigo]

> On 2010-08-06 07:10:08, Aaron Seigo wrote:
> > 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 :)
> 
> Ingo Klöcker 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 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.

"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 ;)


- Aaron


-----------------------------------------------------------
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/statusnotifieritem/CMakeLists.txt 1159836 
>   /branches/KDE/4.5/kdebase/workspace/plasma/generic/dataengines/statusnotifieritem/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 1159039 
> 
> Diff: http://reviewboard.kde.org/r/4898/diff
> 
> 
> Testing
> -------
> 
> Compiles w/o DBusMenuQt present.
> 
> 
> Thanks,
> 
> Marc
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20100806/5bb7053a/attachment.htm>
-------------- next part --------------
_______________________________________________
Plasma-devel mailing list
Plasma-devel at kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel