Skip to content

remove old style connects#237

Merged
atsju merged 7 commits intogithubdoe:masterfrom
atsju:JST/clazyConnect2
Aug 21, 2025
Merged

remove old style connects#237
atsju merged 7 commits intogithubdoe:masterfrom
atsju:JST/clazyConnect2

Conversation

@atsju
Copy link
Collaborator

@atsju atsju commented Aug 20, 2025

close #229

I place this here to show you. Feel free to approve or reject.
I still need to replace one last slot that will require a small rework but is working as is. Hence the "draft" pull request.

Some have been automatically replaced, some manually. You can look at changes commit by commit if you want to dig.
I have found no real issue with original code (except one fixed in #231). This will just make it easier to maintain.

@gr5
Copy link
Collaborator

gr5 commented Aug 20, 2025

I assume you had to do the "increase()" one manually. How many others? I'd like to look over the ones you did manually carefully but don't want to look over all of them as there are so many but a typo in any of them could cause a bug, right? A signal could be lost.

@atsju
Copy link
Collaborator Author

atsju commented Aug 20, 2025

everything that is NOT in first commit e82c3ac has been coded manually. If you want to check everything I recommend you do commit by commit. I tried to make them as reviewable as possible one by one.

a typo in any of them could cause a bug, right?

Yes but not anything near the way it was before. Now all connect are compile time checked. This means there cannot be "no connection" as before with a warning in the logs.
Only thing that could happend is that I connected to a wrong signal (same name from different class ?).
That why I was willing to do the move.

If you think the risk is too high with legacy code I can live with it. Personally I prefer live with an unlikely but possible break in the futur alpha and have a the compile time checks.

@gr5
Copy link
Collaborator

gr5 commented Aug 20, 2025

Ah! I don't usually look at the commits tab above. I usually look at the files tab above. That's a good way for me to break up this PR into smaller chunks.

@atsju
Copy link
Collaborator Author

atsju commented Aug 20, 2025

Ah! I don't usually look at the commits tab above. I usually look at the files tab above. That's a good way for me to break up this PR into smaller chunks.

that's one way of doing it. You can also select individual commits or ranges with this method in file tab
image

fitToWindowAct->setEnabled(false);
fitToWindowAct->setCheckable(true);
fitToWindowAct->setShortcut(tr("Ctrl+f"));
connect(fitToWindowAct, SIGNAL(triggered()), this, SLOT(fitToWindow()));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow! This function: fitToWindow() doesn't appear to exist and it didn't exist in the very first version of DFTF checked into git back in 2016.

@atsju
Copy link
Collaborator Author

atsju commented Aug 20, 2025

It's draft. I still have one small change to add (but now you know how to review single commit).

@atsju atsju marked this pull request as ready for review August 20, 2025 16:38
@atsju
Copy link
Collaborator Author

atsju commented Aug 20, 2025

Last change done. Ready for review

@atsju atsju requested review from githubdoe and gr5 August 20, 2025 16:39
@atsju atsju merged commit ddd3d84 into githubdoe:master Aug 21, 2025
14 checks passed
@atsju atsju deleted the JST/clazyConnect2 branch August 21, 2025 05:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

QT old style connects

3 participants

Comments