Kdiff3 in kdereview

Albert Astals Cid aacid at kde.org
Fri Aug 31 23:45:49 BST 2018


El divendres, 31 d’agost de 2018, a les 14:48:53 CEST, Michael Reeves va escriure:
> On Wed, Aug 29, 2018, 4:34 PM Albert Astals Cid <aacid at kde.org> wrote:
> 
> > El dimecres, 29 d’agost de 2018, a les 1:04:45 CEST, Michael Reeves va
> > escriure:
> > > On Tue, Aug 28, 2018, 5:45 PM Albert Astals Cid <aacid at kde.org> wrote:
> > >
> > > > El divendres, 24 d’agost de 2018, a les 3:20:13 CEST, Michael Reeves va
> > > > escriure:
> > > > > On Thu, Aug 23, 2018, 6:07 PM Albert Astals Cid <aacid at kde.org>
> > wrote:
> > > > >
> > > > > > El dimarts, 7 d’agost de 2018, a les 14:59:50 CEST, Michael Reeves
> > va
> > > > > > escriure:
> > > > > > > Kdiff3 has moved to review in preparation for possible release
> > > > testing. I
> > > > > > > am currently working towards having auto testing but the code
> > needs
> > > > major
> > > > > > > refactoring to make this possible. Specifically it is not well
> > > > > > modularized.
> > > > > > > The purpose of this review is to get feedback on issues that
> > need to
> > > > be
> > > > > > > addressed before moving out of playground.
> > > > > >
> > > > > > Have you seen there's 4 wrong connect signals on startup?
> > > > > > https://paste.kde.org/pcvcje3u1
> > > > > >
> > > > > Not quite sure how to resolve this. How is scrolling content properly
> > > > > implemented in qt5?
> > > >
> > > > I don't understand the question, what is missing is the signal you
> > would
> > > > emit from DiffTextWindow so it's DiffTextWindow saying it wants to
> > scroll
> > > > that is not something that it doesn't say anymore?
> > > >
> > >
> > > The code is a hack by the original author that tries to get notified when
> > > the scroll bar moves. It happens to work as of qt5 but generates this
> > > warning because QWidget::scroll is not a signal. Removing the offending
> > > connects makes text in the diff mini window not scroll at all. How is
> > > scrolling of content supposed to be implemented?
> >
> > Are you saying that removing a connect that is reporting to be broken
> > changes the behaviour of the program?
> >
> 
> I must be crazy anyway it seems to work now without those lines.

Good :)

One minor thing, there seems to be some small issue with memory leaks on optiondialog.

If you compile with
   cmake -DECM_ENABLE_SANITIZERS="undefined;address

and then just run kdiff3 and close it, i get these leaks reported at the end (amogsnst others that are noise) https://paste.kde.org/pp5k6jc6u

Cheers,
  Albert


> 
> >
> > That seems really strange, once you fix the assert if you tell me what to
> > test i can have a look :)
> >
> > Cheers,
> >   Albert
> >
> 
> The assert will no longer happen that actually was committed right after
> you reported it. The code in question seems to be triggered when using
> preprocessing comands. I still need to look at this more closely but it
> should work as is. I have been reworking the file access code to try and
> simplify it. The diff process itself should not be affected buy this. This
> code base definitely feels something that was written under time
> constraints. It took a lot of effort to make it what is now. Not
> surprisingly there's still work to be done. One of my goals is to reduce
> make this code more easily maintainable.
> 
> 
> > >
> > > >
> > > > > >
> > > > > > When trying to compare any two files i hit this assert
> > > > > >
> > > > > >         if(m_lmppData.m_vSize < m_normalData.m_vSize)
> > > > > >         {
> > > > > >             //This a bug that needs fixed elsewhere not hacked
> > around
> > > > > >             Q_ASSERT(m_lmppData.m_vSize == m_normalData.m_vSize);
> > > > > >
> > > > > > Which i do not understand what it is trying to do, i mean you just
> > > > checked
> > > > > > that they are different and the on the next line assert they are
> > not
> > > > > > different?
> > > > > >
> > > > >
> > > > > Actually that tells me what I need ed to know. I don't get this on my
> > > > > machine. The comment made it seem like this was some sort of work
> > around
> > > > > for an odd corner case. I can remove the assert now that I know the
> > > > trigger
> > > > > is an everytime thing.
> > > > > How are you doing the file comparison?
> > > >
> > > > kdiff3 file1 file2
> > > >
> > > > Cheers,
> > > >   Albert
> > > >
> > > >
> > > > > I feel like this points to an issue else where.
> > > > >
> > > > >
> > > > > > Cheers,
> > > > > >   Albert
> > > > > >
> > > > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> >
> >
> >
> >
> >








More information about the kde-core-devel mailing list