Skip to content

Comments

Several enhancements from dalework. See details in PR#262

Merged
gr5 merged 4 commits intomasterfrom
JST/daleworkReview
Nov 9, 2025
Merged

Several enhancements from dalework. See details in PR#262
gr5 merged 4 commits intomasterfrom
JST/daleworkReview

Conversation

@atsju
Copy link
Collaborator

@atsju atsju commented Nov 8, 2025

using this PR in favor of #261

  • Igramarea.cpp
    • Added e hot key command to enable outline edge zoom.
  • MainWindow.cpp
    • Added igram downloader using the “u” hot key will downoad image from skysolve camera at it’s URL of 192.168.50.5:5000. (specialized and not configurable). Could be set into preferences but still only appropriate for getting images from Skysolve.
    • Added ‘i’ hot key igram import igram from latest file in dir x where x is set in preferences.
    • Added “s” hot key to save current wave front.
  • main.cpp
    • Added DPI scaling command but it did not seem to help.
  • Profileplot.cpp
    • Enabled context menu and added change x axis to percent, inches, or mm(default)
    • Moved percent correction command into this menu.
  • Simulationview.cpp
    • Corrected Gaussian Blur calculation.
    • Changed top pushbuttons for make scan film and make to checkboxes. Because then their text is not tiny like it was for when they were buttons on high DPI screens.
    • Reduced file name path to last dir on the top line.
  • Added startest movie function.

githubdoe and others added 4 commits November 1, 2025 18:22
	Added e hot key command to enable outline edge zoom.

MainWindow.cpp
	Added igram downloader using the “u” hot key will downoad image from skysolve camera at it’s URL of 192.168.50.5:5000.  (specialized and not configurable).  Could be set into preferences  but still only appropriate for getting images from Skysolve.
	Added ‘i’ hot key igram import igram from latest file in dir x where x is set in preferences.
	Added “s” hot key to save current wave front.

main.cpp
	Added DPI scaling command but it did not seem to help.

Profileplot.cpp
	Enabled context menu and added change x axis to percent, inches, or mm(default)
	Moved percent correction command into this menu.

Simulationview.cpp
	Corrected Gaussian Blur calculation.
	Changed top pushbuttons for make scan film and make to checkboxes.  Because then their 	text is not tiny like it was for when they were buttons on high DPI screens.
	Reduced file name path to last dir on the top line.

Added startest movie function.
//});
loop.exec();
QByteArray buffer = reply->readAll();
QImage b(buffer);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Suggested change
QImage b(buffer);

Fix warning

@atsju
Copy link
Collaborator Author

atsju commented Nov 8, 2025

@atsju atsju marked this pull request as ready for review November 8, 2025 16:26
@atsju atsju requested review from githubdoe and gr5 November 8, 2025 16:26
@atsju atsju mentioned this pull request Nov 8, 2025
// hue varies from 0 to 179, see cvtColor

int hbins = 1000;
int histSize[] = {hbins};
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's scary to me that I fixed this back in July but this fix got lost in dale's merge. Did Dale have a merge conflict and chose the wrong version?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No this is probably not a bad fix from last merge because I checked against my own merge. The problem is older. Dale does not pull master very often and works long time locally without commit. And if he fixes some thing manually wrong once it stays.

I know it's not easy. The more often you merge master, the easier it is.
I would recommend deleting branch dalework. Create a new branch from most up to date master when you plan to develop something, commit and pull request when you are finished. Don't keep a branch more than 2 weeks.

Golden rule is : start developing from most up to date master.

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes, good golden rule.

However I think I misunderstood - these are dales changes, not your fixes, and also my changes so somehow it got changed back and then fixed again. Maybe during a merge he did.

I had been thinking Dale was missing this fix from july and he deleted it and you fixed it but it looks like dale got this change in eventually somehow on his own before he did the PR. I think.

connect(Show16, SIGNAL(clicked()), this, SLOT(show16()));
connect(OneOnly, SIGNAL(clicked()), this, SLOT(showOne()));
connect(ShowAll, SIGNAL(clicked()), this, SLOT(showAll()));
l1->addStretch();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I approve this.

@githubdoe
Copy link
Owner

githubdoe commented Nov 8, 2025 via email

@gr5
Copy link
Collaborator

gr5 commented Nov 8, 2025

So I looked at all these code changes and I think they are fine. Next is to download and test it

@atsju
Copy link
Collaborator Author

atsju commented Nov 9, 2025

Dela I think there might eventually be some misconception about dalework branch. Or I did miss something.

I want dalework to always exist. It is my backup if I ever lose my machine

A branch is only a sticker to a commit. Usually to tip of a branch.
Now we are merging everything from dalework into master. Dalework will no longer be a tip. This means deleting the sticker dalework will not delete any commit because they are in master branch history. You still have your backup.

Think about it this way: master is your backup when you have no local changes compared to official version. Dalework is a short term backup when you are working on a feature.

What we do usually and I would recommend:
Once we merge this you might delete dalework (or not). Next time you actively develop something you create a branch starting from tip of master. It can be named dalework or different . You try to not have 5 changes over 6 month but instead you do a pull request after you developed something meaningful.
Ideally you do one PR by feature but I do not request that from you.
This makes reviews easier because 5 short changes are simpler than one big. You have less merge conflicts. You remember what/why you just developed during the he review process.

After this PR is merged I will give you a command to set dalework to most up to date code. As you have no local changes right now this will ensure worst case scenario we get no conflict older than today.

@githubdoe
Copy link
Owner

I want a version of DFTFringe like I have now that will compile under Qt5. If I ever have to go back I need it easy to find and not buried in x number of revisions back. Perhaps because I have merged master into dalework I really don't have that anymore. I don't know. But I think I can still compile Dale work. For instance right now I don't want to use the most recent version because I don't like how the wave front list is too large. So I use my version instead.

@atsju
Copy link
Collaborator Author

atsju commented Nov 9, 2025

I want a version of DFTFringe like I have now that will compile under Qt5. If I ever have to go back I need it easy to find and not buried in x number of revisions back. Perhaps because I have merged master into dalework I really don't have that anymore. I don't know. But I think I can still compile Dale work. For instance right now I don't want to use the most recent version because I don't like how the wave front list is too large. So I use my version instead.

I made every effort so that the master branch still builds with QT5 and you have your own dftfringe_dale.pro in master.
As I said your backup is master. After today (and each time dalework is merged into master) there is no difference between dalework and master.
Dalework==master unless you did some fancy stuff.
So I mantain: next time you start developping, create a fresh dalework or feature branch on tip of master. Avoid merging master into the old dalework as it creates conflicts.

If you ever have local changes that you do not want to merge into master than yes keeping dalework would be correct approach.
But if you do so, you cannot easily open a PR to merge parts of dalework to master. You will need the private dalework branch + a feature branch.

I'm happy to explain more if needed. I really don't want to breack your workflow and want to keep things simple from your point of view. I just feel like the little misconception about what is in master VS dalework is adding complexity (merge conflicts) to everybody including you.

@githubdoe
Copy link
Owner

Ok

@gr5
Copy link
Collaborator

gr5 commented Nov 9, 2025

For instance right now I don't want to use the most recent version because I don't like how the wave front list is too large. So I use my version instead.

Shouldn't we fix that then?

I'm not sure but it sounds like QT6 works better with the difference between your display and most others. Maybe you and I should try installing and compiling with QT6 so we can all be on the same codebase.

@gr5
Copy link
Collaborator

gr5 commented Nov 9, 2025

My highest goal today regarding DFTF is to try this PR built by QT6 and to try building it on QT5. Make sure the QT6 version passes a list of tests I wrote down (basically the things you changed) and make sure QT5 seems to work okay as well (less thorough test).

This QA shouldn't take long as I save most of my testing energy for release versions.

Copy link
Collaborator

@gr5 gr5 left a comment

Choose a reason for hiding this comment

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

Tested the one built on github with qt6 and built it myself with qt5.

I could find no obvious differences in the how the gui looked regardless if built with qt5 or qt6.

The qt6 version I did a lot of testing - particularly all of Dale's new features.

Everything looks good. And yesterday I reviewed all the code changes.

I approve this. Time to pull.

@gr5 gr5 merged commit 59e0518 into master Nov 9, 2025
14 checks passed
@gr5
Copy link
Collaborator

gr5 commented Nov 9, 2025

Okay so this is merged into master. Now we have to figure out how to get Dale to use it before he makes more edits. Dale this builds in qt5 no problem but you may find some gui elements are too big or too small. If so maybe it's time to switch to qt6. The qt6 version is here among other places:

https://github.com/githubdoe/DFTFringe/actions/runs/19194845643

Click the black bold text at the bottom. Or here is a direct link to the download:

https://github.com/githubdoe/DFTFringe/actions/runs/19194845643/artifacts/4508196804

@gr5
Copy link
Collaborator

gr5 commented Nov 9, 2025

Dale, FYI - I always check that things work fine in qt creator version 5 because I know that is what you use and I want you to be able to always be happy with the latest code. And I also use QT5. I like QT creator and I've been happy with QT5 but if you move to QT6, then I'll move as well.

@atsju atsju deleted the JST/daleworkReview branch November 9, 2025 21:00
@githubdoe
Copy link
Owner

I tried the install. Maybe the dark would not be too bad if the profile had an option to change the background color. There probably a few other windows or graphs that need a similar option. Qwtplot does have things to make that happen.

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.

3 participants