Conversation
know when to invert the wavefront. This is version in my video explaining my ideas. I need to fix a few things still. work in progress.
Removed the code where I had "invert" button change the autoinvert status Changed wording a little in the auto invert dialog Spaced things out in the auto invert dialog for people with >100% scaling Tested at 225% scaling Fixed project files to include the new autoinvert dialog
Disable the "conic" button if cc=0. If user changes cc to zero within the mirror config and clicks "ok" and if it's in conic mode it switches to "not set" mode. I realized "Your wavefront may be inverted" is inappropriate when running the dialog from the mirror config screen so that text changes depending how you got there. fixed a compiler warning, fixed other minor stuff.
|
Does not compile on linux and qt5 so I'll look into that. |
|
Compiles just fine now. Ready for review. Again - I had edited 2 of the project files (".pro"), but missed the 3rd one (newest one). |
atsju
left a comment
atsju
left a comment
There was a problem hiding this comment.
Code looks good. I have not tested to see how it looks (and do not plan to do it).
I have noticed some minor things to improve and personally I would remove the singleton design pattern.
| explicit autoInvertDlg(QWidget *parent = nullptr); | ||
| ~autoInvertDlg(); | ||
| static autoInvertDlg *get_Instance(); | ||
| void setMainLabel(const QString & str); | ||
| void enableConic(bool b); |
There was a problem hiding this comment.
if implementing singleton: need to delete copy constructor and operator
autoInvertDlg(autoInvertDlg const&) = delete;
autoInvertDlg& operator=(autoInvertDlg const&) = delete;
autoinvertdlg.cpp
Outdated
| autoInvertDlg *autoInvertDlg::get_Instance(){ | ||
| static autoInvertDlg m_Instance{}; | ||
| return &m_Instance; | ||
| } |
There was a problem hiding this comment.
I would have preferred not to use singleton design pattern but this implementation is good.
EDIT: After reviewing all caode and trying to understand the architecture, I see 2 singletons getting each other while autoinvertdlg is only here to manage SurfaceManager->m_inverseMode. Who own who and make what is unclear.
My recommendation:
- do not use singleton (
autoInvertDlgdoes not need to live or be kept when the dialog is closed. Only reason i see isupdateAutoInvertStatusbut if you call it at construction and when needed, then I think you are all set). - You might pass a reference to
sm->m_inverseModeto the constructor of the dialog and it won't need to getsurfaceManagerinstance.
I thing this is a good illustration of why to avoid singleton. It makes the architecture more of a spaghetti code. Easy to write but more difficult to maintain and to keep different things separate.
There was a problem hiding this comment.
I did your first recommendation (next commit; soon) as you are right: there is absolutely no need to make a singleton. The dialog is now created when needed and destroyed immediately after. Code is cleaner, simpler, smaller.
I didn't do the second suggestion. surface manager is already a singleton - might as well just use it the same way it is used everywhere else. No need to pass in a pointer to the surface manager class.
Co-authored-by: Julien Staub <atsju2@yahoo.fr>
Co-authored-by: Julien Staub <atsju2@yahoo.fr>
Co-authored-by: Julien Staub <atsju2@yahoo.fr>
Code is a little simpler, smaller.
atsju
left a comment
atsju
left a comment
There was a problem hiding this comment.
Looks good. Thank you for the modifications
mirrordlg.h
Outdated
| QTimer spacingChangeTimer; | ||
| void saveJson(const QString &fileName); | ||
| void enableAnnular(bool enable); | ||
| autoInvertDlg *m_autoInvertDlg; |
There was a problem hiding this comment.
| autoInvertDlg *m_autoInvertDlg; |
|
@atsju Please approve if... you approve. 😄 Dale has already approved the gist. We had some back and forth emails a few weeks ago. I don't feel he needs to do a review. I think this can be merged if you are okay with it. |
|
I didn't "approve" because of the last unused variable but was fine with general code. |
|
There's an unused variable? I missed that but it doesn't sound critical since you've already approved. |
|
You probably missed my comment (just under the review, it's masked now, I unmasked it) |
2 participants
This adds a feature requested by Yordan and fixes issue #167
Dale and I discussed and settled on the GUI implementation in this PR.
I apologize for 2 things:
Mixed ways of doing braces. Some were autogenerated by QT (like the "on_" functions). Some were copy paste. Some I wrote and I'm very comfortable with either method so I usually just scroll up a bit and copy whatever already exists. A result of all that is a messy mixture of both styles.
"on_" functions: QT automatically generated these slots and signals. This was done 3 months ago -- before Julien fixed all the slots and signals. I'm not sure how to do it the right way or if QT did it the right way or not. I can say there were no compiler warnings but I think the slot/signals warnings are not from the compiler but from clang and I don't know how to run that other than through the "actions" when I do a PR.