Kdiff3 in kdereview

[Michael Reeves]

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.

>
> 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
> > > > >
> > > > >
> > >
> > >
> > >
> > >
> > >
>
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20180831/54a5f589/attachment.htm>