Preserve Existing MRU Computers when Saving

[ChihweiLHBird]

Fix: #6112
This is to fix an issue causing duplicated or lost of MRU computers when multiple windows (instances) of BOINC manager are saving the MRU list into the state.

[Copilot]

Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.

[ChihweiLHBird]

Hi @AenBleidd, any idea about adding tests for this change? I took a look but didn't find a good point for adding test for this functionality of BOINC Manager.

[AenBleidd]

We don't have tests for that, so I'll test that manually.

[AenBleidd]

Ok, looks like this doesn't work as intended.
Here I have tested in on the local installation.

1. Open BOINC Manager
2. Open second instance of BOINC Manager (e.g. via boincmgr.exe -m) and connect to the same machine (e.g. setting localhost)
3. Close second instance and the first instance
4. Open regedit.exe
5. Navigate to Computer\HKEY_CURRENT_USER\Software\Space Sciences Laboratory, U.C. Berkeley\BOINC Manager\ComputerMRU
6. Add duplicate entry (In my case I had 0: localhost and I added 1: localhost)
7. Open BOINC Manager
8. Close BOINC Manager.
9. Check that two duplicate values are still there (should be only one)

[ChihweiLHBird]

@AenBleidd thanks for testing it!

I just realized the current purposed change only prevents new duplicated items from being set, but it won't clean up existing ones.

I will make a change to support the cleaning up, maybe later this week.

[ChihweiLHBird]

Hi @AenBleidd, I think de-duplicating the computer list when loading the state would solve this, and it works in my local experiment. I just pushed the commit and the PR should be good for re-review now. Thank you!

[Copilot]

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

clientgui/BOINCBaseFrame.cpp:782

• [nitpick] Using the same variable 'iIndex' for both iterating over the config entries and for writing the updated list may reduce clarity; consider using a separate variable for the write loop to improve readability.

for (auto computer : existingComputers) {

[Copilot]

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.

[Copilot]

Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (2)

clientgui/BOINCBaseFrame.cpp:773

• [nitpick] Consider using a separate variable for iterating over the MRU list and for subsequent config entry numbering to avoid potential confusion with the reused iIndex variable.

for (iIndex = 0; iIndex < m_aSelectedComputerMRU.GetCount(); iIndex++) {

clientgui/BOINCBaseFrame.cpp:782

• [nitpick] Consider iterating by const reference (e.g., 'for (const auto& computer : existingComputers) {') to improve readability and efficiency.

for (auto computer : existingComputers) {

[Copilot]

Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.

[ChihweiLHBird]

Couldn't understand why the check failed... is it a bug in the check itself?

[Copilot]

Pull Request Overview

This PR fixes an issue (#6112) that caused duplicated or lost MRU computers when multiple BOINC Manager windows concurrently saved the MRU list. Key changes include:

• Using size_t for index variables for better type-safety.
• Retrieving existing computer entries from the config before overwriting.
• Writing back non-duplicated entries and cleaning up extra config entries.

Comments suppressed due to low confidence (2)

clientgui/BOINCBaseFrame.cpp:763

• [nitpick] Reusing 'iIndex' for both reading existing computers and writing the MRU list may be confusing for future maintainers. Consider using separate variables or clearly reinitializing 'iIndex' between these sections to improve clarity.

iIndex = 0;

clientgui/BOINCBaseFrame.cpp:790

• [nitpick] The reuse of 'iIndex' here to remove remaining config entries relies on its previous value from the MRU writing loop. Consider adding an inline comment clarifying this intent to aid future maintenance.

strBuffer.Printf(wxT("%zu"), iIndex);

[Copilot]

Pull Request Overview

This PR updates how the MRU (Most Recently Used) computer list is saved and restored so that entries added by other running BOINC Manager windows are preserved rather than duplicated or dropped.

• Changed MRU index variables to size_t and introduced existingComputers to merge saved entries across instances
• In SaveState(), reads all existing entries before writing the current MRU list, appends any new external entries, then removes stale keys
• In RestoreState(), prevents adding duplicates when rebuilding the in-memory MRU list

Comments suppressed due to low confidence (2)

clientgui/BOINCBaseFrame.cpp:733

• [nitpick] The variable iIndex is used for multiple distinct loops (reading, writing, deleting). Consider renaming to readIndex, writeIndex, and deleteIndex to improve readability.

size_t          iIndex;

clientgui/BOINCBaseFrame.cpp:773

• This complex merge logic would benefit from dedicated unit or integration tests to ensure that MRU entries are correctly preserved and merged when multiple manager instances save concurrently.

for (iIndex = 0; iIndex < m_aSelectedComputerMRU.GetCount(); iIndex++) {

[Copilot]

[nitpick] The pattern for iterating over numeric keys and reading entries is repeated. Extract this into a helper function to reduce duplication and make the logic clearer.

Suggested change

	strBuffer.Printf(wxT("%zu"), iIndex);
	while (pConfig->Exists(strBuffer)) {
	wxString computer = pConfig->Read(strBuffer, wxEmptyString);
	if (computer != wxEmptyString && existingComputers.Index(computer) == wxNOT_FOUND) {
	existingComputers.Add(computer);
	}
	strBuffer.Printf(wxT("%zu"), ++iIndex);
	}
	for (iIndex = 0; iIndex < m_aSelectedComputerMRU.GetCount(); iIndex++) {
	strBuffer.Printf(wxT("%zu"), iIndex);
	pConfig->Write(
	strBuffer,
	m_aSelectedComputerMRU.Item(iIndex)
	);
	}
	IterateNumericKeys(pConfig, wxT(""), [&](const wxString& key) {
	wxString computer = pConfig->Read(key, wxEmptyString);
	if (computer != wxEmptyString && existingComputers.Index(computer) == wxNOT_FOUND) {
	existingComputers.Add(computer);
	}
	});
	IterateNumericKeys(pConfig, wxT(""), [&](const wxString& key, size_t index) {
	pConfig->Write(
	key,
	m_aSelectedComputerMRU.Item(index)
	);
	});

[Copilot]

[nitpick] Searching existingComputers with Index() is O(n) for each lookup. If the MRU list grows large, consider using a hash-based container (e.g., std::unordered_set) for faster membership checks.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 11.93%. Comparing base (5f3e2ee) to head (a9d2077).
Report is 121 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #6110      +/-   ##
============================================
- Coverage     11.94%   11.93%   -0.01%     
  Complexity     1068     1068              
============================================
  Files           278      278              
  Lines         36967    36985      +18     
  Branches       8541     8544       +3     
============================================
  Hits           4416     4416              
- Misses        32150    32168      +18     
  Partials        401      401              

see 11 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.