Skip to content

Update sg_BoincSimpleFrame.cpp - #7216

Open
LezheGao wants to merge 4 commits into
BOINC:masterfrom
LezheGao:patch-1
Open

Update sg_BoincSimpleFrame.cpp#7216
LezheGao wants to merge 4 commits into
BOINC:masterfrom
LezheGao:patch-1

Conversation

@LezheGao

@LezheGao LezheGao commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Fixes #6103

Description of the Change
When a user selects the "Default" skin via the View → Skin menu, the GUI reloads the default appearance, but the selection is never saved to the configuration file. On next startup, RestoreState() reads the previously saved (non‑default) skin name and re‑applies that skin, effectively discarding the user's choice.

The fix adds the same persistence logic already present in OnSelectSkin() for non‑default skins to OnSelectDefaultSkin():

  • wxGetApp().SaveState() – saves the current skin name via CSkinManager
  • wxConfigBase::Get(FALSE)->Flush() – ensures the config is written to disk immediately

This makes the behaviour consistent across all skin selections and ensures the default skin is remembered across sessions.

Assisted-by: (not using an agent): DeepSeek-V4

Alternate Designs

  • A more extensive refactoring could unify the persistence logic into a helper method (e.g., SaveSelectedSkin()), but that would touch more files and add unnecessary churn for a two‑line fix.

The chosen approach is minimal, safe, and mirrors the existing working implementation for non‑default skins, reducing the risk of regression.

Release Notes
Fixed an issue where selecting the default skin did not persist after restarting BOINC Manager.


Summary by cubic

Persists selecting the “Default” skin so BOINC Manager restores it after restart; previously it reapplied the last saved non‑default skin. Also guards config flushing to avoid a null dereference. Fixes #6103.

Changes

  • In OnSelectDefaultSkin(): explicitly call SetSelectedSkin() with the default name, reload the skin, then call wxGetApp().SaveState() and wxConfigBase::Get(FALSE)->Flush() only if a config exists.
  • In OnSelectSkin(): guard the Flush() call behind a null check on wxConfigBase::Get(FALSE).

Written for commit 74514e7. Summary will update on new commits.

Review in cubic

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 1 file

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread clientgui/sg_BoincSimpleFrame.cpp Outdated
@CharlieFenton

Copy link
Copy Markdown
Contributor

This is unnecessary. The change to default skin is remembered on exit from BOINC Manager and restored when BOINC Manager is relaunched. It works correctly in BOINC 8.2.15 on my Mac.

The default skin is already saved when CBOINCGUIApp::OnExit() calls CBOINCGUIApp::SaveState():

    if (m_pSkinManager) {
        m_pConfig->Write(wxT("Skin"), m_pSkinManager->GetSelectedSkin());
    }

@AenBleidd

Copy link
Copy Markdown
Member

@CharlieFenton, however, we still have the issue desribed.
I believe this is just the fix that is incorrect.
@LezheGao, it looks like you are using some kind of AI.
Have you followed our AI policy?

@CharlieFenton

Copy link
Copy Markdown
Contributor

we still have the issue described.

I don't think that issue exists. I cannot reproduce this issue on the Mac, though I have no way to test it on Linux. It is unclear in the description of #6103 whether by "Close Manager" and "Open Manager again" he means closing and reopening the window or exiting the Manager and relaunching it. But I tested both interpretations following his steps and the default skin was preserved.

@CharlieFenton

Copy link
Copy Markdown
Contributor

In issue #6103 @Vulpine05 wrote:

I was not able to reproduce this with a local build on Windows 10.

If the issue exists on Linux but not on Mac or Windows, it seems to imply that CBOINCGUIApp::SaveState() is not being called when the user exits the Manager (assuming he means exiting the Manager and relaunching it.) If so, that is a much bigger issue.

But if he means closing and reopening the Manager window, then calling CBOINCGUIApp::SaveState() then calling CBOINCGUIApp::SaveState() won't fix it because the value is read only in CBOINCGUIApp::OnInit() when the Manager is first launched.

@LezheGao

LezheGao commented Aug 12, 2026

Copy link
Copy Markdown
Contributor Author

Have you followed our AI policy?

Fixed

@davidpanderson

Copy link
Copy Markdown
Contributor

I'm not sure this applies here, but:
config changes should be changed when the user makes them,
not when the manager exits.

Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
@LezheGao
LezheGao marked this pull request as draft August 12, 2026 14:34
@LezheGao
LezheGao marked this pull request as ready for review August 12, 2026 14:37

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 1 file

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="clientgui/sg_BoincSimpleFrame.cpp">

<violation number="1" location="clientgui/sg_BoincSimpleFrame.cpp:588">
P3: The newly added persistence block in `OnSelectDefaultSkin()` is byte-for-byte identical to the block in `OnSelectSkin()` (both call `wxGetApp().SaveState()` then flush the config). This 5-line sequence is now duplicated across the two skin-selection handlers, so any future change to persistence (e.g., flushing under another config path or adding error handling) must be made in two places and can drift. The PR description acknowledges a helper could unify this but chose the two-line approach; a small private helper such as `SaveSelectedSkin()` would remove the duplication with minimal churn.</violation>
</file>

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

// the name of the default skin is not localized
pSkinManager->ReloadSkin(pSkinManager->GetDefaultSkinName());

wxGetApp().SaveState();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3: The newly added persistence block in OnSelectDefaultSkin() is byte-for-byte identical to the block in OnSelectSkin() (both call wxGetApp().SaveState() then flush the config). This 5-line sequence is now duplicated across the two skin-selection handlers, so any future change to persistence (e.g., flushing under another config path or adding error handling) must be made in two places and can drift. The PR description acknowledges a helper could unify this but chose the two-line approach; a small private helper such as SaveSelectedSkin() would remove the duplication with minimal churn.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At clientgui/sg_BoincSimpleFrame.cpp, line 588:

<comment>The newly added persistence block in `OnSelectDefaultSkin()` is byte-for-byte identical to the block in `OnSelectSkin()` (both call `wxGetApp().SaveState()` then flush the config). This 5-line sequence is now duplicated across the two skin-selection handlers, so any future change to persistence (e.g., flushing under another config path or adding error handling) must be made in two places and can drift. The PR description acknowledges a helper could unify this but chose the two-line approach; a small private helper such as `SaveSelectedSkin()` would remove the duplication with minimal churn.</comment>

<file context>
@@ -584,6 +584,12 @@ void CSimpleFrame::OnSelectDefaultSkin( wxCommandEvent& WXUNUSED(event) ) {
     // the name of the default skin is not localized
     pSkinManager->ReloadSkin(pSkinManager->GetDefaultSkinName());
+
+    wxGetApp().SaveState();
+    wxConfigBase* pConfig = wxConfigBase::Get(FALSE);
+    if (pConfig) {
</file context>

@LezheGao

Copy link
Copy Markdown
Contributor Author

@CharlieFenton, however, we still have the issue desribed. I believe this is just the fix that is incorrect.

Hopefully 74514e7 will work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Manager] [Simple View] Unable to permanently revert back to default skin

4 participants