[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.