Some code improvements #211

atsju · 2025-07-21T06:28:48Z

With these changes there are few enough warnings to re-enable Clazy problem matcher and display warnings in the pull request. close #210

I have found several real problems with this tool so I think it's worse it to fix the warnings.

AI is also of great help to make code changes. I'm not fluent enough in C++ to write high quality code but it helps me learn and I can check all proposals made by AI.

I keep the PR draft for now as I need to test my changes. Mainly about ffmpeg video generation.

…I freeze in movie maker

atsju · 2025-07-21T16:16:52Z

ffmpeg works fine. Still lots of memory leaks and probably problems in case of abort but this is for different PR

gr5 · 2025-07-22T23:55:36Z

Well it builds just fine in qtcreator and very basic testing works. I'll look at the code changes more carefully and possibly test features related to the code changes.

githubdoe · 2025-07-23T02:57:14Z

Does the Best fit conic update correctly? From the code I looked at I think it would be broken.

gr5 · 2025-07-23T14:00:39Z

Does the Best fit conic update correctly? From the code I looked at I think it would be broken.

It works for me. Is there something in particular I should try? I just processed a single igram and the best fit conic value displayed is correct.

gr5 · 2025-07-23T15:34:05Z

DFTArea.cpp line 143 is getting a warning message that the emit "probably has no effect". However it works fine because a few lines of code above, is "tools->connectTo(this) which connects dfttools slot to this signal.

Is there a way to disable this warning for this line of code only?
(I tried to make this an inline comment but the review process wouldn't let me as there was no code change here)

gr5

I looked through these pretty carefully and I'm quite confident you didn't introduce any bugs. Except for the mmpeg stuff. That I didn't go through quite as carefully.

gr5 · 2025-07-23T13:42:00Z

astigpolargraph.cpp

    int closeNdx = -1;
    int ndx = 0;
-    for (auto sample : m_list) {
+    for (const auto& sample : m_list) {


Did you fix one warning only to create a different one? I don't really understand the warning.

correct. I fixed an actual warning where the container was detaching.
And now we have a warning about C++11 not perfectly managing it. But this will be fixed with future compiler if I remember well. So I consider this warning less harmful and more future ready than previous one.

atsju · 2025-07-23T16:43:09Z

Hi @gr5 and @githubdoe thank you for the careful review and testing. I really appreciate it.

DFTArea.cpp line 143 is getting a warning message that the emit "probably has no effect". However it works fine because a few lines of code above, is "tools->connectTo(this) which connects dfttools slot to this signal.

Is there a way to disable this warning for this line of code only? (I tried to make this an inline comment but the review process wouldn't let me as there was no code change here)

I had same conclusion as you and already though about it.
Yes I think it's possible to mask single warnings. However here I think we could just convert the slot to a function and it would be clean and maintainable. There is no hard reason to use the slot mechanism here. I did not fix it here to not introduce too much changes.

About mmpeg stuf, I actually tested (and discovered) it. It works at least as good as before with maybe a bit more error checking where you can cancel without hanging.

Unless you have more remarks I think with should merge this, the other two (or not if someone is against the changes) and then make a changelog and release 7.4.0 (new autoinvert feature ?) or 7.3.5

gr5 · 2025-07-23T19:53:35Z

I think you should merge this.

I decided not to add any autoinvert features at this time. There didn't seem to be a strong interest. Yordan asked for the feature but didn't comment on my suggestions.

I do still plan to change the wording of the current popup dialog. Slightly. I just want to add a little more explanation about what it is doing and why.

Sure we can do a 7.3.5 release. The one "histogram related" bug is probably worth releasing just for that alone. Do you want me to write up the changes in revisionhistory?

githubdoe · 2025-07-24T02:23:33Z

Nobody has addressed my comments.

githubdoe · 2025-07-24T02:38:25Z

I liked most of your invert modifications. I have recently been comparing wave fronts using the subtract. I not sure the code processes that correctly if it thinks the sign of the SA is not correct. I think in this case just like in the do not use the null case it should not invert. But I think it is silently inverting the result when it things all wave front signs should match the desired conic sign.

I just looked at the code and as near as I can tell it does nothing special after the subtract. Meaning that it is going to invert the wave front quietly if it has been told to for regular processing. Please someone see what they think about this.

I was looking forward to seeing your invert dialog pop up where I could then know if it was going to try and invert. Mine of course will not pop up if already told to invert which it has been in my current session.

atsju · 2025-07-24T05:38:14Z

hi @githubdoe sorry if your comment has been addressed but this pull request does not change tool behavior at all so I'm not sure what your are talking about. Could you point out a commit, issue, pull request or a file+line ?

Is it about this pull request comment #208 (comment) ?
Or about this issue #199 ?
Best to answer at right place directly. I get notification of all your messages and I think so does @gr5 .
@gr5 maybe you understand Dale's comment better. I'm more focused on the code but do not understand all the invert and SA features.

Yes we must probably do a patch release soon mainly because of the fixed crash. I should be able to changelog within 48h. What about the 2 other PR ?
then we can do an other release if auto-invert needs changes.

I merge now. @githubdoe you can still answer here or anywhere. We will get notified.

atsju · 2025-07-24T16:50:31Z

@githubdoe also you can totally comment in closed pull requests. We will see it. Button is only "grayed out" as long as you type nothing.

githubdoe · 2025-07-21T17:53:51Z

batchigramwizard.cpp

What I remember is that this emit is important to make the GUI work correctly. During Batch processing the signals need to be process differently. This enables that. So it should not be changed. Why is it removed?

The compiler warning was correct; this emit has no effect.
Indeed., the connect was done after emit. But you you call correct function after so it's working as intended. That makes me think the connect and signal can be removed completely as unused. Thank you for heads up.

batchWiz = new batchIgramWizard(m_igramsToProcess, this,Qt::Window); connect(batchWiz,SIGNAL(swapBathConnections(bool)),this, SLOT(batchConnections(bool))); batchConnections(true);

githubdoe · 2025-07-21T17:57:33Z

dftarea.cpp

Put some debug in the signal processor for that emit and see if it does get processed to confirm that assertion.

I did not change the code here so I did not introduce new problems. Only warnings that were not processed before.

You are right it needs to be checked and I will do it some day.

githubdoe · 2025-07-21T18:01:34Z

edgeplot.cpp

Just another way of coding what was already done. This is not necessary. It does not improve readability in my opinion.

It avoids unnecessary object copies because of detaching temporary.
This whole pull request is about fixing warning you never saw before (from clazy tool). I did not do it for readability.

My understanding could be wrong but he creates axesH and axesV and then they are destroyed (deleted) at the end of the function. The older code also calls axes() which returns an object but it never gets deleted. maybe?

Makes sense. I checked the internet and it does not cause memory leak. Temporary get freed correctly. It's just you copy the temporary thus double the object.
Inefficient but not dangerous.

githubdoe · 2025-07-21T18:10:25Z

mainwindow.cpp

Explain why you think an additional variable (selectedFiles) was needed.

Explain what the modifications to the code that watches the FFMPEG output does. The previous code does occasionally hang and so I know it's not exactly correct. Forgive me if you have explained the changes somewhere that I have not seen.

dialog.selectedFiles().first() will detach temporary
(https://github.com/KDE/clazy/blob/master/docs/checks/README-detaching-temporary.md)

Now that I read it again and again, I think the new code does exactly that but explicitly. So there is no warning but it's not really performance improved.

I should probably have changed to loadFile(dialog.selectedFiles().constFirst()); if it works it's better. I will change in a different PR if it works

Let me explain FFMPEG changes in a different comment

githubdoe · 2025-07-21T18:13:53Z

myplotpicker.h

Explain why changing the name was necessary. I don't think it was.

because selected was clashing with a QWT function we inherited in this class. Nasty warning that could mask important issue. It probably did work fine but now we are 100% sure.

githubdoe · 2025-07-21T18:18:56Z

psi_dlg.cpp

Explain why an additional variable (files) was needed. I don't think one is.

same as above. https://github.com/githubdoe/DFTFringe/pull/211/files#r2229198458

in fact here it's somewhat different. you see files was already created at line 70. I just reused it

githubdoe · 2025-07-21T18:21:22Z

unwraperrorsview.cpp

While technically correct a char and an uchar have the same size I think. Thus each is equivalent in this context.

Yes they are 100% equivalent. But one is not overflowing and avoids warning. Thus I used CV's expected signness

atsju · 2025-07-24T17:42:33Z

@githubdoe Comments successfully shared. I will answer them

atsju · 2025-07-24T18:08:26Z

mainwindow.cpp

+                // ensure we kill ffmpeg if the dialog is closed
+                connect(dialog, &QDialog::finished, dialog, [=](int) {
+                    if (proc->state() == QProcess::Running) {
+                        proc->kill();
+                        proc->waitForFinished();
+                        qDebug() << "ffmpeg Process killed by user";
+                    }
+                });
+


this connects a dialog close (X) to QProcess proc. In some cases I have hang whole application by closing dialog at wrong time. Closing was not handled

atsju · 2025-07-24T18:12:10Z

mainwindow.cpp

-                while (!proc->waitForFinished(200)){
-                    QString q = proc->readAll();
-                    if (q != "")
-                        textEdit->append(q);
-                    QApplication::processEvents();
-                }
+                connect(proc, &QProcess::readyRead, textEdit, [proc, textEdit]() {
+                    QString output = proc->readAll();
+                    if (!output.isEmpty())
+                        textEdit->append(output);
+                });
+
+                QEventLoop loop;
+                QObject::connect(proc, SIGNAL(finished(int, QProcess::ExitStatus)), &loop, SLOT(quit()));
+                loop.exec();


other changes are more self explanatory so I will focus on this one. Let me know if you have more questions on the other changes and I will answer happily.

Original code was doing proc->readAll(); in a busy loop. Instead I connected QProcess::readyRead to what the loop was doing.

Then I created a QEventLoop loop who does the waiting instead of original waitForFinished. All the close and finish signals are connected correctly to kill everything properly.

githubdoe · 2025-07-24T18:34:58Z

Thank you. You might have guessed I never really understood how to capture output from a process. I always found that they could hang. I will study your example and try to fix similar issues in some of my personal apps I use.

atsju · 2025-07-24T18:40:26Z

Thank you. You might have guessed I never really understood how to capture output from a process. I always found that they could hang. I will study your example and try to fix similar issues in some of my personal apps I use.

Also this code has been proposed and reviewed by chatGPT. Of course I studied it to understand and check it was what I expected. but I find it an excellent learning tool. It has been fed with documentation and "best" examples so it will often output best ways of doing things. I'm absolutely not the QT or C++ expert your think. I just like learning

improve after #211 comments

atsju added 11 commits July 20, 2025 19:39

-Wclazy-qstring-arg

12f21ad

-Wclazy-unused-non-trivial-variable, fix memleak in movie maker, no U…

57b012e

…I freeze in movie maker

-Wclazy-overloaded-signal

6b0be51

re-enable clazy problem matcher as there are few enough warnings

f5e6f35

more safety and memory leaks

987553e

clazy-range-loop-reference

8b2c890

clazy-unused-non-trivial-variable

04b0b77

clazy-container-anti-pattern

5ca7c49

clazy-detaching-temporary

cc79f50

fix QT5 build

b418b42

some less warnings

38a805c

atsju marked this pull request as draft July 21, 2025 06:28

atsju marked this pull request as ready for review July 21, 2025 16:16

atsju requested review from githubdoe and gr5 July 21, 2025 16:17

Wclazy-incorrect-emit

1b6b16a

gr5 approved these changes Jul 23, 2025

View reviewed changes

atsju merged commit 86d5a39 into githubdoe:master Jul 24, 2025
8 checks passed

atsju mentioned this pull request Jul 24, 2025

Improve DFTFringe speed on zernike related computation #206

Merged

atsju deleted the JST/clazyOnly branch July 24, 2025 16:50

githubdoe reviewed Jul 24, 2025

View reviewed changes

atsju commented Jul 24, 2025

View reviewed changes

atsju added a commit to atsju/DFTFringe that referenced this pull request Jul 24, 2025

improve after githubdoe#211 comments

9584f3b

atsju mentioned this pull request Jul 24, 2025

improve after #211 comments #212

Merged

atsju added a commit that referenced this pull request Jul 30, 2025

Merge pull request #212 from atsju/JST/improve

d194cf6

improve after #211 comments

This was referenced Aug 15, 2025

QT old style connects #229

Closed

fix RMS plot wft selection. broken 7.4.0 #231

Merged

end support of saving .ini file format for mirror configs #235

Merged

Some code improvements #211

Some code improvements #211

Uh oh!

Conversation

atsju commented Jul 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

atsju commented Jul 21, 2025

Uh oh!

gr5 commented Jul 22, 2025

Uh oh!

githubdoe commented Jul 23, 2025

Uh oh!

gr5 commented Jul 23, 2025

Uh oh!

gr5 commented Jul 23, 2025

Uh oh!

gr5 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atsju commented Jul 23, 2025

Uh oh!

gr5 commented Jul 23, 2025

Uh oh!

githubdoe commented Jul 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

githubdoe commented Jul 24, 2025

Uh oh!

atsju commented Jul 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

atsju commented Jul 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atsju commented Jul 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

githubdoe commented Jul 24, 2025

atsju commented Jul 21, 2025 •

edited

Loading

githubdoe commented Jul 24, 2025 •

edited

Loading

atsju commented Jul 24, 2025 •

edited

Loading

atsju commented Jul 24, 2025 •

edited

Loading