Repository: RetroAchievements/RAIntegration
Pull request #865 (2022-02-03 21:09:53)

include LowerIsBetter in serialized leaderboard data
fixes "lower is better" not being remembered for local leaderboards between sessions


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/data/models/LeaderboardModel.cpp b/src/data/models/LeaderboardModel.cpp
index 7a1fea7a..c2cc893d 100644
--- a/src/data/models/LeaderboardModel.cpp
+++ b/src/data/models/LeaderboardModel.cpp
@@ -258,6 +258,7 @@ void LeaderboardModel::Serialize(ra::services::TextWriter& pWriter) const
 
     WritePossiblyQuoted(pWriter, GetLocalValue(NameProperty));
     WritePossiblyQuoted(pWriter, GetLocalValue(DescriptionProperty));
+    WriteNumber(pWriter, IsLowerBetter() ? 1 : 0);
 }
 
 bool LeaderboardModel::Deserialize(ra::Tokenizer& pTokenizer)
@@ -297,6 +298,11 @@ bool LeaderboardModel::Deserialize(ra::Tokenizer& pTokenizer)
     if (!pTokenizer.EndOfString() && !ReadPossiblyQuoted(pTokenizer, sDescription))
         return false;
 
+    // field 9: lower is better
+    uint32_t nLowerIsBetter = 0;
+    if (!pTokenizer.EndOfString() && !ReadNumber(pTokenizer, nLowerIsBetter))
+        return false;
+
     // line is valid
     SetName(ra::Widen(sTitle));
     SetDescription(ra::Widen(sDescription));
@@ -304,6 +310,7 @@ bool LeaderboardModel::Deserialize(ra::Tokenizer& pTokenizer)
     SetSubmitTrigger(sSubmitTrigger);
     SetCancelTrigger(sCancelTrigger);
     SetValueDefinition(sValueDefinition);
+    SetLowerIsBetter(nLowerIsBetter != 0);
 
     if (sFormat == "VALUE")
         SetValueFormat(ValueFormat::Value);
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/data/context/GameAssets_Tests.cpp b/tests/data/context/GameAssets_Tests.cpp
index 554d0057..1f022d96 100644
--- a/tests/data/context/GameAssets_Tests.cpp
+++ b/tests/data/context/GameAssets_Tests.cpp
@@ -622,7 +622,7 @@ public:
 
         gameAssets.SaveAllAssets();
 
-        const auto& sExpected = ra::StringPrintf("0.0.0.0\nGameName\nL%u:\"0xH2234=1\":\"0xH2234=2\":\"0xH2234=3\":\"M:0xH2235\":MINUTES:LB2:Desc2\n", GameAssets::FirstLocalId + 1);
+        const auto& sExpected = ra::StringPrintf("0.0.0.0\nGameName\nL%u:\"0xH2234=1\":\"0xH2234=2\":\"0xH2234=3\":\"M:0xH2235\":MINUTES:LB2:Desc2:0\n", GameAssets::FirstLocalId + 1);
         Assert::AreEqual(sExpected, gameAssets.GetUserFile());
     }
 
diff --git a/tests/data/models/LeaderboardModel_Tests.cpp b/tests/data/models/LeaderboardModel_Tests.cpp
index 9fcc315b..bedc77b8 100644
--- a/tests/data/models/LeaderboardModel_Tests.cpp
+++ b/tests/data/models/LeaderboardModel_Tests.cpp
@@ -196,6 +196,46 @@ public:
     {
         LeaderboardModelHarness leaderboard;
 
+        const std::string sSerialized = ":\"0xH1234=1\":\"0xH1234=2\":\"0xH1234=3\":\"0xH1234\":FRAMES:Title:Desc:0";
+        ra::Tokenizer pTokenizer(sSerialized);
+        pTokenizer.Consume(':');
+
+        Assert::IsTrue(leaderboard.Deserialize(pTokenizer));
+
+        Assert::AreEqual(std::string("0xH1234=1"), leaderboard.GetStartTrigger());
+        Assert::AreEqual(std::string("0xH1234=2"), leaderboard.GetCancelTrigger());
+        Assert::AreEqual(std::string("0xH1234=3"), leaderboard.GetSubmitTrigger());
+        Assert::AreEqual(std::string("0xH1234"), leaderboard.GetValueDefinition());
+        Assert::AreEqual(ValueFormat::Frames, leaderboard.GetValueFormat());
+        Assert::AreEqual(std::wstring(L"Title"), leaderboard.GetName());
+        Assert::AreEqual(std::wstring(L"Desc"), leaderboard.GetDescription());
+        Assert::IsFalse(leaderboard.IsLowerBetter());
+    }
+
+    TEST_METHOD(TestDeserializeAlternateValues)
+    {
+        LeaderboardModelHarness leaderboard;
+
+        const std::string sSerialized = ":\"0xH1234=1_P:0xH2345=2\":\"0xH1234<2\":\"0xH1234>3\":\"M:0xH1234\":MILLISECS:\"My Title\":\"My Desc\":1";
+        ra::Tokenizer pTokenizer(sSerialized);
+        pTokenizer.Consume(':');
+
+        Assert::IsTrue(leaderboard.Deserialize(pTokenizer));
+
+        Assert::AreEqual(std::string("0xH1234=1_P:0xH2345=2"), leaderboard.GetStartTrigger());
+        Assert::AreEqual(std::string("0xH1234<2"), leaderboard.GetCancelTrigger());
+        Assert::AreEqual(std::string("0xH1234>3"), leaderboard.GetSubmitTrigger());
+        Assert::AreEqual(std::string("M:0xH1234"), leaderboard.GetValueDefinition());
+        Assert::AreEqual(ValueFormat::Centiseconds, leaderboard.GetValueFormat());
+        Assert::AreEqual(std::wstring(L"My Title"), leaderboard.GetName());
+        Assert::AreEqual(std::wstring(L"My Desc"), leaderboard.GetDescription());
+        Assert::IsTrue(leaderboard.IsLowerBetter());
+    }
+
+    TEST_METHOD(TestDeserializeNoLower)
+    {
+        LeaderboardModelHarness leaderboard;
+
         const std::string sSerialized = ":\"0xH1234=1\":\"0xH1234=2\":\"0xH1234=3\":\"0xH1234\":FRAMES:Title:Desc";
         ra::Tokenizer pTokenizer(sSerialized);
         pTokenizer.Consume(':');
@@ -209,6 +249,7 @@ public:
         Assert::AreEqual(ValueFormat::Frames, leaderboard.GetValueFormat());
         Assert::AreEqual(std::wstring(L"Title"), leaderboard.GetName());
         Assert::AreEqual(std::wstring(L"Desc"), leaderboard.GetDescription());
+        Assert::IsFalse(leaderboard.IsLowerBetter());
     }
 
     TEST_METHOD(TestDeserializeEmpty)
@@ -228,6 +269,7 @@ public:
         Assert::AreEqual(ValueFormat::Value, leaderboard.GetValueFormat());
         Assert::AreEqual(std::wstring(), leaderboard.GetName());
         Assert::AreEqual(std::wstring(), leaderboard.GetDescription());
+        Assert::IsFalse(leaderboard.IsLowerBetter());
     }
 
     void TestSerializeValueFormat(ValueFormat nFormat, const std::string& sFormat)
@@ -245,7 +287,7 @@ public:
         ra::services::impl::StringTextWriter pTextWriter(sSerialized);
         leaderboard.Serialize(pTextWriter);
 
-        std::string sExpected = ":\"0xH1234=1\":\"0xH1234=2\":\"0xH1234=3\":\"0xH1234\":" + sFormat + ":Title:Desc";
+        std::string sExpected = ":\"0xH1234=1\":\"0xH1234=2\":\"0xH1234=3\":\"0xH1234\":" + sFormat + ":Title:Desc:0";
         Assert::AreEqual(sExpected, sSerialized);
 
         ra::Tokenizer pTokenizer(sSerialized);
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;