[not ready for review] fix mirror settings modified in registry before user press accept button

[atsju]

fix #121
mentionning #224 #234

I'm really sorry this one is quite long. It was difficult to split and even now some ellipse related things are temporarly brocken.

---

short version

The mirror dialog's settings were exposed as public members, allowing user interactions to corrupt the persistent copy even on Cancel. This PR implements a draft pattern + settings facade to fix it:

• m_current (persistent copy) is now protected; external code reads it via currentSettings() (read-only)
• m_draft (working copy) handles all user edits; discarded on Cancel, committed on OK
• New SettingsFacade provides single atomic save/load point; eliminates duplicate QSettings calls
• All 30+ UI handlers refactored to use m_draft; removed 13+ public data members

---

Long version explanation

Problem Fixed

The singleton mirror dialog's settings were exposed as public members scattered across the class, violating encapsulation and creating a critical flaw: the Cancel button could not reliably discard edits because the persistent settings copy was unprotected and could be corrupted by user interactions.

Solution Implemented: Draft Pattern + Settings Facade

1. Architectural Changes

New Infrastructure:

• SettingsFacade (new singleton): Centralized access point for all application settings

  • Enforces friend-only access to store implementations
  • Single atomic save/load point for all settings persistence

• MirrorSettingsStore & ApplicationSettingsStore (new): Private store classes with friend-only access to mirrordlg and facade

  • Generated via X-macros for compile-time field validation
  • QSettings keys defined once in settingsstores_fields.inc

• settingsstores_fields.inc (new): Single source of truth for all settings schema

  • 18 mirror config fields (diameter, roc, cc, flipH, etc.)
  • 3 application fields (projectPath, mirrorConfigFile, lastPath)
  • Eliminates magic strings throughout codebase

Dual-Copy Pattern in mirrorDlg:

m_current  → persistent copy (last-saved state, read-only via currentSettings() to external code)
m_draft    → working copy for dialog edits (temporary, discarded on Cancel)

2. Transactional Edit Semantics

Before Dialog Shown:

• Constructor calls loadDraftFromSettings() for safety (in case dialog used without showEvent())
• Both m_current and m_draft are initialized from persistent storage

While User Edits:

• All 30+ UI event handlers modify m_draft (working copy)
• External code still reads m_current via currentSettings() (persistent copy)
• User actions are completely isolated from other code

On Dialog Close:

• OK/Accept: Commits m_draft → m_current → QSettings atomically via facade

  • m_current = m_draft (merge working to persistent)
  • SettingsFacade::instance().saveMirrorSettings(m_current) (atomic save)

• Cancel: Discards m_draft entirely

  • Next time dialog shown, loadDraftFromSettings() reloads from persistent storage
  • User changes are completely reverted

showEvent() Override:

• Always calls loadDraftFromSettings() before dialog becomes visible
• Syncs UI with reloaded draft values
• Guarantees Cancel reverts to last-saved state (fixes issue Mirror dialog needs to revert when cancel pressed #121)

3. Encapsulation Improvements

Removed Public Data Members:

• Old: diameter, roc, obs, cc, flipH, lambda, doNull, fringeSpacing, aperatureReduction, m_useAnnular, m_outlineShape, m_verticalAxis, etc. (13+ exposed members)
• New: Single private MirrorSettings m_draft struct

Replaced With Managed Access:

• External code: mirrorDlg::get_Instance()->currentSettings() returns const MirrorSettings& (read-only)
• Settings access is now type-safe, compile-checked, and change-tracked

Added Inline Getters for Computed Values:

double getFNumber() const { return FNumber; }        // f-number (computed on-the-fly)
double getZ8() const { return z8; }                  // Z8 Zernike coefficient
double getMinorAxis() const;                         // Ellipse minor axis
bool isEllipse() const;                              // Outline shape query
bool shouldFlipH() const;                            // Horizontal flip query
const MirrorSettings& currentSettings() const;       // Full settings struct (persistent copy)

Compile-Time Access Control:

class MirrorSettingsStore {
private:
    friend class SettingsFacade;      // Only facade can construct and call save()
    friend class mirrorDlg;           // Only dialog can commit settings
    // ...
};

→ No way for external code to call save() directly; enforced at compile-time by linker

4. Settings Persistence Flow

Old Design:

External Code → QSettings → mirrorDlg scattered members → QSettings
(Magic strings, duplicated keys, type-unsafe, vulnerable to corruption)

New Design:

External Code → mirrorDlg::currentSettings() (m_current) → SettingsFacade
                  ↓ (on dialog accept)
             SettingsFacade → saveMirrorSettings(m_current) → QSettings
                  ↓ (on dialog show)
             SettingsFacade → mirrorStore().load() → m_current + m_draft

Benefits

Benefit	Impact
Data Integrity	Singleton's persistent copy (m_current) is now protected from Cancel operations
Backward Compatible	No QSettings key changes—legacy config files load correctly
Type Safety	Struct fields replace magic strings; caught at compile-time, not runtime
Testability	Facade enables easy mock/override for testing without filesystem I/O
Single Source of Truth	External code reads currentSettings() (persistent), dialog edits m_draft (working)
No Duplication	Settings defined once in X-macros; auto-synced across load/save code
Error Prevention	Removed 13+ public data members; accidental modifications now impossible

Changed Files

Core Architecture

• settingsfacade.h / settingsfacade.cpp — New facade enforcing friend-based access control
• settingsstores.h / settingsstores.cpp — New stores with generated struct definitions
• settingsstores_fields.inc — X-macro field schema (shared with stores and UI)

Mirror Dialog Refactoring

• mirrordlg.h — Replaced 13+ public members with dual-copy pattern + getters
• mirrordlg.cpp — Refactored all 30+ UI handlers to use m_draft, added loadDraftFromSettings() and showEvent(), simplified on_buttonBox_accepted()

Build System

• DFTFringe.pro, DFTFringe_QT5.pro, DFTFringe_Dale.pro — Updated to include new settings files

---

Future Work (Next PRs)

Critical TODOs from mirrordlg.h

1. Persist Unit Preference (mm vs. inches)

// TODO: actually mm is not saved in settings. should probably be saved 
// as it's a user preference
bool mm;  // Unit display flag: true = mm, false = other units

Current Behavior: Unit preference (mm) is transient—resets to default on restart.

Solution:

• Add bool unitsMM field to settingsstores_fields.inc
• Persist in showEvent() and on_buttonBox_accepted()
• Remove hardcoded mm(true) initialization

Benefit: Remembers user's preferred unit system across sessions.

---

2. Clarify & Consolidate Outline Shape API

// TODO: to be fixed with #358. 
// Saving shape shall be asked as it is for ROC, lambda and diameter 
// and be saved using adoptWavefrontSettings
void setOutlineShape(outlineShape shape);
void setMinorAxis(double val);

Current Problem:

• Two separate methods for modifying outline shape
• No guarantee that shape changes persist to m_current and QSettings
• Wavefront loading sometimes calls setMinorAxis(), sometimes direct member access

Solution:

• Rename adoptWavefrontSettings() to accept outline shape:

  void adoptWavefrontSettings(double diameter, double roc, double lambda, 
                               outlineShape shape, double minorAxis);

• Remove setOutlineShape() separate methods
• Single call-point ensures all shape changes are atomic and persisted
• Wavefront loader makes ONE decision and commits all at once

Benefit: Eliminates inconsistent state where shape is changed but not saved.

---

3. Clean allispe outline helper

// TODO: This is still not 100% clean
// One call from loading file should be integrated to adoptWavefrontSettings
// Other calls are outline helpers. Need to be clarified.
void setMinorAxis(double val);

Current Confusion:

• setMinorAxis is called from outlining window and can modify mirror setting at each outline.

Solution:

• To be investigated. Might be OK as is. Renaming to clarify could be enough

Benefit: Eliminates API confusion; clear roles for each method.

---

Technical Notes

Why X-Macros?

Reduces boilerplate and eliminates source-of-truth duplication. A single field definition in settingsstores_fields.inc automatically generates:

• Struct member declaration
• QSettings load code
• QSettings save code
• Type validation

Adding a new mirror setting requires exactly one edit location.

Why Compile-Time Friend Enforcement?

Prevents accidental calls to save() from unintended code locations. The linker will reject any save() call outside the allowed friend scope—impossible to miss in code review or CI.

Why Always Reload on showEvent()?

Ensures the dialog never operates on stale data if another dialog modified settings between invocations. Also handles programmatic dialog reuse without explicit reset calls.

---

References

• Original Issue Mirror dialog needs to revert when cancel pressed #121: "Cancel button doesn't revert mirror config"
• Issue Importing wavefront with different config: "yes to all" doesn't work #358: "Clarify outline shape persistence"

[github-actions]

🚀 New build available for commit ef52e33
Download installer here

[github-actions]

🚀 New build available for commit 1cfb677
Download installer here

[github-actions]

🚀 New build available for commit 8360238
Download installer here

[github-actions]

🚀 New build available for commit f5824a4
Download installer here

[atsju]

Hi @gr5
while this PR is not ready for review, could you have a look at it and tell me honnestly what you think about it ?
It's quite large and indirectly does more than title says.
I added some comments in the review pane and in presentation to help out.
The build should be functionnal except for elipse related features.

I have a new personal project restoring an old lathe, so I would rather not spend hours finishing this if it doesn't pass initial smell test. I already spent quite some time to arrive here.

[atsju]

all direct access to Qsettings have been repalced with settingsFacade

[atsju]

all parameters like lambda, diameter and other are in a settings struc of the mirror for better access control and management when settings are modifed

[atsju]

mirror settings (draft during edition and current when accepted) are no private and managed with accessors.

[atsju]

facade file is scalable and supposed to be the only entry point for every Qsettings (not in this PR, but in extremely long term it's doable if we want)

[atsju]

the store is accessed by facade and uses some macro to read the settingsstores_fields.inc file

[atsju]

this is the really neat part
FIELD(type, memberName, defaultValue, key, converter)
Everthing is typed here only once. It's the main source a truth.
I know Dale has been extremelly carefull to copy paste the same default aclues, key strings, converters but this solved programatically the risk of error.

It also centralises all the used keys in one knwoledge file. I did not change any key to not break compatibility.

[gr5]

I think this should be ".h". I read about .inc but I use 3 different editors regularly to look at the code. QT is great where I can right click something and find it's definition. But the other editors I have to specify which type of file to search through and I don't want to add ".inc" or ".*".

You didn't define FIELD macro/function yet, right? I didn't find that anywhere. I assume that's what you meant by you still had work to do.

[atsju]

I can rename to .h no problem. It was .inc because it's not "really" code.

FIELD is not a macro this all already works.

search for SETTINGS_STORE_FOR_EACH_MIRROR_FIELD and SETTINGS_STORE_DECLARE_STRUCT_FIELD for example

[gr5]

Maybe put a few starter comments in there. Like how roc and diameter are in mm. That will remind/encourage me to add other comments in the future if I'm trying to understand some setting.

In QT creator I put the cursor on a line of code in averagewavefrontfilesdlg.cpp where it was looking at lambda. I hit F2 key and it properly jumped to correct line in the .inc file. So if there was a comment there I would see it.

It is definitely nice to see the defaults there on the same line as the variable type (double) and what it's called in the registry.

[gr5]

I will try to get to this tomorrow. I spent most of my days away from the computer today and yesterday. Tomorrow I should have more time.

[gr5]

I love the bit where you have m_draft and m_current.

By the way please add an "Apply" button to the mirror dialog so people can see the result of their change without closing the dialog.

I know your love your simplification of storing these variables but there are only 15. Someone trying to understand the code would normally look at the 15 in the .h file and possibly get to see comments about each one explaining things like the units (mm versus inches) and other helpful details. Not sure where to look for that info. I guess in the ".inc" file.

I don't hate it. I don't love it.

If Dale wants to add another member variable to mirrordlg, will he figure this out? Will he know to go to the settingsstores_fields.inc file?

It is pretty nice and elegant. You have such ambition - I love that part.

Let me talk to Dale.

[atsju]

Yes "apply" button is a good idea.

I also had in mind to add comments about each of the qsettings into the .inc to centralise it but somehow I didn't end doing it.

Note the settings store is merely a proof of the concept and is applied only for mirror settings. there are plenty of other Qsettings that could use the same way (or not). I believe it's arround 210 settings in total QsettingsList.txt.

Edit: I probably misunderstood you were talking about the 15 member variables in stuct or the 15 Qsettings. Anyway, most of comment is true for both.

[atsju]

Thank you for having looked at it !

[githubdoe]

I want to make sure you know that mirror settings should never be saved in the QSettings as the source for the mirror project. They are unique to each mirror and test setup. So they must be read at startup each time from the current mirror configuration file. Not from QSettings.

[gr5]

What? What if there is no mirror file? And if the mirror is set to the latest mirror file, then qsettings should also reflect the settings of the latest mirror file anyway so why would it matter.

Isn't the way it works now is that it's stored in qsettings?

[gr5]

When you say "mirror project" are you referring to this PR? This pr is about mirrorDlg. I assume that's what you mean by "mirror project".

[gr5]

Dale I sent you a long email yesterday. I hope you got it.

[githubdoe]

No I did not get it.

[githubdoe]

mirror project is the thing a user creates when testing a mirror. Usually unique to each mirror. Usually a directory with at least a mirror config file. From dftfring you switch mirror projects by loading a different mirror config. IIRC the source of truth for DFTFringe is the mirror dialog which was created from the mirror config. I don't remember if any classes use the Qsettings to get mirror config data. But if they do that was what I use to do. These days I always use the mirror dialog. It is a singleton just for that purpose.

Yes the mirror dialog stores it state in QSettings and recovers them. from there. Yes some people like an apply button. I don't think I ever thought it needed one. It would be nice to allow the user to ack out recent changes when they made a mistake. I was just too lazy to implement that.

[gr5]

Dale I sent you a long email yesterday. I hope you got it.

I just resent it.