Discussion:
Kdiff3 in kdereview
Michael Reeves
2018-08-07 12:59:50 UTC
Permalink
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.
Albert Astals Cid
2018-08-23 22:07:49 UTC
Permalink
Post by Michael Reeves
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


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?

Cheers,
Albert
Albert Astals Cid
2018-08-28 21:45:47 UTC
Permalink
Post by Michael Reeves
Post by Michael Reeves
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.
Post by Michael Reeves
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?
Post by Michael Reeves
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.
Post by Michael Reeves
Cheers,
Albert
Michael Reeves
2018-08-28 23:04:45 UTC
Permalink
El divendres, 24 d’agost de 2018, a les 3:20:13 CEST, Michael Reeves va
El dimarts, 7 d’agost de 2018, a les 14:59:50 CEST, Michael Reeves va
Post by Michael Reeves
Kdiff3 has moved to review in preparation for possible release
testing. I
Post by Michael Reeves
am currently working towards having auto testing but the code needs
major
Post by Michael Reeves
refactoring to make this possible. Specifically it is not well
modularized.
Post by Michael Reeves
The purpose of this review is to get feedback on issues that need to
be
Post by Michael Reeves
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?
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
Albert Astals Cid
2018-08-29 20:34:24 UTC
Permalink
Post by Michael Reeves
Post by Michael Reeves
El dimarts, 7 d’agost de 2018, a les 14:59:50 CEST, Michael Reeves va
Post by Michael Reeves
Kdiff3 has moved to review in preparation for possible release
testing. I
Post by Michael Reeves
am currently working towards having auto testing but the code needs
major
Post by Michael Reeves
refactoring to make this possible. Specifically it is not well
modularized.
Post by Michael Reeves
The purpose of this review is to get feedback on issues that need to
be
Post by Michael Reeves
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?

That seems really strange, once you fix the assert if you tell me what to test i can have a look :)

Cheers,
Albert
Post by Michael Reeves
Post by Michael Reeves
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
Michael Reeves
2018-08-31 12:48:53 UTC
Permalink
El dimecres, 29 d’agost de 2018, a les 1:04:45 CEST, Michael Reeves va
Post by Michael Reeves
El divendres, 24 d’agost de 2018, a les 3:20:13 CEST, Michael Reeves va
El dimarts, 7 d’agost de 2018, a les 14:59:50 CEST, Michael Reeves
va
Post by Michael Reeves
Post by Michael Reeves
Kdiff3 has moved to review in preparation for possible release
testing. I
Post by Michael Reeves
am currently working towards having auto testing but the code
needs
Post by Michael Reeves
major
Post by Michael Reeves
refactoring to make this possible. Specifically it is not well
modularized.
Post by Michael Reeves
The purpose of this review is to get feedback on issues that
need to
Post by Michael Reeves
be
Post by Michael Reeves
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
Post by Michael Reeves
emit from DiffTextWindow so it's DiffTextWindow saying it wants to
scroll
Post by Michael Reeves
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.
Post by Michael Reeves
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
Post by Michael Reeves
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
Post by Michael Reeves
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
Post by Michael Reeves
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
Albert Astals Cid
2018-08-31 22:45:49 UTC
Permalink
Post by Michael Reeves
Post by Michael Reeves
Post by Michael Reeves
El divendres, 24 d’agost de 2018, a les 3:20:13 CEST, Michael Reeves va
El dimarts, 7 d’agost de 2018, a les 14:59:50 CEST, Michael Reeves
va
Post by Michael Reeves
Post by Michael Reeves
Kdiff3 has moved to review in preparation for possible release
testing. I
Post by Michael Reeves
am currently working towards having auto testing but the code
needs
Post by Michael Reeves
major
Post by Michael Reeves
refactoring to make this possible. Specifically it is not well
modularized.
Post by Michael Reeves
The purpose of this review is to get feedback on issues that
need to
Post by Michael Reeves
be
Post by Michael Reeves
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
Post by Michael Reeves
emit from DiffTextWindow so it's DiffTextWindow saying it wants to
scroll
Post by Michael Reeves
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
Post by Michael Reeves
Post by Michael Reeves
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.
Post by Michael Reeves
Post by Michael Reeves
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
Post by Michael Reeves
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
Post by Michael Reeves
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
Post by Michael Reeves
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
Michael Reeves
2018-09-03 15:38:35 UTC
Permalink
The memory leak should be gone with the latest commit. This also resolve s
one source of noise coming from code that is not needed as of QT 5.3.2.
Tested up to looking at a two directory diff and opening of the files. All
messages seem to be gone. Some unnecessary Windows specific code has been
removed from FileAccess. It now handles directory searches the same way on
any platform. Qt program's really shouldn't have to know the os they are
running on in most cases. That's kind of the point of using it in the first
place.
Post by Albert Astals Cid
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
Albert Astals Cid
2018-09-03 19:17:54 UTC
Permalink
Post by Michael Reeves
The memory leak should be gone with the latest commit. This also resolve s
one source of noise coming from code that is not needed as of QT 5.3.2.
Tested up to looking at a two directory diff and opening of the files. All
messages seem to be gone. Some unnecessary Windows specific code has been
removed from FileAccess. It now handles directory searches the same way on
any platform. Qt program's really shouldn't have to know the os they are
running on in most cases. That's kind of the point of using it in the first
place.
Sounds good :)

Cheers,
Albert
Post by Michael Reeves
Post by Albert Astals Cid
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
Michael Reeves
2018-09-14 16:36:10 UTC
Permalink
Can some do a clean install and see if right clicking on a file brings up
the kdiff3 context menu? I am troubleshooting a possible configuration
issue on my machine with Kubuntu. Currently this does not work for me.
Post by Albert Astals Cid
Post by Michael Reeves
The memory leak should be gone with the latest commit. This also resolve
s
Post by Michael Reeves
one source of noise coming from code that is not needed as of QT 5.3.2.
Tested up to looking at a two directory diff and opening of the files.
All
Post by Michael Reeves
messages seem to be gone. Some unnecessary Windows specific code has been
removed from FileAccess. It now handles directory searches the same way
on
Post by Michael Reeves
any platform. Qt program's really shouldn't have to know the os they are
running on in most cases. That's kind of the point of using it in the
first
Post by Michael Reeves
place.
Sounds good :)
Cheers,
Albert
Post by Michael Reeves
Post by Albert Astals Cid
Good :)
One minor thing, there seems to be some small issue with memory leaks
on
Post by Michael Reeves
Post by Albert Astals Cid
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
Post by Michael Reeves
Post by Albert Astals Cid
end (amogsnst others that are noise) https://paste.kde.org/pp5k6jc6u
Cheers,
Albert
Yuri Chornoivan
2018-09-14 17:56:06 UTC
Permalink
Post by Michael Reeves
Can some do a clean install and see if right clicking on a file brings up
the kdiff3 context menu? I am troubleshooting a possible configuration
issue on my machine with Kubuntu. Currently this does not work for me.
Confirmed to work (with Ukrainian translation) on Mageia 6 (kf5 5.42,
Applications 17.12, Dolphin context menu) with minor modifications to compile
on gcc 5 (remove new compilation frags and replacing "" with QString() where
it is appropriate).

Thanks for your work.

Best regards,
Yuri
Post by Michael Reeves
El dilluns, 3 de setembre de 2018, a les 17:38:35 CEST, Michael Reeves va
Post by Michael Reeves
The memory leak should be gone with the latest commit. This also resolve
s
Post by Michael Reeves
one source of noise coming from code that is not needed as of QT 5.3.2.
Tested up to looking at a two directory diff and opening of the files.
All
Post by Michael Reeves
messages seem to be gone. Some unnecessary Windows specific code has been
removed from FileAccess. It now handles directory searches the same way
on
Post by Michael Reeves
any platform. Qt program's really shouldn't have to know the os they are
running on in most cases. That's kind of the point of using it in the
first
Post by Michael Reeves
place.
Sounds good :)
Cheers,
Albert
Post by Michael Reeves
Post by Albert Astals Cid
Good :)
One minor thing, there seems to be some small issue with memory leaks
on
Post by Michael Reeves
Post by Albert Astals Cid
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
Post by Michael Reeves
Post by Albert Astals Cid
end (amogsnst others that are noise) https://paste.kde.org/pp5k6jc6u
Cheers,
Albert
Wolfgang Bauer
2018-09-15 00:26:37 UTC
Permalink
Post by Michael Reeves
Can some do a clean install and see if right clicking on a file brings up
the kdiff3 context menu?
You mean right-clicking on a file in dolphin?

Seems to work fine here with latest git master, the kdiff3 menu does show up,
and seems to work as expected.

That's openSUSE 42.3 with latest KF5 and Qt5 (not a clean install though).

Kind Regards,
Wolfgang
Michael Reeves
2018-09-15 16:16:08 UTC
Permalink
Yes I am going to setup a vm and see if this works. My system is having
trouble with this.
Post by Wolfgang Bauer
Post by Michael Reeves
Can some do a clean install and see if right clicking on a file brings up
the kdiff3 context menu?
You mean right-clicking on a file in dolphin?
Seems to work fine here with latest git master, the kdiff3 menu does show up,
and seems to work as expected.
That's openSUSE 42.3 with latest KF5 and Qt5 (not a clean install though).
Kind Regards,
Wolfgang
Loading...