Repository: RetroAchievements/RAIntegration
Pull request #864 (2022-02-02 17:58:29)

prevent loss of custom bookmark descriptions over multiple sessions
When a custom bookmark description was loaded from a previous session, it wasn't being flagged as a custom bookmark description, so it didn't get saved for the next session if any other modifications were made to bookmarks during the session.


Patch:
diff --git a/src/data/context/EmulatorContext.cpp b/src/data/context/EmulatorContext.cpp
index 39875dce..1de5ea7a 100644
--- a/src/data/context/EmulatorContext.cpp
+++ b/src/data/context/EmulatorContext.cpp
@@ -790,14 +790,15 @@ uint32_t EmulatorContext::ReadMemory(ra::ByteAddress nAddress, MemSize nSize) co
 
 void EmulatorContext::WriteMemoryByte(ra::ByteAddress nAddress, uint8_t nValue) const
 {
+    auto nBlockAddress = nAddress;
     for (const auto& pBlock : m_vMemoryBlocks)
     {
-        if (nAddress < pBlock.size)
+        if (nBlockAddress < pBlock.size)
         {
             if (!pBlock.write)
                 return;
 
-            pBlock.write(nAddress, nValue);
+            pBlock.write(nBlockAddress, nValue);
             m_bMemoryModified = true;
 
             // create a copy of the list of pointers in case it's modified by one of the callbacks
@@ -811,7 +812,7 @@ void EmulatorContext::WriteMemoryByte(ra::ByteAddress nAddress, uint8_t nValue)
             return;
         }
 
-        nAddress -= gsl::narrow_cast<ra::ByteAddress>(pBlock.size);
+        nBlockAddress -= gsl::narrow_cast<ra::ByteAddress>(pBlock.size);
     }
 }
 
diff --git a/src/ui/viewmodels/MemoryBookmarksViewModel.cpp b/src/ui/viewmodels/MemoryBookmarksViewModel.cpp
index d58771d4..af52d8f5 100644
--- a/src/ui/viewmodels/MemoryBookmarksViewModel.cpp
+++ b/src/ui/viewmodels/MemoryBookmarksViewModel.cpp
@@ -318,8 +318,18 @@ void MemoryBookmarksViewModel::OnCodeNoteChanged(ra::ByteAddress nAddress, const
     for (gsl::index nIndex = 0; ra::to_unsigned(nIndex) < m_vBookmarks.Count(); ++nIndex)
     {
         auto* pBookmark = m_vBookmarks.GetItemAt(nIndex);
-        if (pBookmark && pBookmark->GetAddress() == nAddress && !pBookmark->IsCustomDescription())
-            pBookmark->SetDescription(sNewNote);
+        if (pBookmark && pBookmark->GetAddress() == nAddress)
+        {
+            if (!pBookmark->IsCustomDescription())
+            {
+                pBookmark->SetDescription(sNewNote);
+            }
+            else
+            {
+                if (pBookmark->GetDescription() == sNewNote)
+                    pBookmark->SetIsCustomDescription(false);
+            }
+        }
     }
 }
 
@@ -403,14 +413,17 @@ void MemoryBookmarksViewModel::LoadBookmarks(ra::services::TextReader& sBookmark
                     vmBookmark->SetAddress(bookmark["Address"].GetUint());
                 }
 
+                const auto* pNote = pGameContext.FindCodeNote(vmBookmark->GetAddress());
+
                 std::wstring sDescription;
                 if (bookmark.HasMember("Description"))
                 {
                     sDescription = ra::Widen(bookmark["Description"].GetString());
+                    vmBookmark->SetIsCustomDescription(!pNote || sDescription != *pNote);
                 }
                 else
                 {
-                    const auto* pNote = pGameContext.FindCodeNote(vmBookmark->GetAddress());
+                    vmBookmark->SetIsCustomDescription(false);
                     if (pNote)
                         sDescription = *pNote;
                 }


Test patch:
diff --git a/tests/data/context/EmulatorContext_Tests.cpp b/tests/data/context/EmulatorContext_Tests.cpp
index df15e4f8..264ef890 100644
--- a/tests/data/context/EmulatorContext_Tests.cpp
+++ b/tests/data/context/EmulatorContext_Tests.cpp
@@ -82,6 +82,19 @@ private:
         }
     };
 
+    class EmulatorContextNotifyHarness : public EmulatorContext::NotifyTarget
+    {
+    public:
+        void OnByteWritten(ra::ByteAddress nAddress, uint8_t nValue) noexcept override
+        {
+            nLastByteWrittenAddress = nAddress;
+            nLastByteWritten = nValue;
+        }
+
+        ra::ByteAddress nLastByteWrittenAddress = 0xFFFFFFFFU;
+        uint8_t nLastByteWritten = 0xFF;
+    };
+
 public:
     TEST_METHOD(TestClientName)
     {
@@ -1342,6 +1355,42 @@ public:
             Assert::AreEqual(i, memory.at(i));
     }
 
+    TEST_METHOD(TestWriteMemoryByteEvents)
+    {
+        InitializeMemory();
+
+        EmulatorContextHarness emulator;
+        emulator.AddMemoryBlock(0, 20, &ReadMemory0, &WriteMemory0);
+        emulator.AddMemoryBlock(1, 10, &ReadMemory2, &WriteMemory2);
+        Assert::AreEqual({ 30U }, emulator.TotalMemorySize());
+
+        EmulatorContextNotifyHarness notify;
+        emulator.AddNotifyTarget(notify);
+
+        // write to first block
+        uint8_t nByte = 0xCE;
+        emulator.WriteMemoryByte(6U, nByte);
+        Assert::AreEqual(6U, notify.nLastByteWrittenAddress);
+        Assert::AreEqual(nByte, notify.nLastByteWritten);
+
+        // write to second block
+        nByte = 0xEE;
+        emulator.WriteMemoryByte(26U, nByte);
+        Assert::AreEqual(26U, notify.nLastByteWrittenAddress);
+        Assert::AreEqual(nByte, notify.nLastByteWritten);
+
+        // write to invalid address
+        emulator.WriteMemoryByte(36U, nByte + 3);
+        Assert::AreEqual(26U, notify.nLastByteWrittenAddress);
+        Assert::AreEqual(nByte, notify.nLastByteWritten);
+
+        // write partial value
+        nByte = 0xC4;
+        emulator.WriteMemory(6U, MemSize::Nibble_Lower, 0x04);
+        Assert::AreEqual(6U, notify.nLastByteWrittenAddress);
+        Assert::AreEqual(nByte, notify.nLastByteWritten);
+    }
+
     TEST_METHOD(TestWriteMemory)
     {
         memory.at(4) = 0xA8;
diff --git a/tests/ui/viewmodels/MemoryBookmarksViewModel_Tests.cpp b/tests/ui/viewmodels/MemoryBookmarksViewModel_Tests.cpp
index 4288d7e4..56a9ec68 100644
--- a/tests/ui/viewmodels/MemoryBookmarksViewModel_Tests.cpp
+++ b/tests/ui/viewmodels/MemoryBookmarksViewModel_Tests.cpp
@@ -264,6 +264,66 @@ public:
         Assert::AreEqual(1234U, bookmark.GetAddress());
         Assert::AreEqual(MemSize::EightBit, bookmark.GetSize());
         Assert::AreEqual((int)MemFormat::Hex, (int)bookmark.GetFormat());
+        Assert::IsFalse(bookmark.IsCustomDescription());
+    }
+
+    TEST_METHOD(TestLoadBookmarksDescriptionFromFile)
+    {
+        MemoryBookmarksViewModelHarness bookmarks;
+        bookmarks.SetIsVisible(true);
+        bookmarks.mockGameContext.SetGameId(3U);
+        bookmarks.mockLocalStorage.MockStoredData(ra::services::StorageItemType::Bookmarks, L"3",
+            "{\"Bookmarks\":[{\"Address\":1234,\"Size\":10,\"Description\":\"desc\"}]}");
+
+        bookmarks.mockGameContext.NotifyActiveGameChanged();
+
+        Assert::AreEqual({ 1U }, bookmarks.Bookmarks().Count());
+        const auto& bookmark = *bookmarks.Bookmarks().GetItemAt(0);
+        Assert::AreEqual(std::wstring(L"desc"), bookmark.GetDescription());
+        Assert::AreEqual(1234U, bookmark.GetAddress());
+        Assert::AreEqual(MemSize::EightBit, bookmark.GetSize());
+        Assert::AreEqual((int)MemFormat::Hex, (int)bookmark.GetFormat());
+        Assert::IsTrue(bookmark.IsCustomDescription());
+    }
+
+    TEST_METHOD(TestLoadBookmarksDescriptionFromFileAndCodeNotes)
+    {
+        MemoryBookmarksViewModelHarness bookmarks;
+        bookmarks.SetIsVisible(true);
+        bookmarks.mockGameContext.SetGameId(3U);
+        bookmarks.mockGameContext.SetCodeNote(1234U, L"Note description");
+        bookmarks.mockLocalStorage.MockStoredData(ra::services::StorageItemType::Bookmarks, L"3",
+            "{\"Bookmarks\":[{\"Address\":1234,\"Size\":10,\"Description\":\"desc\"}]}");
+
+        bookmarks.mockGameContext.NotifyActiveGameChanged();
+
+        Assert::AreEqual({ 1U }, bookmarks.Bookmarks().Count());
+        const auto& bookmark = *bookmarks.Bookmarks().GetItemAt(0);
+        Assert::AreEqual(std::wstring(L"desc"), bookmark.GetDescription());
+        Assert::AreEqual(1234U, bookmark.GetAddress());
+        Assert::AreEqual(MemSize::EightBit, bookmark.GetSize());
+        Assert::AreEqual((int)MemFormat::Hex, (int)bookmark.GetFormat());
+        Assert::IsTrue(bookmark.IsCustomDescription());
+    }
+
+    TEST_METHOD(TestLoadBookmarksDescriptionFromFileMatchesCodeNote)
+    {
+        MemoryBookmarksViewModelHarness bookmarks;
+        bookmarks.SetIsVisible(true);
+        bookmarks.mockGameContext.SetGameId(3U);
+        bookmarks.mockGameContext.SetCodeNote(1234U, L"Note description");
+        bookmarks.mockLocalStorage.MockStoredData(ra::services::StorageItemType::Bookmarks, L"3",
+            "{\"Bookmarks\":[{\"Address\":1234,\"Size\":10,\"Description\":\"Note description\"}]}");
+
+        bookmarks.mockGameContext.NotifyActiveGameChanged();
+
+        Assert::AreEqual({ 1U }, bookmarks.Bookmarks().Count());
+        const auto& bookmark = *bookmarks.Bookmarks().GetItemAt(0);
+        Assert::AreEqual(std::wstring(L"Note description"), bookmark.GetDescription());
+        Assert::AreEqual(1234U, bookmark.GetAddress());
+        Assert::AreEqual(MemSize::EightBit, bookmark.GetSize());
+        Assert::AreEqual((int)MemFormat::Hex, (int)bookmark.GetFormat());
+        Assert::IsFalse(bookmark.IsCustomDescription());
     }
 
     TEST_METHOD(TestLoadSaveBookmarkSizes)
@@ -372,6 +432,92 @@ public:
         Assert::AreEqual(std::string("{\"Bookmarks\":[{\"MemAddr\":\"0xH04d2\",\"Decimal\":true}]}"), sContents);
     }
 
+    TEST_METHOD(TestCodeNoteChanged)
+    {
+        MemoryBookmarksViewModelHarness bookmarks;
+        bookmarks.SetIsVisible(true);
+        bookmarks.mockGameContext.SetGameId(3U);
+        bookmarks.mockGameContext.SetCodeNote(1234U, L"Note description");
+        bookmarks.mockLocalStorage.MockStoredData(ra::services::StorageItemType::Bookmarks, L"3",
+            "{\"Bookmarks\":[{\"Address\":1234,\"Size\":10}]}");
+
+        bookmarks.mockGameContext.NotifyActiveGameChanged();
+
+        Assert::AreEqual({ 1U }, bookmarks.Bookmarks().Count());
+        const auto* bookmark = bookmarks.Bookmarks().GetItemAt(0);
+        Expects(bookmark != nullptr);
+        Assert::AreEqual(std::wstring(L"Note description"), bookmark->GetDescription());
+        Assert::AreEqual(1234U, bookmark->GetAddress());
+        Assert::AreEqual(MemSize::EightBit, bookmark->GetSize());
+        Assert::AreEqual((int)MemFormat::Hex, (int)bookmark->GetFormat());
+        Assert::IsFalse(bookmark->IsCustomDescription());
+
+        bookmarks.mockGameContext.SetCodeNote(1234U, L"New description");
+
+        Assert::AreEqual({ 1U }, bookmarks.Bookmarks().Count());
+        bookmark = bookmarks.Bookmarks().GetItemAt(0);
+        Expects(bookmark != nullptr);
+        Assert::AreEqual(std::wstring(L"New description"), bookmark->GetDescription());
+        Assert::AreEqual(1234U, bookmark->GetAddress());
+        Assert::AreEqual(MemSize::EightBit, bookmark->GetSize());
+        Assert::AreEqual((int)MemFormat::Hex, (int)bookmark->GetFormat());
+        Assert::IsFalse(bookmark->IsCustomDescription());
+    }
+
+    TEST_METHOD(TestCodeNoteChangedCustomDescription)
+    {
+        MemoryBookmarksViewModelHarness bookmarks;
+        bookmarks.SetIsVisible(true);
+        bookmarks.mockGameContext.SetGameId(3U);
+        bookmarks.mockGameContext.SetCodeNote(1234U, L"Note description");
+        bookmarks.mockLocalStorage.MockStoredData(ra::services::StorageItemType::Bookmarks, L"3",
+            "{\"Bookmarks\":[{\"Address\":1234,\"Size\":10,\"Description\":\"My Description\"}]}");
+
+        bookmarks.mockGameContext.NotifyActiveGameChanged();
+
+        Assert::AreEqual({ 1U }, bookmarks.Bookmarks().Count());
+        const auto* bookmark = bookmarks.Bookmarks().GetItemAt(0);
+        Expects(bookmark != nullptr);
+        Assert::AreEqual(std::wstring(L"My Description"), bookmark->GetDescription());
+        Assert::AreEqual(1234U, bookmark->GetAddress());
+        Assert::AreEqual(MemSize::EightBit, bookmark->GetSize());
+        Assert::AreEqual((int)MemFormat::Hex, (int)bookmark->GetFormat());
+        Assert::IsTrue(bookmark->IsCustomDescription());
+
+        bookmarks.mockGameContext.SetCodeNote(1234U, L"New description");
+
+        Assert::AreEqual({ 1U }, bookmarks.Bookmarks().Count());
+        bookmark = bookmarks.Bookmarks().GetItemAt(0);
+        Expects(bookmark != nullptr);
+        Assert::AreEqual(std::wstring(L"My Description"), bookmark->GetDescription());
+        Assert::AreEqual(1234U, bookmark->GetAddress());
+        Assert::AreEqual(MemSize::EightBit, bookmark->GetSize());
+        Assert::AreEqual((int)MemFormat::Hex, (int)bookmark->GetFormat());
+        Assert::IsTrue(bookmark->IsCustomDescription());
+
+        bookmarks.mockGameContext.SetCodeNote(1234U, L"My Description");
+
+        Assert::AreEqual({ 1U }, bookmarks.Bookmarks().Count());
+        bookmark = bookmarks.Bookmarks().GetItemAt(0);
+        Expects(bookmark != nullptr);
+        Assert::AreEqual(std::wstring(L"My Description"), bookmark->GetDescription());
+        Assert::AreEqual(1234U, bookmark->GetAddress());
+        Assert::AreEqual(MemSize::EightBit, bookmark->GetSize());
+        Assert::AreEqual((int)MemFormat::Hex, (int)bookmark->GetFormat());
+        Assert::IsFalse(bookmark->IsCustomDescription());
+
+        bookmarks.mockGameContext.SetCodeNote(1234U, L"New description");
+
+        Assert::AreEqual({ 1U }, bookmarks.Bookmarks().Count());
+        bookmark = bookmarks.Bookmarks().GetItemAt(0);
+        Expects(bookmark != nullptr);
+        Assert::AreEqual(std::wstring(L"New description"), bookmark->GetDescription());
+        Assert::AreEqual(1234U, bookmark->GetAddress());
+        Assert::AreEqual(MemSize::EightBit, bookmark->GetSize());
+        Assert::AreEqual((int)MemFormat::Hex, (int)bookmark->GetFormat());
+        Assert::IsFalse(bookmark->IsCustomDescription());
+    }
+
     TEST_METHOD(TestDoFrame)
     {
         MemoryBookmarksViewModelHarness bookmarks;