<br><br><div class="gmail_quote">On Fri, Oct 28, 2011 at 4:27 AM, Jeremy Whiting <span dir="ltr"><<a href="mailto:jpwhiting@kde.org">jpwhiting@kde.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<div class="im">On Thu, Oct 27, 2011 at 7:14 PM, Kevin Kofler <<a href="mailto:kevin.kofler@chello.at">kevin.kofler@chello.at</a>> wrote:<br>
> On Friday 28 October 2011, Jeremy Whiting wrote:<br>
>> Sorry for cross-posting, I'm not sure who is on which list. We can<br>
>> continue this discussion on one or the other, but I wanted anyone<br>
>> interested to be able to chime in.<br>
><br>
</div>> I'm only on kompare-devel. I'm the current maintainer of Kompare.<br>
<div class="im">><br>
>> As you may or may not know, libdiff2 is used in kdesdk/kompare and<br>
>> kdevelop/kdevplatform/plugins/patchreview (one is a fork/copy of the<br>
>> other). I'd like to bring these back together into one library that<br>
>> is exported like a real library so both places can use it.<br>
><br>
</div>> I'm very much in favor of that. In fact, I still don't understand why KDevelop<br>
> forked the library in the first place, since I accepted all the API additions<br>
> they sent me patches for, and also told them that I won't complain if they<br>
> just commit the API additions directly as long as they don't break Kompare.<br>
<br>
>From what I understand they (kdevelop devs) told me it was just a<br>
misunderstanding. So lets fix it :)<br>
<div class="im"><br>
><br>
>> Where it lives is still up for debate, but probably kdelibs or kdesupport or<br>
>> something.<br>
><br>
</div>> Why can't kdevelop just depend on kdesdk? It's the natural place for libdiff2<br>
> to live. And there's already the optional dependency on Okteta which is also<br>
> in kdesdk. For packagers (and there I can speak with my Fedora packager hat<br>
> on), optional vs. required doesn't make that much of a difference, we want to<br>
> build against kdesdk anyway.<br>
<br>
That would probably work. KDevelop guys, is there a problem with<br>
depending on kdesdk?<br></blockquote><div>Isn't kdesdk moving to git at all? I think depending on such a big module is bad and I don't see the benefit of not having a libdiff project.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
I see in the patchreview plugin there is some #if 0 around #ifdef<br>
HAVE_KOMPARE could that dependency come back?<br></blockquote><div>That was something different (trying to integrate the KPart), I don't think the patchreview can be built without libdiff. We can make the patchreview building optional if libdiff is not found, for sure.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<div class="im"><br>
>> In komparemodellist.cpp saveDestination the version in kdesdk checks<br>
>> for opening a file and emits an error if it can't open. Curiously the<br>
>> version in kdevelop does the opposite, it sends an error if it is able<br>
>> to open the file. My only guess is that this code is not used in the<br>
>> kdevelop patchreview code. If anyone knows why it is the way it is in<br>
>> kdevelop copy I'd like to hear otherwise I'll use the version from<br>
>> kdesdk.<br>
><br>
</div>> IIRC, this is a bug we fixed in Kompare, the fix didn't reach the KDevelop fork.<br>
<br>
Awesome.<br>
<div class="im"><br>
>> Towards the end of that method in the kdevelop version it has<br>
>> an if false where in the kdesdk version it checks for a temp close<br>
>> error, not sure why.<br>
><br>
</div>> That, too, is a bugfix in Kompare which didn't reach the fork.<br>
<br>
Cool.<br>
<div class="im"><br>
>> Similarly in the kompare version of komparemodellist.cpp there are a<br>
>> bunch of actions in the ctor that are just commented out in the<br>
>> kdevelop version. Maybe komparemodellist should move out of libdiff2<br>
>> and into kompare itself? Alternatively we could check at runtime if<br>
>> our parent is a KomparePart and initialize the actions only then, so<br>
>> kdevelop could still use the model.<br>
><br>
</div>> Now the question is, why are the actions not wanted in KDevelop?<br>
<br>
Good question. kdevelop guys?<br></blockquote><div><br></div><div>I'm not sure why is that, probably we can have them if they don't appear in the UI.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<br>
> Of course, we could use qobject_cast or QObject::inherits or something like<br>
> that on the parent.<br>
<div class="im">><br>
>> One final difference is that the kompare version has the idea of a<br>
>> malformed patch and keeps track of that throughout komparemodellist,<br>
>> parser, and parserbase. My instinct is to take the kdesdk version as<br>
>> that seems like an addition.<br>
><br>
</div>> Yes, this is something I added to Kompare.<br>
><br>
> That said, there's a known issue with this: the detection is too strict and<br>
> reports some patches as malformed when they aren't actually malformed, but<br>
> just have legitimate text such as "Binary files foo and bar differ" between<br>
> hunks. I haven't gotten around to fixing this. (I think it should only complain<br>
> about junk lines if they're diff lines, i.e. start with a ' ', '+', '-', '<',<br>
> '>' or such character.)<br>
<br>
That doesn't sound too tricky to fix. If you don't get to it I'll<br>
take a look soonish.<br>
<div class="im"><br>
>> comments on any of the above are welcome.<br>
><br>
</div>> My main comment: folks, forking is bad. Don't do it! Just depend on libdiff2<br>
> from kdesdk, packagers can easily accomodate that.<br>
><br>
> Kevin Kofler<br>
><br>
<br>
Agreed.<br>
<br>
That said in comparing the two versions I merged some things locally<br>
from kdevelop version to kdesdk version. I've attached a patch to<br>
kdesdk version with those improvements. I suggest these changes get<br>
applied to kdesdk version and after the dependency issue is resolved<br>
the forked version go away.<br>
<br>
BR,<br>
<font color="#888888">Jeremy<br>
</font><br>--<br>
KDevelop-devel mailing list<br>
<a href="mailto:KDevelop-devel@kdevelop.org">KDevelop-devel@kdevelop.org</a><br>
<a href="https://barney.cs.uni-potsdam.de/mailman/listinfo/kdevelop-devel" target="_blank">https://barney.cs.uni-potsdam.de/mailman/listinfo/kdevelop-devel</a><br>
<br></blockquote></div><br><div>Thanks a lot for your work, Jeremy, I don't like to have two competing code bases in KDE, it's healthier to share both binaries and source code, there's no doubt of that.</div>
<div><br></div><div>Regarding dependencies, I'd like to note that in KDE we're shifting to aim for smaller dependencies, I won't make any speech out of that, I just think other people might be interested in this library too and a small dependency is always better.</div>
<div><br></div><div>I hope that helped :)</div><div>Aleix</div>