KSnapshots: fixed remaining issues related to PR #50. #52

As per title. Refers to PR #50 for the description of the issues.

It looks good.

+    connect(this, TQT_SIGNAL(finished()), TQT_SLOT(slotFinished()));

Do we need an additional slot for this? Why not put it in KSnapshot's destructor?

+    connect(this, TQT_SIGNAL(finished()), TQT_SLOT(slotFinished()));
Do we need an additional slot for this? Why not put it in KSnapshot's destructor?

Ah yes, excellent question and the answer is "yes, we need a slot" because when KSnapshot is closed, the destructor ~KSnapshot() does not get called!! In fact I had initially put the code there and it didn't work. After searching the docs, I added slotFinished() which is invoked when the dialog is closed.

@MicheleC

+    connect(this, TQT_SIGNAL(finished()), TQT_SLOT(slotFinished()));
Do we need an additional slot for this? Why not put it in KSnapshot's destructor?
Ah yes, excellent question and the answer is "yes, we need a slot" because when KSnapshot is closed, the destructor ~KSnapshot() does not get called!! In fact I had initially put the code there and it didn't work. After searching the docs, I added slotFinished() which is invoked when the dialog is closed.

Strange. Could this be pointing to a memory handling error? Because it would certainly seem that the KSnapshot object does not get deleted automatically, as it should. I don't see anything out of place in main.cpp. TDEApplication should automatically delete its main widget on exit and that would call its destructor. Anyway, I understand the need for the slot, thanks for clarifying.

Strange. Could this be pointing to a memory handling error? Because it would certainly seem that the KSnapshot object does not get deleted automatically, as it should. I don't see anything out of place in main.cpp. TDEApplication should automatically delete its main widget on exit and that would call its destructor. Anyway, I understand the need for the slot, thanks for clarifying.

I have been thinking hard on why the destructor does not get called, so I think it may indicates a possible memory leak (KSnapshot object not getting deleted). I remember some similar from some time in the past when working on some other part of TDE.
KSnapshot gets created with a null parent, meaning is should become an object tree in TQt object system and this should get cleared automatically by TQt when the application exits the event loop (there is a post routine added exactly for that reason in TQObject). But it does not seem to happen.
There is clearly a bug somewhere, but I don't think it is specific to KSnapshot but rather to TQt3/tdelibs and it would require separate investigation.
I will add a bug report for this, it will be good to fix it for R14.1.x or R14.2.0 (depending on whether API changes are needed for it).

I think I have encountered similar behaviour in other applications too, so it's definitely a TQt or TDELibs issue. I'll try to look into it too, but I have still limited knowledge of TQt internals. It is important to fix it anyway.

@blu.256 so ok to proceed with merging?

I'm unable to test it ATM but it certainly looks ok.

I think I have encountered similar behaviour in other applications too, so it's definitely a TQt or TDELibs issue. I'll try to look into it too, but I have still limited knowledge of TQt internals. It is important to fix it anyway.

Yes, I agree. For R14.2.0 I am planning to do some updates on TQt3, because the object tree system as it is now could be very slow (see my recent work on the TDEIO::Job speed improvement). Qt4 code has some significant differences there, so it will be good to make TQt3 better!

I'm unable to test it ATM but it certainly looks ok.

I will wait till you do a test, there is no rush. I had assumed you had already tested :-)

Just tested it, works well.

On another note, I accidentally forgot to remove a connection no longer needed in PR #50 (which was also introduced as part of this same PR, oops!). I'll sneak in a commit to fix this problem in this PR. Hope that's okay.

[2023/03/01 21:06:43.433] TQObject::connect: No such slot KSnapshotWidget::slotOpenWithClicked()
[2023/03/01 21:06:43.433] TQObject::connect:  (sender name:   'btnOpenWith')
[2023/03/01 21:06:43.433] TQObject::connect:  (receiver name: 'mainWidget')

On slotFinished(): I just noticed there is a closeEvent() function. Maybe we could use that instead?

Hi,
sorry I was busy and just now read all your messages. I don't have anything to add - Philippe even fixed the one in ksnapshotwidget.ui which I was going to mention.
The only thing is that the commit does not seem to be signed.

And thank you a lot for all your effort and ironing and polishing. It is indeed great to see how you work together.

BR

On slotFinished(): I just noticed there is a closeEvent() function. Maybe we could use that instead?

I did further testing following your comment and found out that neither slotFinished nor closeEvent were called if KSnapshot was being closed from a dcop call. So I modified the previous code to use the TQApplication's aboutToQuit signal and do everything in the connected slot.
Note that we still need the closeEvent slot, otherwise the KDialog won't close correctly (and become irresponsive) since the event is not accepted (I am surprised this is not the default behavior if the slot is not defined, but anyway...)

I'll sneak in a commit to fix this problem in this PR. Hope that's okay.

Not a problem at all. Since I had to make changes to my previous commit too, I took the opportunity to squash your commit into mine.

It is indeed great to see how you work together.

How we work together, you are part of the team too :-)

It is indeed great to see how you work together.

How we work together, you are part of the team too :-)

Yes, and I am proud to be here, but I missed this yesterday as I was with customers and when I came home, I saw that you have done the whole work - so I was meaning this exact effort by saying "you".

Compared to what I see in companies ... it is a dream to watch how you handle it. It might be that it is the size of the team but also the attitude and the qualities of all of us obviously.

So this PR is ready for merge?

Everything seems to me ready for merge.

So this PR is ready for merge?

It is definitely ready for me. I was waiting to see if you had further comments on the new code I posted earlier.
If not, then we can merge. Please advice @blu.256

No objections, go ahead :-)

KSnapshots: fixed remaining issues related to PR #50. #52

Reviewers