Re: Kdiff3 in kdereview
Would there be any objections to moving Kdiff3 to extragear. I looking to do a test release by the end of Feb. On Nov 14, 2018 10:30 AM, "Michael Reeves" wrote: Its been two months now has any found problems that must be resolved. I am currently working my through the code tring to update it with a more modular approach. At present it is nightmare trying sort through the code a decipher what going on much less change anything. The current approach uses the class keyword but doesn't isolate class from eachother either in terms of the file they are in or member variable access. All classes are switching to accessore funtion and private member data as a first step in cleanup. In FileAccess the code is now much cleaner and relies on QT/KIO for most of the work. QDir::setCurrent calls have been removed as they introduced an unnecessary point of failure. On Sat, Sep 15, 2018, 12:16 PM Michael Reeves Yes I am going to setup a vm and see if this works. My system is having > trouble with this. > > On Sat, Sep 15, 2018, 10:30 AM Wolfgang Bauer wrote: > >> Am Freitag, 14. September 2018, 16:36:10 schrieb 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 >> >>
Re: Kdiff3 in kdereview
Its been two months now has any found problems that must be resolved. I am currently working my through the code tring to update it with a more modular approach. At present it is nightmare trying sort through the code a decipher what going on much less change anything. The current approach uses the class keyword but doesn't isolate class from eachother either in terms of the file they are in or member variable access. All classes are switching to accessore funtion and private member data as a first step in cleanup. In FileAccess the code is now much cleaner and relies on QT/KIO for most of the work. QDir::setCurrent calls have been removed as they introduced an unnecessary point of failure. On Sat, Sep 15, 2018, 12:16 PM Michael Reeves Yes I am going to setup a vm and see if this works. My system is having > trouble with this. > > On Sat, Sep 15, 2018, 10:30 AM Wolfgang Bauer wrote: > >> Am Freitag, 14. September 2018, 16:36:10 schrieb 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 >> >>
Re: Kdiff3 in kdereview
Yes I am going to setup a vm and see if this works. My system is having trouble with this. On Sat, Sep 15, 2018, 10:30 AM Wolfgang Bauer wrote: > Am Freitag, 14. September 2018, 16:36:10 schrieb 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 > >
Re: Kdiff3 in kdereview
Am Freitag, 14. September 2018, 16:36:10 schrieb 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
Re: Kdiff3 in kdereview
пʼятниця, 14 вересня 2018 р. 19:36:10 EEST 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 > On Mon, Sep 3, 2018 at 7:18 PM Albert Astals Cid wrote: > > El dilluns, 3 de setembre de 2018, a les 17:38:35 CEST, Michael Reeves va > > > > escriure: > > > 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 > > > > > On Fri, Aug 31, 2018, 6:45 PM Albert Astals Cid wrote: > > > > 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
Re: Kdiff3 in kdereview
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. On Mon, Sep 3, 2018 at 7:18 PM Albert Astals Cid wrote: > El dilluns, 3 de setembre de 2018, a les 17:38:35 CEST, Michael Reeves va > escriure: > > 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 > > > > > On Fri, Aug 31, 2018, 6:45 PM Albert Astals Cid wrote: > > > > > > > > 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 > > > > > > > > > > > > > > > > > > >
Re: Kdiff3 in kdereview
El dilluns, 3 de setembre de 2018, a les 17:38:35 CEST, Michael Reeves va escriure: > 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 > > On Fri, Aug 31, 2018, 6:45 PM Albert Astals Cid wrote: > > > > > 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 > > > > > > > > >
Re: Kdiff3 in kdereview
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. On Fri, Aug 31, 2018, 6:45 PM Albert Astals Cid wrote: > > 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 > > > >
Re: Kdiff3 in kdereview
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 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 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 > > 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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
Re: Kdiff3 in kdereview
On Wed, Aug 29, 2018, 4:34 PM Albert Astals Cid 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 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 > 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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
Re: Kdiff3 in kdereview
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 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 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? That seems really strange, once you fix the assert if you tell me what to test i can have a look :) Cheers, Albert > > > > > > > > > > > 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 > > > > > > > > > > > > > > > > > >
Re: Kdiff3 in kdereview
On Tue, Aug 28, 2018, 5:45 PM Albert Astals Cid 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 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? > > > > > > > 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 > > > > > > > > > > >
Re: Kdiff3 in kdereview
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 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? > > > > 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 > > > >
Re: Kdiff3 in kdereview
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 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