Repository: RetroAchievements/RAIntegration
Pull request #871 (2022-02-25 01:40:55)

prevent note from being changed while confirming overwrite
Fixes an issue where clicking on another note while being asked to confirm overwriting a note will send the newly selected note's data to the server instead of the original data. Depending on the new note's length compared to the original note, it might also result in a crash.


Patch:
diff --git a/src/RA_Shared.rc b/src/RA_Shared.rc
index cb18360a..db09f876 100644
--- a/src/RA_Shared.rc
+++ b/src/RA_Shared.rc
@@ -272,16 +272,16 @@ BEGIN
 END
 
 IDD_RA_CODENOTES DIALOGEX 0, 0, 287, 230
-STYLE DS_SETFONT | WS_POPUP | WS_CAPTION | WS_SYSMENU | WS_THICKFRAME
+STYLE DS_SETFONT | DS_FIXEDSYS | WS_POPUP | WS_CAPTION | WS_SYSMENU | WS_THICKFRAME
 CAPTION "Code Notes"
 FONT 8, "MS Shell Dlg", 400, 0, 0x0
 BEGIN
     LTEXT           "Search:",IDC_STATIC,6,6,33,11
     EDITTEXT        IDC_RA_FILTER_VALUE,37,4,194,14,ES_AUTOHSCROLL
     PUSHBUTTON      "Filter",IDC_RA_APPLY_FILTER,233,4,50,14
-    LTEXT           "Results:",IDC_STATIC,6,24,33,8
-    LTEXT           "0/0",IDC_RA_RESULT_COUNT,37,24,49,8
-    PUSHBUTTON      "Bookmark Selected",IDC_RA_ADDBOOKMARK,90,21,70,14,BS_MULTILINE
+    LTEXT           "Results:",IDC_STATIC,6,24,36,8
+    LTEXT           "99999/99999",IDC_RA_RESULT_COUNT,37,24,42,8
+    PUSHBUTTON      "Bookmark Selected",IDC_RA_ADDBOOKMARK,162,21,70,14
     PUSHBUTTON      "Reset",IDC_RA_RESET_FILTER,233,21,50,14
     CONTROL         "",IDC_RA_LBX_ADDRESSES,"SysListView32",LVS_REPORT | LVS_SINGLESEL | LVS_ALIGNLEFT | LVS_NOSORTHEADER | WS_CLIPCHILDREN | WS_BORDER,4,37,279,189
 END
@@ -330,7 +330,8 @@ BEGIN
     PUSHBUTTON      "&Cancel",IDCANCEL,136,16,60,14
 END
 
-///////////////////////////////////////////////////////////
+
+/////////////////////////////////////////////////////////////////////////////
 //
 // DESIGNINFO
 //
@@ -528,7 +529,7 @@ BEGIN
         HORZGUIDE, 107
         HORZGUIDE, 123
     END
-    
+
     IDD_RA_NEWASSET, DIALOG
     BEGIN
         LEFTMARGIN, 4
@@ -552,7 +553,7 @@ END
 2 TEXTINCLUDE 
 BEGIN
     "#include ""winresrc.h""\r\n"
-    "#define IDC_STATIC -1"\r\n
+    "#define IDC_STATIC -1\r\n"
     "\0"
 END
 
diff --git a/src/services/AchievementRuntime.cpp b/src/services/AchievementRuntime.cpp
index 0a9dc706..1e6a7bd8 100644
--- a/src/services/AchievementRuntime.cpp
+++ b/src/services/AchievementRuntime.cpp
@@ -42,16 +42,9 @@ int AchievementRuntime::ActivateAchievement(ra::AchievementID nId, const std::st
     std::lock_guard<std::mutex> pLock(m_pMutex);
     EnsureInitialized();
 
-    if (m_bPaused)
-    {
-        // If processing is paused, just queue the achievement. Once processing is unpaused, if the trigger
-        // is not false, the achievement will be moved to the active list. This ensures achievements don't
-        // trigger immediately upon loading the game due to uninitialized memory.
-        m_vQueuedAchievements.insert_or_assign(nId, sTrigger);
-        rc_runtime_deactivate_achievement(&m_pRuntime, nId);
-        return RC_OK;
-    }
-
+    // When an achievement is activated, it's set to Waiting. The state of the achievement must evaluate
+    // to false for at least one frame before it is promoted to Active. This ensures achievements don't
+    // trigger from uninitialized (or randomly initialized) memory when first starting a game.
     return rc_runtime_activate_achievement(&m_pRuntime, nId, sTrigger.c_str(), nullptr, 0);
 }
 
@@ -203,21 +196,6 @@ std::wstring AchievementRuntime::GetRichPresenceDisplayString() const
     return std::wstring(L"No Rich Presence defined.");
 }
 
-void AchievementRuntime::SetPaused(bool bValue) 
-{ 
-    if (!bValue && m_bPaused && !m_vQueuedAchievements.empty())
-    {
-        m_bPaused = false;
-
-        for (const auto pPair : m_vQueuedAchievements)
-            ActivateAchievement(pPair.first, pPair.second);
-
-        m_vQueuedAchievements.clear();
-    }
-
-    m_bPaused = bValue; 
-}
-
 static std::vector<AchievementRuntime::Change>* g_pChanges;
 
 static void map_event_to_change(const rc_runtime_event_t* pRuntimeEvent)
diff --git a/src/services/AchievementRuntime.hh b/src/services/AchievementRuntime.hh
index c2061648..d90d6648 100644
--- a/src/services/AchievementRuntime.hh
+++ b/src/services/AchievementRuntime.hh
@@ -233,7 +233,7 @@ public:
     /// <summary>
     /// Sets whether achievement processing should be temporarily suspended.
     /// </summary>
-    void SetPaused(bool bValue);
+    void SetPaused(bool bValue) noexcept { m_bPaused = bValue; }
 
     /// <summary>
     /// Resets any active achievements and disables them until their triggers are false.
@@ -262,7 +262,6 @@ private:
 
     void EnsureInitialized() noexcept;
 
-    std::map<unsigned int, std::string> m_vQueuedAchievements;
     int m_nRichPresenceParseResult = RC_OK;
     int m_nRichPresenceErrorLine = 0;
     bool m_bInitialized = false;
diff --git a/src/services/SearchResults.cpp b/src/services/SearchResults.cpp
index e1183679..33f7ad63 100644
--- a/src/services/SearchResults.cpp
+++ b/src/services/SearchResults.cpp
@@ -1601,7 +1601,7 @@ void SearchResults::MergeSearchResults(const SearchResults& srMemory, const Sear
     m_nFilterType = srAddresses.m_nFilterType;
     m_nFilterValue = srAddresses.m_nFilterValue;
 
-    for (const auto pSrcBlock : srAddresses.m_vBlocks)
+    for (const auto& pSrcBlock : srAddresses.m_vBlocks)
     {
         // copy the block from the srAddresses collection, then update the memory from the srMemory collection.
         // this creates a new block with the memory from the srMemory collection and the addresses from the
@@ -1611,7 +1611,7 @@ void SearchResults::MergeSearchResults(const SearchResults& srMemory, const Sear
         ra::ByteAddress nAddress = pNewBlock.GetFirstAddress();
         unsigned char* pWrite = pNewBlock.GetBytes();
 
-        for (const auto pMemBlock : srMemory.m_vBlocks)
+        for (const auto& pMemBlock : srMemory.m_vBlocks)
         {
             if (nAddress >= pMemBlock.GetFirstAddress() && nAddress < pMemBlock.GetFirstAddress() + pMemBlock.GetBytesSize())
             {
diff --git a/src/ui/viewmodels/MemoryInspectorViewModel.cpp b/src/ui/viewmodels/MemoryInspectorViewModel.cpp
index 9e069d4c..0e183c4d 100644
--- a/src/ui/viewmodels/MemoryInspectorViewModel.cpp
+++ b/src/ui/viewmodels/MemoryInspectorViewModel.cpp
@@ -142,7 +142,9 @@ static std::wstring ShortenNote(const std::wstring& sNote)
 
 void MemoryInspectorViewModel::SaveCurrentAddressNote()
 {
-    const auto& sNewNote = GetCurrentAddressNote();
+    // create a copy of the note. it's less efficient, but using a reference allows the value to be
+    // changed out from under us while the confirmation dialog is open.
+    const std::wstring sNewNote = GetCurrentAddressNote();
     const auto nAddress = GetCurrentAddress();
 
     std::string sAuthor;
diff --git a/src/ui/viewmodels/MemorySearchViewModel.cpp b/src/ui/viewmodels/MemorySearchViewModel.cpp
index 8bbd66aa..980b9305 100644
--- a/src/ui/viewmodels/MemorySearchViewModel.cpp
+++ b/src/ui/viewmodels/MemorySearchViewModel.cpp
@@ -637,13 +637,15 @@ void MemorySearchViewModel::ApplyContinuousFilter()
     // for 10000 results, only filter every 100ms
     // for 50000 results, only filter every 500ms
     // for 100000 results, only filter every second
+    // up to a max of one filter every ten seconds at 1000000 or more results
     const auto nResults = pResult.pResults.MatchingAddressCount();
     if (nResults > 1000)
     {
         const auto tNow = ra::services::ServiceLocator::Get<ra::services::IClock>().UpTime();
         const auto nElapsed = std::chrono::duration_cast<std::chrono::milliseconds>(tNow - m_tLastContinuousFilter);
+        const auto nElapsedMilliseconds = gsl::narrow_cast<size_t>(nElapsed.count());
 
-        if (gsl::narrow_cast<size_t>(nElapsed.count()) < nResults / 100)
+        if (nElapsedMilliseconds < 10000 && nElapsedMilliseconds < nResults / 100)
             return;
 
         m_tLastContinuousFilter = tNow;
diff --git a/src/ui/win32/CodeNotesDialog.cpp b/src/ui/win32/CodeNotesDialog.cpp
index 59060949..48c1344d 100644
--- a/src/ui/win32/CodeNotesDialog.cpp
+++ b/src/ui/win32/CodeNotesDialog.cpp
@@ -103,9 +103,12 @@ CodeNotesDialog::CodeNotesDialog(CodeNotesViewModel& vmCodeNotes)
     using namespace ra::bitwise_ops;
     SetAnchor(IDC_RA_FILTER_VALUE, Anchor::Top | Anchor::Left | Anchor::Right);
     SetAnchor(IDC_RA_APPLY_FILTER, Anchor::Top | Anchor::Right);
-    SetAnchor(IDC_RA_ADDBOOKMARK, Anchor::Top);
+    SetAnchor(IDC_RA_RESULT_COUNT, Anchor::Top | Anchor::Left);
+    SetAnchor(IDC_RA_ADDBOOKMARK, Anchor::Top | Anchor::Right);
     SetAnchor(IDC_RA_RESET_FILTER, Anchor::Top | Anchor::Right);
     SetAnchor(IDC_RA_LBX_ADDRESSES, Anchor::Top | Anchor::Left | Anchor::Bottom | Anchor::Right);
+
+    SetMinimumSize(327, 200);
 }
 
 BOOL CodeNotesDialog::OnInitDialog()


Test patch:
diff --git a/tests/services/AchievementRuntime_Tests.cpp b/tests/services/AchievementRuntime_Tests.cpp
index 2710692c..46f389ec 100644
--- a/tests/services/AchievementRuntime_Tests.cpp
+++ b/tests/services/AchievementRuntime_Tests.cpp
@@ -230,6 +230,51 @@ public:
         AssertChange(vChanges, AchievementRuntime::ChangeType::AchievementTriggered, 6U);
     }
 
+    TEST_METHOD(TestReactivateAchievementPaused)
+    {
+        std::array<unsigned char, 1> memory{ 0x00 };
+
+        AchievementRuntimeHarness runtime;
+        runtime.mockEmulatorContext.MockMemory(memory);
+        const char* pTriggerDefinition = "0xH0000=1";
+        rc_trigger_t* pTrigger;
+
+        std::vector<AchievementRuntime::Change> vChanges;
+        runtime.ActivateAchievement(6U, pTriggerDefinition);
+        pTrigger = runtime.GetAchievementTrigger(6U);
+        Assert::IsNotNull(pTrigger);
+        Assert::AreEqual(static_cast<int>(RC_TRIGGER_STATE_WAITING), static_cast<int>(pTrigger->state));
+
+        runtime.Process(vChanges);
+        AssertChange(vChanges, AchievementRuntime::ChangeType::AchievementActivated, 6U);
+        Assert::AreEqual(static_cast<int>(RC_TRIGGER_STATE_ACTIVE), static_cast<int>(pTrigger->state));
+        vChanges.clear();
+
+        runtime.SetPaused(true);
+        Assert::IsTrue(runtime.IsPaused());
+
+        runtime.DeactivateAchievement(6U);
+        Assert::IsNull(runtime.GetAchievementTrigger(6U));
+
+        // activating a previously active achievement should resurrect it, even if paused
+        runtime.ActivateAchievement(6U, pTriggerDefinition);
+        pTrigger = runtime.GetAchievementTrigger(6U);
+        Assert::IsNotNull(pTrigger);
+        Assert::AreEqual(static_cast<int>(RC_TRIGGER_STATE_WAITING), static_cast<int>(pTrigger->state));
+
+        // achievement should not become active when paused
+        runtime.SetPaused(true);
+        Assert::IsTrue(runtime.IsPaused());
+        runtime.Process(vChanges);
+        Assert::AreEqual({ 0U }, vChanges.size());
+
+        // processing re-enabled, achievement should become active
+        runtime.SetPaused(false);
+        runtime.Process(vChanges);
+        Assert::AreEqual({ 1U }, vChanges.size());
+        AssertChange(vChanges, AchievementRuntime::ChangeType::AchievementActivated, 6U);
+    }
+
     TEST_METHOD(TestMonitorAchievementReset)
     {
         std::array<unsigned char, 5> memory{0x00, 0x12, 0x34, 0xAB, 0x56};