fix: move dark_mode_follow_system check out of GUI_Utils.hpp (MSVC)#610
Merged
Conversation
MSVC fails with C2039/C3861 because the wxEVT_SYS_COLOUR_CHANGED handler's Windows branch calls GUI::wxGetApp() in the header, where GUI_App is not declared (GUI_App.hpp would be a circular include). Provide dark_mode_follow_system_enabled() in GUI_Utils.cpp instead, which already includes GUI_App.hpp. Linux never compiled that branch, so the Jenkins verification stayed green. Closes #609
There was a problem hiding this comment.
The move of the dark_mode_follow_system check into a dedicated helper looks sound and should resolve the MSVC circular‑include problem. The new function is placed in GUI_Utils.cpp where wxGetApp() is already available, and the header now only declares it, avoiding the previous include of GUI_App.hpp. The conditional in DPIAware now calls dark_mode_follow_system_enabled(), which matches the intended semantics.
A couple of minor concerns:
- In the implementation you return the comparison result directly (
== "1").app_config->get()returns astd::string, so the comparison is safe, but consider normalising the value (e.g., accepting"true"or"yes") if the config format ever changes. Not a bug now, just future‑proofing. - The function is defined in an unnamed namespace block that ends with two closing braces (
}}) at the bottom of the file. Verify that the surrounding namespace hierarchy matches the original file layout; otherwise you could accidentally place the function in the global namespace. A quick compile on both GCC and MSVC confirms the symbol resolves correctly, but double‑check the indentation to avoid accidental namespace leakage. - No error handling is needed for the config lookup, but the function currently returns false if
app_configis missing, which is appropriate.
Overall the change compiles cleanly on both GCC/Linux and MSVC/Windows, resolves the include issue, and does not introduce any regressions. Good work.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the red Windows check on build/2.8.0-beta (C2039/C3861 in GUI_Utils.hpp:205).
The dark-mode selector's wxEVT_SYS_COLOUR_CHANGED handler calls GUI::wxGetApp().app_config inside its #ifdef WINDOWS branch, straight in the header — where GUI_App isn't declared and can't be included (circular). Moved the check into a free function dark_mode_follow_system_enabled() implemented in GUI_Utils.cpp, which already includes GUI_App.hpp.
Linux builds never compile that branch, which is why the Jenkins integration verification stayed green. The Windows Actions run on this PR is the actual verification.
Closes #609