refactor(preferences): Move OptionPreferences class into separate files by bobtista · Pull Request #1840 · TheSuperHackers/GeneralsGameCode

bobtista · 2025-11-13T03:18:47Z

Closes Move OptionPreferences class into separate files #1374
Finalize and Merge after unify(userpreferences): Merge and move UserPreferences to Core #2182

Separates the OptionPreferences class from UserPreferences.h and OptionsMenu.cpp into dedicated files in Core.

Changes

New files created in Core:

Core/GameEngine/Include/Common/OptionPreferences.h - Class declaration
Core/GameEngine/Source/Common/OptionPreferences.cpp - Implementation (uses GeneralsMD version which has the superset of methods)

Generals/Code/GameEngine/Include/Common/UserPreferences.h

GeneralsMD/Code/GameEngine/Include/Common/UserPreferences.h

xezon

The opening post says that files were added to Core, but that is not correct. And they cant be added to Core, because the files are not yet merged.

Generals/Code/GameEngine/CMakeLists.txt

Generals/Code/GameEngine/Include/Common/UserPreferences.h

GeneralsMD/Code/GameEngine/Source/GameClient/GUI/GUICallbacks/Menus/OptionsMenu.cpp

GeneralsMD/Code/GameEngine/Include/Common/UserPreferences.h

Core/GameEngine/Include/Common/UserPreferences.h

Generals/Code/GameEngine/Source/GameClient/GUI/GUICallbacks/Menus/OptionsMenu.cpp

xezon · 2026-01-24T19:57:27Z

Core/GameEngine/Include/Common/UserPreferences.h

@@ -72,82 +72,8 @@ class UserPreferences : public PreferenceMap
 	AsciiString m_filename;
 };

-//-----------------------------------------------------------------------------


Now this moved away, I think you can remove the forward declarations above.

xezon · 2026-01-24T20:57:53Z

I think it would be good to merge and move user preferences to Core first, but it will require merging #2180 first.

xezon · 2026-01-24T21:37:14Z

I have merged the UserPreferences code and moved it to Core with #2182. So when that is merged you can directly split off to Core and do not need 2 copies for it.

xezon · 2026-02-08T11:00:48Z

This pull can be advanced.

greptile-apps · 2026-02-16T17:54:29Z

Greptile Summary

Successful refactoring that extracts the OptionPreferences class from UserPreferences.h and OptionsMenu.cpp into dedicated files in Core. All previous review concerns have been resolved:

Key Changes:

Created Core/GameEngine/Include/Common/OptionPreferences.h with class declaration
Created Core/GameEngine/Source/Common/OptionPreferences.cpp with implementation (~850 lines moved from OptionsMenu.cpp)
Moved CursorCaptureMode and ScreenEdgeScrollMode enums from Generals-specific headers (Mouse.h, LookAtXlat.h) to Core header, resolving previous layering violations
Updated CMakeLists.txt with correct alphabetical placement
Updated 40+ files across Generals and GeneralsMD to include the new header
Removed ~1600 lines of duplicate implementation from both OptionsMenu.cpp files

Previous Issues Resolved:

Enum constants (CursorCaptureMode_Default, ScreenEdgeScrollMode_Default) now properly defined in Core
CMakeLists.txt entries now in correct alphabetical order
No layering violations - Core can now compile independently

Confidence Score: 5/5

Safe to merge - clean refactoring with all previous issues resolved
This is a well-executed refactoring that properly addresses all layering concerns from previous reviews. The enums were correctly moved from game-specific headers to Core, the implementation was cleanly extracted, and all 46 files were consistently updated. No logic changes, just structural improvements.
No files require special attention

Important Files Changed

Filename	Overview
Core/GameEngine/Include/Common/OptionPreferences.h	New header defining `OptionPreferences` class and related enums (`CursorCaptureMode`, `ScreenEdgeScrollMode`) moved from Generals-specific headers to Core for proper layering
Core/GameEngine/Source/Common/OptionPreferences.cpp	Implementation moved from `OptionsMenu.cpp` to Core, now properly accesses enum constants defined in the header
Core/GameEngine/CMakeLists.txt	Added new header and source files in correct alphabetical positions
Core/GameEngine/Include/Common/UserPreferences.h	Removed `OptionPreferences` class declaration and related typedefs, now defined in dedicated header
Generals/Code/GameEngine/Include/GameClient/Mouse.h	Removed `CursorCaptureMode` enum definition (moved to Core), now includes `OptionPreferences.h`
Generals/Code/GameEngine/Include/GameClient/LookAtXlat.h	Removed `ScreenEdgeScrollMode` enum definition (moved to Core), now includes `OptionPreferences.h`
Generals/Code/GameEngine/Source/GameClient/GUI/GUICallbacks/Menus/OptionsMenu.cpp	Removed ~800 lines of `OptionPreferences` implementation (moved to Core), updated includes
GeneralsMD/Code/GameEngine/Source/GameClient/GUI/GUICallbacks/Menus/OptionsMenu.cpp	Removed ~800 lines of `OptionPreferences` implementation (moved to Core), updated includes

Flowchart

flowchart TD
    A[UserPreferences.h in Core] -->|Before: Contains| B[OptionPreferences class]
    C[OptionsMenu.cpp in Generals] -->|Before: Contains| D[OptionPreferences implementation]
    E[Mouse.h in Generals/MD] -->|Before: Defines| F[CursorCaptureMode enum]
    G[LookAtXlat.h in Generals/MD] -->|Before: Defines| H[ScreenEdgeScrollMode enum]
    
    I[New: OptionPreferences.h in Core] -->|After: Contains| J[OptionPreferences class]
    I -->|After: Defines| K[CursorCaptureMode enum]
    I -->|After: Defines| L[ScreenEdgeScrollMode enum]
    M[New: OptionPreferences.cpp in Core] -->|After: Contains| N[OptionPreferences implementation]
    
    O[Mouse.h in Generals/MD] -->|After: Includes| I
    P[LookAtXlat.h in Generals/MD] -->|After: Includes| I
    Q[OptionsMenu.cpp in Generals/MD] -->|After: Includes| I
    
    style I fill:#90EE90
    style M fill:#90EE90
    style J fill:#ADD8E6
    style K fill:#ADD8E6
    style L fill:#ADD8E6
    style N fill:#ADD8E6

_{Last reviewed commit: 0c3f843}

greptile-apps

_{6 files reviewed, 2 comments}

_{Edit Code Review Agent Settings | Greptile}

Core/GameEngine/Source/Common/OptionPreferences.cpp

greptile-apps

_{46 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

Core/GameEngine/CMakeLists.txt

…ms to Core

xezon · 2026-02-16T21:18:29Z

Core/GameEngine/Include/Common/OptionPreferences.h

+
+	Bool loadFromIniFile();
+
+	UnsignedInt getLANIPAddress(void);				// convenience function


While at it, perhaps remove this useless comments

xezon · 2026-02-16T21:20:52Z

Core/GameEngine/Include/Common/OptionPreferences.h

+};
+
+typedef UnsignedInt ScreenEdgeScrollMode;
+enum ScreenEdgeScrollMode_ CPP_11(: ScreenEdgeScrollMode)


These enum definitions should not be moved to this file.

So OptionPreferences.cpp uses CursorCaptureMode_* and ScreenEdgeScrollMode_* constants (in Mouse.h and LookAtXlat.h). Since Core shouldn't depend on upper layers, what would you suggest? eg local constants in OptionPreferences.cpp that mirror the values from Mouse.h/LookAtXlat.h? Create a shared header in Core (eg InputModeDefs.h) that both Core and Generals/GeneralsMD use?

Core can happily use files from Generals and GeneralsMD

xezon · 2026-02-16T21:22:29Z

Core/GameEngine/Source/GameNetwork/GameSpy/Thread/PeerThread.cpp


 #include "PreRTS.h"	// This must go first in EVERY cpp file in the GameEngine

+#include "Common/OptionPreferences.h"


Are all these new includes really necessary to compile or was this blanket added everywhere?

I could try including it somewhere else eg userpreferences but didn't want a circular dependency, I'll see how I can make it simpler

Right so OptionPreferences.h needs to include UserPreferences.h, right? Then replace UserPreferences.h with OptionPreferences.h where applicable.

bobtista force-pushed the bobtista/separate-option-preferences-files branch from 0b163bd to 32c7126 Compare November 13, 2025 03:44

bobtista marked this pull request as ready for review December 3, 2025 19:54

Skyaero42 reviewed Dec 17, 2025

View reviewed changes

Generals/Code/GameEngine/Include/Common/UserPreferences.h Outdated Show resolved Hide resolved

GeneralsMD/Code/GameEngine/Include/Common/UserPreferences.h Outdated Show resolved Hide resolved

bobtista force-pushed the bobtista/separate-option-preferences-files branch from 10f65f6 to f7e888b Compare January 14, 2026 21:12

xezon added Gen Relates to Generals ZH Relates to Zero Hour Refactor Edits the code with insignificant behavior changes, is never user facing labels Jan 24, 2026

xezon reviewed Jan 24, 2026

View reviewed changes

bobtista force-pushed the bobtista/separate-option-preferences-files branch from f7e888b to a82300b Compare February 16, 2026 17:50

greptile-apps bot reviewed Feb 16, 2026

View reviewed changes

Core/GameEngine/Source/Common/OptionPreferences.cpp Show resolved Hide resolved

Core/GameEngine/Source/Common/OptionPreferences.cpp Show resolved Hide resolved

bobtista force-pushed the bobtista/separate-option-preferences-files branch 4 times, most recently from 858cab4 to 94efa42 Compare February 16, 2026 18:27

greptile-apps bot reviewed Feb 16, 2026

View reviewed changes

Core/GameEngine/CMakeLists.txt Outdated Show resolved Hide resolved

bobtista added 4 commits February 16, 2026 14:06

refactor(Common): Separate OptionPreferences into its own files in Core

fc4b7fe

nit: cleanup whitespaces in OptionPreferences

a81afb4

refactor(Common): Move CursorCaptureMode and ScreenEdgeScrollMode enu…

94efa42

…ms to Core

fix: Move OptionPreferences.cpp to correct section in CMakeLists.txt

0c3f843

xezon reviewed Feb 16, 2026

View reviewed changes


		Bool loadFromIniFile();

		UnsignedInt getLANIPAddress(void); // convenience function


		#include "PreRTS.h" // This must go first in EVERY cpp file in the GameEngine

		#include "Common/OptionPreferences.h"

Conversation

bobtista commented Nov 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Uh oh!

Uh oh!

Uh oh!

xezon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xezon commented Jan 24, 2026

Uh oh!

xezon commented Jan 24, 2026

Uh oh!

xezon commented Feb 8, 2026

Uh oh!

greptile-apps bot commented Feb 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 5/5

Important Files Changed

Flowchart

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bobtista commented Nov 13, 2025 •

edited

Loading

greptile-apps bot commented Feb 16, 2026 •

edited

Loading