Bug 153463

Created attachment 22344 [details]
Fix Kompare in KDE 4

Here's the promised patch.

One more note: I also had to fix the text view which was only partly
implemented. It should fully work now (it did for me) and even use KatePart's
"Diff" highlighting.

2 more bugs I found (I'm also planning to fix those):
* "Blend URL with diff" crashes. (I didn't test that use case, bad me.)
* The "fileblend" icon for "Blend URL with diff" doesn't exist. It isn't there in KDE 3 either, so I'm going to just remove the icon.

Hmm, I haven't seen any of the people listed as kompare's maintainers list around... I'll ask around; if no one feels responsible for it, it would be super-cool if you adopted it..

Created attachment 22345 [details]
Fix Kompare in KDE 4 (rev 2)

This version fixes the 2 bugs related to blending URLs with diffs above. (The
crasher was due to using long-lived iterators on a QStringList, which no longer
works with the QList-based implementation of QStringList, I fixed it by copying
the QStringList into a QLinkedList<QString>.) It also fixes the Levenshtein
computation (the inline difference marking) to actually work. Who ever thought
it was a good idea to change this:
	const QChar* sq = s.unicode();
	const QChar* dq = d.unicode();
...
		dj = dq[ j ];

		for ( i = 1; i < m; ++i )
		{
			si = sq[ i ];
to this:
		dj = QString(dq[ j ]).toInt();

		for ( i = 1; i < m; ++i )
		{
			si = QString(sq[ i ]).toInt();
??? I fixed it to:
		dj = dq[j].unicode();

		for ( i = 1; i < m; ++i )
		{
			si = sq[i].unicode();

What's still broken though is the layouts in the startup and preferences
dialogs (the KPageDialogs).

Created attachment 22346 [details]
Fix Kompare in KDE 4 (rev 3)

A minor cleanup: I had copied the implementation of qSmartMinSize from Qt 3,
but actually the one from qlayoutengine_p.h in Qt 4 is exported already, the
reason I had unresolved references was that it was taking a QWidget * in Qt 3
and now takes a const QWidget * instead, so I fixed the declaration in
komparesplitter.cpp and removed the copy of qSmartMinSize. (Yes, they copied
the declaration because they didn't want to copy all of qlayoutengine_p.h. Now
that I copied all of QSplitter, I need some more stuff from there though. ;-) )

Created attachment 22375 [details]
Fix Kompare in KDE 4 (rev 4)

And here's a version with working layouts. This looks in good shape now.

To Winfried Dobbe:

Hi.. You seem to be the person behind the 3_way_kompare branch which the CMakeLists.txt in kdesdk states would replace the main kompare; but that seems to not have seen much activity... Any chance you could comment on this patch and overall direction of kompare?

Thanks.

Kevin,

Thanks for all the great patches.
Would you be interested in becoming the KCompare maintainer?

It seems that the 3_way_kompare development has stalled and certainly won't be ready for the 4.0.0. release.  We need something done very quickly for the January release so you'd need to devote lots of time in the near future to make it happen.

If you have questions/concerns you can write to me at winter@kde.org.
Let me know your decision as soon as you can.

Regards,
Allen, KDE Release Team

IMHO Kompare is probably good enough to ship in 4.0 with the patchset I submitted, but maybe I'm too optimistic there.

(Note: All the bugs and graphical glitches I noticed are fixed in the latest iteration of the patch, I wouldn't know what else to fix, unless of course a flood of bug reports comes in. Sure, it could use some porting away from Q3* and K3* stuff, but that's hardly what I'd call release-critical, I think having it working is more important, more Q3/K3* porting is actually likely to break things.)

Hi Marksim,

Actually Jeff Snyder is the person behind that branch. You can reach  
him at jeff@caffeinated.me.uk
I worked on it for just a few months.

regards,
Winfried


On Dec 6, 2007, at 6:08 PM, Maksim Orlovich wrote:

[bugs.kde.org quoted mail]

On Saturday 08 December 2007, Jeff Snyder wrote:
> I noticed you mentioned the splitter in some detail - and I dearly wish
> we'd talked sooner - because you seem to be developing against something
> less than the head of 3_way_kompare in svn. I did a significant bit of
> porting at akademy and shortly afterwards earlier this year which you
> seem to be missing.
> The head of 3_way_kompare has KompareSplitter ported to Qt4's qsplitter,
> which has the features needed to do what kompare wants (partly at my
> request). The code's far cleaner, ridding kompare of all the hacks which
> I made in kde3 and you've now brought forward into kde4.


See, I have no doubt the 3_way_kompare branch is the right code base to start 
from long term (KDE 4.1+). However, there is that TODO list of things needing 
to be done there, and I wanted to have a working Kompare as soon as possible, 
thinking both of the KDE 4.0.0 release in January and the Fedora 9 release in 
April.

I'll be entirely happy if that codebase gets replaced by a working 
3_way_kompare, I'm also willing to help making that happen as my time permits 
(do note that I have several other projects to work on though!), but right 
now my priority was having something which _works_. ;-) And that's what I've 
got now after 2 nights of work.

Don't get me wrong, I know having clean code is important long term and I know 
my straight port of the current kdesdk/kompare trunk definitely isn't clean 
code (there's not just the splitter hacks, but also lots of Qt3Support and 
kde3support junk), however I also think there is a phase in a release cycle 
in which the focus really has to be on getting things working. You may 
disagree, but my personal opinion is that it's better to have my hacked-up 
Kompare in 4.0 than no Kompare at all, it can always be replaced by a cleaner 
version from 3_way_kompare branch in 4.1, can't it?

        Kevin Kofler

> Getting back to the code:
>  - you may want to look at how QSplitter from kde4 is used in the
> branch, the migration to it is very easy - probably easier than taking
> the old hacks forward. It would also avoid the risk of kompare crashing
> when used with a qt version that's not exactly the same as yours.


I'll have a look. Does the branch currently compile? If not, I'll try to come 
up with a "get this beast to build" patch for that too, it shouldn't take 
longer than for the one from the trunk, hopefully.

>  - Is the main view now based on Q3ListView? and if so, were there any
> major problems with that move, or bugs arising from it?


I'm using a K3ListView, which works just like KListView worked.

We should really not be using either Q3ListView or K3ListView, but something 
like KListWidget/QListWidget or even the new MVC-based QListView.

> Lastly, there's the subject of replacing kompare trunk with kompare
> branch. It can (and must) of course be pushed back to kde4.1 or even
> later. I've got no problem with that, and I hope that I/we/someone can
> make it happen. But how serious are you about being part of it? I'm
> sorry if this sounds cynical, but your "as time permits" proviso sounds
> like open-source speak for "never gonna happen". If I'm wrong about
> that, I'd be very happy to know it ;-)


Don't worry, I'm serious when I say I'm willing to help you out with that. The 
problem is that it's kinda hard to predict the future. ;-) I think I will 
have some time for this, but I don't know what other stuff to do will come 
up.

(BTW, somehow your replies don't show up in the bug report's audit trail, 
despite your CC:153463@bugs.kde.org.)

        Kevin Kofler

Created attachment 22435 [details]
Fix 3_way_kompare branch build

Here's a patch to make the 3_way_kompare branch build (but not actually work,
it crashes when comparing files, maybe one of the bugs I already fixed in the
trunk, I have to check). Notes:
* I commented out the custom QStyle: QStyle had a lot of changes, so this needs
some porting of its own. I already ported a QStyle (Bluecurve), which was an
actual style and not just a wrapper, so I have the experience to do the port,
I'll do so in a followup patch, but I wanted to get things building first.
* About KSharedConfigPtr: that's a typedef now, so you can't forward-declare it
with "class KSharedConfigPtr", that's why I changed these to actual #includes.
* Just to make this clear: There are lots of TODOs in the 3_way_kompare branch,
both in the TODO file and in the code (search for TODO and FIXME), so I'm _not_
suggesting to merge the branch just now.

Created attachment 22441 [details]
3_way_kompare: port KompareDirStyle

Here's the promised followup patch which reenables the KompareDirStyle in the
3_way_kompare branch and ports it to Qt 4.

Created attachment 22443 [details]
Fix Kompare in KDE 4 (rev 5)

New patch against trunk:
* backports the KompareSplitter from 3_way_kompare branch (the backported
KompareSplitter combines KompareSplitter and KompareFileSplitter from
3_way_kompare because we don't have a KompareDirSplitter in trunk), eliminates
QSplitter hackery (no more forward-port of the Qt 3 QSplitter, no more (ab)use
of qlayoutengine_p.h). Yes, I tested this and found no regressions.
* fixes a warning about an undefined signal (KUrlSelecter::urlSelected(const
QString &)). This now passes a const KUrl & instead, but the signal handler
appears not to be needed at all anymore (files are already recognized as such
and displayed without the file:// prefix, so no need to hack the URL string for
that anymore), so I just removed it.

Created attachment 22466 [details]
Fix Kompare in KDE 4 (rev 6)

Whoops, I just tested saving and it didn't work (both saving diffs and saving
destination files failed due to 2 instances of the same bug), luckily the fix
was trivial: KTemporaryFile now requires an explicit open(), so I fixed the 2
uses to add that, 2 lines later and the bug is gone. :-) So here's revision 6
of the patch. Hopefully I got through all the menu items with my testing now.

SVN commit 747074 by winterz:

Apply patch5 from Kevin which makes kompare work for KDE 4.0.
It also ports over some things from the 3_way_kompare branch.
This is the first time that kompare builds in trunk.

I'm doing this for Kevin until he gets his own SVN account.
Oh, and Kevin is the new maintainer too!

CCMAIL: release-team@kde.org
BUGS: 153463


 M  +3 -3      CMakeLists.txt  
 M  +1 -2      doc/CMakeLists.txt  
 M  +1 -1      kompare/CMakeLists.txt  
 M  +69 -46    kompare/kompare_shell.cpp  
 M  +3 -1      kompare/kompare_shell.h  
 M  +1 -1      kompare/komparenavtreepart/CMakeLists.txt  
 M  +5 -18     kompare/komparenavtreepart/komparenavtreepart.cpp  
 M  +1 -2      kompare/komparenavtreepart/komparenavtreepart.h  
 M  +2 -1      kompare/komparepart/CMakeLists.txt  
 M  +30 -29    kompare/komparepart/kompare_part.cpp  
 M  +5 -4      kompare/komparepart/kompare_part.h  
 D             kompare/komparepart/kompare_qsplitter.h  
 M  +49 -40    kompare/komparepart/kompareconnectwidget.cpp  
 M  +16 -13    kompare/komparepart/kompareconnectwidget.h  
 M  +29 -23    kompare/komparepart/komparelistview.cpp  
 M  +14 -7     kompare/komparepart/komparelistview.h  
 M  +23 -11    kompare/komparepart/kompareprefdlg.cpp  
 M  +2 -2      kompare/komparepart/kompareprefdlg.h  
 A             kompare/komparepart/komparesaveoptionsbase.cpp   [License: UNKNOWN]
 A             kompare/komparepart/komparesaveoptionsbase.h   [License: UNKNOWN]
 M  +266 -307  kompare/komparepart/komparesaveoptionsbase.ui  
 M  +6 -6      kompare/komparepart/komparesaveoptionswidget.cpp  
 M  +245 -481  kompare/komparepart/komparesplitter.cpp  
 M  +35 -29    kompare/komparepart/komparesplitter.h  
 M  +24 -21    kompare/kompareurldialog.cpp  
 M  +27 -9     kompare/libdialogpages/diffpage.cpp  
 M  +1 -1      kompare/libdialogpages/diffpage.h  
 M  +22 -21    kompare/libdialogpages/diffsettings.cpp  
 M  +13 -36    kompare/libdialogpages/filespage.cpp  
 M  +1 -5      kompare/libdialogpages/filespage.h  
 M  +13 -12    kompare/libdialogpages/filessettings.cpp  
 M  +1 -1      kompare/libdialogpages/pagebase.cpp  
 M  +3 -5      kompare/libdialogpages/pagebase.h  
 M  +17 -5     kompare/libdialogpages/viewpage.cpp  
 M  +1 -1      kompare/libdialogpages/viewpage.h  
 M  +6 -6      kompare/libdialogpages/viewsettings.cpp  
 M  +55 -39    kompare/libdiff2/komparemodellist.cpp  
 M  +3 -2      kompare/libdiff2/komparemodellist.h  
 M  +2 -2      kompare/libdiff2/levenshteintable.cpp  
 M  +1 -1      kompare/libdiff2/parserbase.cpp  
 M  +2 -2      kompare/main.cpp  


WebSVN link: http://websvn.kde.org/?view=rev&revision=747074

Looks like my latest revision came just a few minutes too late, just a minute, I'm preparing a patch from rev 5 to 6. :-)

Created attachment 22467 [details]
Diff from rev 5 to 6

Here's the diff from rev 5 to 6. (Yes, it's just 2 lines added, each one fixes
a bug. :-) )

SVN commit 747076 by winterz:

From Kevin, fixes to patch5 I just committed.
CCBUGS: 153463


 M  +2 -0      komparemodellist.cpp  


WebSVN link: http://websvn.kde.org/?view=rev&revision=747076

> I'll reply in full later this week, mainly wrt the branch stuff - I
> havn't got time to do it justice right now.
>
> As far as the trunk version goes, you've got my blessing to do whatever
> you want with it, because everything you've done so far seems to be
> going in the right direction, and my main interest is in the branch, in
> equal parts.


Andreas Pakulat, one of the KDevelop maintainers, has also expressed interest 
in working on the 3_way_kompare branch, we really need to discuss the way to 
go forward so we don't step on each other's toes. By working together, we 
should be able to complete the unfinished work there.

        Kevin Kofler

Created attachment 22471 [details]
Fix Kompare build on non-x86

Arrrgh, now I try building that on non-x86 architectures in Fedora's Koji build
system (I'd have loved doing it sooner, but I couldn't build kdesdk because
subversion in Rawhide had broken dependencies) and it fails because it's trying
to link non-PIC .a files into shared libraries. (This is a problem even on
i386, by the way, because it creates text relocations and SELinux forbids these
by default.)

/usr/bin/ld:
/builddir/build/BUILD/kdesdk-3.97.0/x86_64-redhat-linux-gnu/lib/libdiff2.a(diffmodellist.o):
relocation R_X86_64_32S against `vtable for Diff2::DiffModelList' can not be
used when making a shared object; recompile with -fPIC
/builddir/build/BUILD/kdesdk-3.97.0/x86_64-redhat-linux-gnu/lib/libdiff2.a:
could not read symbols: Bad value
collect2: ld returned 1 exit status

I tried to make libdiff2 and libdialogpages shared libraries first, but this
needs export macros and so on, I'm not sure it's worth it for internal
convenience libraries like these. (I can attach my preliminary patch, but it
doesn't currently build.) So here's a patch which builds the libraries with
${CMAKE_SHARED_LIBRARY_C_FLAGS}, that's -fPIC or equivalent on targets which
support/require it and empty on others. With that patch, it builds on at least
i386, x86_64, ppc and ppc64 (the architectures Fedora is built for), see:
http://koji.fedoraproject.org/koji/buildinfo?buildID=27666

As my KDE SVN account is still pending creation, can someone please commit this
for me?

Actually I'd like to have at least libdiff as shared library. KDevelops teamwork plugin currently uses a copy of kompare's libdiff with a few modifications (mostly to make it usable standalone) so it might make sense to share that stuff instead of copying it around.

I don't know about the libdialog stuff, but using -fPIC is AFAIK not an option for KDE code. Apart from that its quite easy to just add the files to the real target and not build any convenience libs at all.

I'm not hardcoding -fPIC for a reason, I'm using ${CMAKE_SHARED_LIBRARY_C_FLAGS} which should be correct for all targets!

And the situation right now is that kdesdk doesn't build at all on most non-i386 targets, so I think this fix really needs to get in at least as a temporary solution.

Created attachment 22473 [details]
INCOMPLETE patch to build libdiff2 and libdialogpages shared

Here's the patch to build the libraries shared, but that one does NOT work yet,
it's missing the export magic, so things linking against the shared libraries
end up with unresolved symbols.

Let me fix that later today, I've gotta go to work now. It shouldn't take more than an hour or so to fix the exports.

AFAIK its not allowed in KDE4 to link a static library into a shared library or plugin, no matter wether you use -fPIC or some other compiler switch.

But the code currently in SVN already does that (Kompare always did that even before I resurrected it, it was just masked because it didn't build) and is broken, with the patch, it still does it (bad), but at least it builds, so isn't it an improvement? I'm not saying this should be the permanent solution, just that it should go in as a quick fix to unbreak the build!

> Let me fix that later today, I've gotta go to work now. It shouldn't take
> more than an hour or so to fix the exports.

Many thanks for that, your help is appreciated (especially for unbreaking something I accidentally broke ;-) ).

I know the code always did it, this is leftover from autotools buildsystem. In KDE3 any plugin or shared library that wanted to separate different parts of its code into subdirs needed a static library to do so. However with CMake thats not necessary anymore, as cmake supports putting source files into subdirs or even completely different dirs (i.e. parent dir, parent-sibling-dir).

I have to have a closer look at the code to see wether thats feasible, problem with the above approach is if you have files that are shared for multiple targets, i.e. a plugin and an executable you end up compiling them twice.

Anyway, the easiest hotfix is disabling kompare from the build until we figured out the right thing to do. I'm going to do that now.

SVN commit 747254 by apaku:

Make the diff2 and dialogpages shared libraries including proper export macros. This should fix the build issues on non-x86.
Note: These two libraries are not so-versioned because they are only support
libraries for the kompare KPart and nothing else should link against them.
CCMAIL:kevin.kofler@chello.at
CCBUG:153463


 M  +1 -1      CMakeLists.txt  
 M  +1 -71     kompare/CMakeLists.txt  
 A             kompare/interfaces/kompareinterfaceexport.h   [License: LGPL (v2+)]
 M  +1 -1      kompare/komparenavtreepart/CMakeLists.txt  
 M  +1 -1      kompare/komparepart/CMakeLists.txt  
 M  +3 -43     kompare/libdialogpages/CMakeLists.txt  
 A             kompare/libdialogpages/dialogpagesexport.h   [License: LGPL (v2+)]
 M  +2 -1      kompare/libdialogpages/diffpage.h  
 M  +2 -1      kompare/libdialogpages/diffsettings.h  
 M  +2 -1      kompare/libdialogpages/filespage.h  
 M  +2 -1      kompare/libdialogpages/filessettings.h  
 M  +2 -1      kompare/libdialogpages/viewpage.h  
 M  +2 -1      kompare/libdialogpages/viewsettings.h  
 M  +3 -48     kompare/libdiff2/CMakeLists.txt  
 A             kompare/libdiff2/diff2export.h   [License: LGPL (v2+)]
 M  +4 -3      kompare/libdiff2/difference.h  
 M  +2 -1      kompare/libdiff2/diffmodel.h  
 M  +2 -1      kompare/libdiff2/diffmodellist.h  
 M  +3 -1      kompare/libdiff2/kompare.h  
 M  +2 -1      kompare/libdiff2/komparemodellist.h  


WebSVN link: http://websvn.kde.org/?view=rev&revision=747254

I don't really like the unversioned libraries. It makes things harder for packaging (-devel packages) because you have to special-case the unversioned .so (not the usual devel symlink). I'd much prefer there to be a SOVERSION, I don't care what version exactly, but there should be one.

SVN commit 749452 by kkofler:

Fix 3_way_compare branch build (at least on x86, the branch still has the non-PIC static library problem apaku fixed in the trunk) (applied my 2 patches from bug 153463: the build fix and the KompareDirStyle port).
CCBUG: 153463

 M  +1 -1      CMakeLists.txt  
 M  +1 -1      diffimpl/CMakeLists.txt  
 M  +1 -1      interfaces/kompareinterface.h  
 M  +2 -4      kompare_shell.cpp  
 M  +2 -2      kompare_shell.h  
 M  +3 -3      komparepart/kompare_part.cpp  
 M  +0 -3      komparepart/kompareconnectwidget.h  
 M  +9 -10     komparepart/komparedirlistview.cpp  
 M  +91 -66    komparepart/komparedirstyle.cpp  
 M  +24 -20    komparepart/komparedirstyle.h  
 M  +7 -6      komparepart/komparefilelistview.cpp  
 M  +2 -2      komparepart/komparefilelistview.h  
 M  +10 -10    komparepart/komparemininavwidget.cpp  
 M  +9 -5      komparepart/komparenavframe.cpp  
 M  +23 -12    komparepart/kompareprefdlg.cpp  
 M  +5 -5      komparepart/komparesaveoptionswidget.cpp  
 M  +0 -6      komparepart/komparesplitter.h  
 M  +1 -1      libdialogpages/CMakeLists.txt  
 M  +1 -1      libdialogpages/diffpage.h  
 M  +1 -2      libdialogpages/diffsettings.cpp  
 M  +1 -1      libdialogpages/filespage.h  
 M  +1 -0      libdialogpages/filessettings.cpp  
 M  +1 -1      libdialogpages/pagebase.h  
 M  +1 -1      libdialogpages/settingsbase.h  
 M  +1 -1      libdialogpages/viewpage.h  


WebSVN link: http://websvn.kde.org/?view=rev&revision=749452