Repository: RetroAchievements/RAIntegration
Pull request #798 (2021-09-10 16:07:08)

improvements to code note size annotations
If bit and byte both exist in a code note, give preference to byte. Allows for annotating large collections of smaller data: "[400 byte] 100x 32-bit integer"

Differentiate between explicitly specifying "[8-bit]" versus not having an annotation. Fixes a minor issue where adding conditions for an annotated "[8-bit]" address in non-8-bit mode would not create an 8-bit comparison.


Patch:
diff --git a/RAInterface b/RAInterface
index 87dadd78..e5785964 160000
--- a/RAInterface
+++ b/RAInterface
@@ -1,1 +1,1 @@
-Subproject commit 87dadd78b782f723d216f287c5b4400f7ae5a309
+Subproject commit e5785964ad5eee6f50a22a3a29c0588cdd8320b5
diff --git a/src/data/context/GameContext.cpp b/src/data/context/GameContext.cpp
index 7a239004..21c806d5 100644
--- a/src/data/context/GameContext.cpp
+++ b/src/data/context/GameContext.cpp
@@ -844,10 +844,11 @@ void GameContext::RefreshCodeNotes()
 static unsigned int ExtractSize(const std::wstring& sNote)
 {
     bool bIsBytes = false;
+    int nBytesFromBits = 0;
 
     // "Nbit" smallest possible note - and that's just the size annotation
     if (sNote.length() < 4)
-        return 1;
+        return 0;
 
     const size_t nStop = sNote.length() - 3;
     for (size_t nIndex = 1; nIndex <= nStop; ++nIndex)
@@ -859,10 +860,22 @@ static unsigned int ExtractSize(const std::wstring& sNote)
         c = sNote.at(nIndex + 1);
         if (c == L'i' || c == L'I')
         {
+            // already found one bit reference, give it precedence
+            if (nBytesFromBits > 0)
+                continue;
+
             c = sNote.at(nIndex + 2);
             if (c != L't' && c != L'T')
                 continue;
 
+            // match "bits", but not "bite" even if there is a numeric prefix
+            if (nIndex < nStop)
+            {
+                c = sNote.at(nIndex + 3);
+                if (isalpha(c) && c != L's' && c != L'S')
+                    continue;
+            }
+
             // found "bit"
             bIsBytes = false;
         }
@@ -915,22 +928,29 @@ static unsigned int ExtractSize(const std::wstring& sNote)
         // if a number was found, return it
         if (nBytes > 0)
         {
+            // if "bytes" were found, we're done. if bits were found, it might be indicating
+            // the size of individual elements. capture the bit value and keep searching.
             if (bIsBytes)
                 return nBytes;
 
             // convert bits to bytes, rounding up
-            return (nBytes + 7) / 8;
+            nBytesFromBits = (nBytes + 7) / 8;
         }
     }
 
-    // could not find annotation, associate note to single address
-    return 1;
+    // did not find any byte reference, return the bit reference (if found)
+    if (nBytesFromBits > 0)
+        return nBytesFromBits;
+
+    // could not find annotation
+    return 0;
 }
 
 void GameContext::AddCodeNote(ra::ByteAddress nAddress, const std::string& sAuthor, const std::wstring& sNote)
 {
-    const unsigned int nBytes = ExtractSize(sNote);
-    m_mCodeNotes.insert_or_assign(nAddress, CodeNote{ sAuthor, sNote, nBytes });
+    const unsigned int nSize = ExtractSize(sNote);
+    const unsigned int nBytes = (nSize == 0) ? 1 : nSize;
+    m_mCodeNotes.insert_or_assign(nAddress, CodeNote{ sAuthor, sNote, nBytes, nSize });
     OnCodeNoteChanged(nAddress, sNote);
 }
 
diff --git a/src/data/context/GameContext.hh b/src/data/context/GameContext.hh
index 766e16e2..cc1ca8fe 100644
--- a/src/data/context/GameContext.hh
+++ b/src/data/context/GameContext.hh
@@ -202,7 +202,7 @@ public:
     unsigned FindCodeNoteSize(ra::ByteAddress nAddress) const
     {
         const auto pIter = m_mCodeNotes.find(nAddress);
-        return (pIter == m_mCodeNotes.end()) ? 0 : pIter->second.Bytes;
+        return (pIter == m_mCodeNotes.end()) ? 0 : pIter->second.AnnotatedSize;
     }
 
     /// <summary>
@@ -308,6 +308,7 @@ protected:
         std::string Author;
         std::wstring Note;
         unsigned int Bytes;
+        unsigned int AnnotatedSize;
     };
     std::map<ra::ByteAddress, CodeNote> m_mCodeNotes;
 
diff --git a/src/ui/viewmodels/AssetListViewModel.cpp b/src/ui/viewmodels/AssetListViewModel.cpp
index b1cf2b66..eb12846b 100644
--- a/src/ui/viewmodels/AssetListViewModel.cpp
+++ b/src/ui/viewmodels/AssetListViewModel.cpp
@@ -573,6 +573,8 @@ void AssetListViewModel::DoUpdateButtons()
     bool bHasModified = false;
     bool bHasUnpublished = false;
     bool bHasUnofficial = false;
+    bool bHasCore = false;
+    bool bHasLocal = false;
     const bool bHardcore = _RA_HardcoreModeIsActive();
 
     const bool bGameLoaded = (GetGameId() != 0);
@@ -652,9 +654,15 @@ void AssetListViewModel::DoUpdateButtons()
                 {
                     switch (pItem->GetCategory())
                     {
+                        case ra::data::models::AssetCategory::Core:
+                            bHasCore = true;
+                            break;
                         case ra::data::models::AssetCategory::Unofficial:
                             bHasUnofficial = true;
                             break;
+                        case ra::data::models::AssetCategory::Local:
+                            bHasLocal = true;
+                            break;
                         default:
                             break;
                     }
@@ -751,6 +759,8 @@ void AssetListViewModel::DoUpdateButtons()
             SetValue(RevertButtonTextProperty, L"Re&vert");
         else if (bHasLocalSelection)
             SetValue(RevertButtonTextProperty, L"&Delete");
+        else if (bHasLocal && !bHasCore && !bHasUnofficial)
+            SetValue(RevertButtonTextProperty, L"&Delete All");
         else
             SetValue(RevertButtonTextProperty, RevertButtonTextProperty.GetDefaultValue());
 
@@ -1276,9 +1286,9 @@ void AssetListViewModel::RevertSelected()
         vmMessageBox.SetHeader(L"Revert from server?");
 
         if (vAssetsToReset.size() == m_vFilteredAssets.Count())
-            sWarningMessage = L"This will discard all local changes and revert the assets to the last state retrieved from the server.";
+            sWarningMessage = L"This will discard all local work and revert the assets to the last state retrieved from the server.";
         else
-            sWarningMessage = L"This will discard any local changes to the selected assets and revert them to the last state retrieved from the server.";
+            sWarningMessage = L"This will discard any local work for the selected assets and revert them to the last state retrieved from the server.";
 
         if (bLocalAssetSelected)
             sWarningMessage += L"\n\n";
diff --git a/src/ui/viewmodels/TriggerViewModel.cpp b/src/ui/viewmodels/TriggerViewModel.cpp
index e484e3ba..32c07496 100644
--- a/src/ui/viewmodels/TriggerViewModel.cpp
+++ b/src/ui/viewmodels/TriggerViewModel.cpp
@@ -330,6 +330,10 @@ void TriggerViewModel::NewCondition()
             nSize = pMemoryInspector.Viewer().GetSize();
             break;
 
+        case 1:
+            nSize = MemSize::EightBit;
+            break;
+
         case 2:
             nSize = MemSize::SixteenBit;
             break;


Test patch:
diff --git a/tests/data/context/GameContext_Tests.cpp b/tests/data/context/GameContext_Tests.cpp
index 7d7910d2..9b729eae 100644
--- a/tests/data/context/GameContext_Tests.cpp
+++ b/tests/data/context/GameContext_Tests.cpp
@@ -92,15 +92,6 @@ public:
             return pLeaderboard;
         }
 
-        unsigned int GetCodeNoteSize(ra::ByteAddress nAddress)
-        {
-            const auto pIter = m_mCodeNotes.find(nAddress);
-            if (pIter == m_mCodeNotes.end())
-                return 0U;
-
-            return pIter->second.Bytes;
-        }
-
         void RemoveNonAchievementAssets()
         {
             for (gsl::index nIndex = Assets().Count() - 1; nIndex >= 0; nIndex--)
@@ -2292,16 +2283,16 @@ public:
         game.mockThreadPool.ExecuteNextTask(); // FetchUserUnlocks and FetchCodeNotes are async
         game.mockThreadPool.ExecuteNextTask();
 
-        Assert::AreEqual(nExpectedSize, game.GetCodeNoteSize(1234U), sNote.c_str());
+        Assert::AreEqual(nExpectedSize, game.FindCodeNoteSize(1234U), sNote.c_str());
     }
 
     TEST_METHOD(TestLoadCodeNotesSized)
     {
-        TestCodeNoteSize(L"", 1U);
-        TestCodeNoteSize(L"Test", 1U);
+        TestCodeNoteSize(L"", 0U);
+        TestCodeNoteSize(L"Test", 0U);
         TestCodeNoteSize(L"16-bit Test", 2U);
         TestCodeNoteSize(L"Test 16-bit", 2U);
-        TestCodeNoteSize(L"Test 16-bi", 1U);
+        TestCodeNoteSize(L"Test 16-bi", 0U);
         TestCodeNoteSize(L"[16-bit] Test", 2U);
         TestCodeNoteSize(L"[16 bit] Test", 2U);
         TestCodeNoteSize(L"[16 Bit] Test", 2U);
@@ -2316,15 +2307,18 @@ public:
         TestCodeNoteSize(L"[128-bit] Test", 16U);
         TestCodeNoteSize(L"[17-bit] Test", 3U);
         TestCodeNoteSize(L"[100-bit] Test", 13U);
+        TestCodeNoteSize(L"[0-bit] Test", 0U);
+        TestCodeNoteSize(L"[1-bit] Test", 1U);
         TestCodeNoteSize(L"[4-bit] Test", 1U);
-        TestCodeNoteSize(L"[0-bit] Test", 1U);
-        TestCodeNoteSize(L"bit", 1U);
+        TestCodeNoteSize(L"[8-bit] Test", 1U);
+        TestCodeNoteSize(L"[9-bit] Test", 2U);
+        TestCodeNoteSize(L"bit", 0U);
         TestCodeNoteSize(L"9bit", 2U);
-        TestCodeNoteSize(L"-bit", 1U);
+        TestCodeNoteSize(L"-bit", 0U);
 
         TestCodeNoteSize(L"8 BYTE Test", 8U);
         TestCodeNoteSize(L"Test 8 BYTE", 8U);
-        TestCodeNoteSize(L"Test 8 BYT", 1U);
+        TestCodeNoteSize(L"Test 8 BYT", 0U);
         TestCodeNoteSize(L"[2 Byte] Test", 2U);
         TestCodeNoteSize(L"[4 Byte] Test", 4U);
         TestCodeNoteSize(L"[4 Byte - Float] Test", 4U);
@@ -2335,12 +2329,16 @@ public:
         TestCodeNoteSize(L"Test (6 bytes)", 6U);
         TestCodeNoteSize(L"[2byte] Test", 2U);
 
-        TestCodeNoteSize(L"42=bitten", 1U);
-        TestCodeNoteSize(L"bit by bit", 1U);
-        TestCodeNoteSize(L"bit1=chest", 1U);
+        TestCodeNoteSize(L"42=bitten", 0U);
+        TestCodeNoteSize(L"42-bitten", 0U);
+        TestCodeNoteSize(L"bit by bit", 0U);
+        TestCodeNoteSize(L"bit1=chest", 0U);
 
         TestCodeNoteSize(L"Bite count (16-bit)", 2U);
         TestCodeNoteSize(L"Number of bits collected (32 bits)", 4U);
+
+        TestCodeNoteSize(L"100 32-bit pointers [400 bytes]", 400U);
+        TestCodeNoteSize(L"[400 bytes] 100 32-bit pointers", 400U);
     }
 
     TEST_METHOD(TestFindCodeNoteSized)
diff --git a/tests/ui/viewmodels/AssetListViewModel_Tests.cpp b/tests/ui/viewmodels/AssetListViewModel_Tests.cpp
index 42587a55..d1eca8f7 100644
--- a/tests/ui/viewmodels/AssetListViewModel_Tests.cpp
+++ b/tests/ui/viewmodels/AssetListViewModel_Tests.cpp
@@ -125,6 +125,7 @@ private:
         Revert,
         RevertAll,
         Delete,
+        DeleteAll,
         Disabled
     };
 
@@ -284,6 +285,11 @@ private:
                     Assert::IsTrue(CanRevert());
                     break;
 
+                case RevertButtonState::DeleteAll:
+                    Assert::AreEqual(std::wstring(L"&Delete All"), GetValue(RevertButtonTextProperty));
+                    Assert::IsTrue(CanRevert());
+                    break;
+
                 case RevertButtonState::Disabled:
                     Assert::AreEqual(std::wstring(L"Re&vert All"), GetValue(RevertButtonTextProperty));
                     Assert::IsFalse(CanRevert());
@@ -3664,7 +3670,7 @@ public:
         vmAssetList.mockDesktop.ExpectWindow<MessageBoxViewModel>([&bDialogShown](MessageBoxViewModel& vmMessageBox)
         {
             Assert::AreEqual(std::wstring(L"Revert from server?"), vmMessageBox.GetHeader());
-            Assert::AreEqual(std::wstring(L"This will discard all local changes and revert the assets to the last state retrieved from the server."), vmMessageBox.GetMessage());
+            Assert::AreEqual(std::wstring(L"This will discard all local work and revert the assets to the last state retrieved from the server."), vmMessageBox.GetMessage());
 
             bDialogShown = true;
             return DialogResult::Yes;
@@ -3693,7 +3699,7 @@ public:
         vmAssetList.mockDesktop.ExpectWindow<MessageBoxViewModel>([&bDialogShown](MessageBoxViewModel& vmMessageBox)
         {
             Assert::AreEqual(std::wstring(L"Revert from server?"), vmMessageBox.GetHeader());
-            Assert::AreEqual(std::wstring(L"This will discard all local changes and revert the assets to the last state retrieved from the server."), vmMessageBox.GetMessage());
+            Assert::AreEqual(std::wstring(L"This will discard all local work and revert the assets to the last state retrieved from the server."), vmMessageBox.GetMessage());
 
             bDialogShown = true;
             return DialogResult::Yes;
@@ -3723,7 +3729,7 @@ public:
         vmAssetList.mockDesktop.ExpectWindow<MessageBoxViewModel>([&bDialogShown](MessageBoxViewModel& vmMessageBox)
         {
             Assert::AreEqual(std::wstring(L"Revert from server?"), vmMessageBox.GetHeader());
-            Assert::AreEqual(std::wstring(L"This will discard all local changes and revert the assets to the last state retrieved from the server."), vmMessageBox.GetMessage());
+            Assert::AreEqual(std::wstring(L"This will discard all local work and revert the assets to the last state retrieved from the server."), vmMessageBox.GetMessage());
 
             bDialogShown = true;
             return DialogResult::Yes;
@@ -3750,14 +3756,13 @@ public:
         vmAssetList.SetFilterCategory(AssetListViewModel::FilterCategory::Local);
         vmAssetList.ForceUpdateButtons();
 
-        // when nothing is selected, the button defaults to revert all, even if only local achievements are present
-        vmAssetList.AssertButtonState(RevertButtonState::RevertAll);
+        vmAssetList.AssertButtonState(RevertButtonState::DeleteAll);
 
         bool bDialogShown = false;
         vmAssetList.mockDesktop.ExpectWindow<MessageBoxViewModel>([&bDialogShown](MessageBoxViewModel& vmMessageBox)
         {
-            Assert::AreEqual(std::wstring(L"Revert from server?"), vmMessageBox.GetHeader());
-            Assert::AreEqual(std::wstring(L"This will discard all local changes and revert the assets to the last state retrieved from the server.\n\nAssets not available on the server will be permanently deleted."), vmMessageBox.GetMessage());
+            Assert::AreEqual(std::wstring(L"Permanently delete?"), vmMessageBox.GetHeader());
+            Assert::AreEqual(std::wstring(L"Assets not available on the server will be permanently deleted."), vmMessageBox.GetMessage());
 
             bDialogShown = true;
             return DialogResult::Yes;
@@ -3955,6 +3960,38 @@ public:
         Assert::AreEqual(0, pLocalBadges->GetReferenceCount(sBadge2, true));
     }
 
+    TEST_METHOD(TestRevertSelectedCore)
+    {
+        AssetListViewModelHarness vmAssetList;
+        vmAssetList.MockGameId(22U);
+        vmAssetList.AddThreeAchievements();
+
+        bool bDialogShown = false;
+        vmAssetList.mockDesktop.ExpectWindow<MessageBoxViewModel>([&bDialogShown](MessageBoxViewModel& vmMessageBox)
+        {
+            Assert::AreEqual(std::wstring(L"Revert from server?"), vmMessageBox.GetHeader());
+            Assert::AreEqual(std::wstring(L"This will discard any local work for the selected assets and revert them to the last state retrieved from the server."), vmMessageBox.GetMessage());
+
+            bDialogShown = true;
+            return DialogResult::Yes;
+        });
+
+        auto* pAsset = vmAssetList.mockGameContext.Assets().FindAchievement({ 1U });
+        Assert::IsNotNull(pAsset);
+        Ensures(pAsset != nullptr);
+        pAsset->SetName(L"Apple");
+        Assert::AreEqual(AssetChanges::Modified, pAsset->GetChanges());
+
+        vmAssetList.FilteredAssets().GetItemAt(0)->SetSelected(true);
+        vmAssetList.ForceUpdateButtons();
+        vmAssetList.AssertButtonState(RevertButtonState::Revert);
+
+        vmAssetList.RevertSelected();
+
+        Assert::IsTrue(bDialogShown);
+        Assert::AreEqual(AssetChanges::None, pAsset->GetChanges());
+    }
+
     TEST_METHOD(TestRevertSelectedCoreHardcore)
     {
         AssetListViewModelHarness vmAssetList;
@@ -4009,7 +4046,7 @@ public:
         vmAssetList.AddAchievement(AssetCategory::Local, 5, L"Test1", L"Desc1", L"12345", "0xH1234=1");
         vmAssetList.SetFilterCategory(AssetListViewModel::FilterCategory::Local);
         vmAssetList.ForceUpdateButtons();
-        vmAssetList.AssertButtonState(RevertButtonState::RevertAll);
+        vmAssetList.AssertButtonState(RevertButtonState::DeleteAll);
 
         auto* pAsset = vmAssetList.mockGameContext.Assets().FindAchievement({ 1U });
         Assert::IsNotNull(pAsset);
diff --git a/tests/ui/viewmodels/TriggerViewModel_Tests.cpp b/tests/ui/viewmodels/TriggerViewModel_Tests.cpp
index fedefa52..2080532f 100644
--- a/tests/ui/viewmodels/TriggerViewModel_Tests.cpp
+++ b/tests/ui/viewmodels/TriggerViewModel_Tests.cpp
@@ -749,6 +749,37 @@ public:
         Assert::AreEqual(std::string("0xH1234=16_0x 0008=2312_0xX0004=117835012"), vmTrigger.Serialize());
     }
 
+    TEST_METHOD(TestNewConditionCodeNoteSizeViewerSize)
+    {
+        std::array<uint8_t, 10> pMemory = { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9 };
+        TriggerViewModelHarness vmTrigger;
+        Parse(vmTrigger, "0xH1234=16");
+        Assert::AreEqual({ 1U }, vmTrigger.Conditions().Count());
+        vmTrigger.mockGameContext.SetCodeNote({ 8U }, L"[16-bit] test");
+        vmTrigger.mockGameContext.SetCodeNote({ 2U }, L"[8-bit] test");
+        vmTrigger.mockGameContext.SetCodeNote({ 4U }, L"test");
+
+        vmTrigger.InitializeMemory(&pMemory.at(0), pMemory.size());
+        vmTrigger.mockWindowManager.MemoryInspector.Viewer().SetAddress(8);
+        vmTrigger.mockWindowManager.MemoryInspector.Viewer().SetSize(MemSize::ThirtyTwoBit);
+        vmTrigger.NewCondition();
+
+        Assert::AreEqual({ 2U }, vmTrigger.Conditions().Count());
+        Assert::AreEqual(std::string("0xH1234=16_0x 0008=2312"), vmTrigger.Serialize());
+
+        vmTrigger.mockWindowManager.MemoryInspector.Viewer().SetAddress(4);
+        vmTrigger.NewCondition();
+
+        Assert::AreEqual({ 3U }, vmTrigger.Conditions().Count());
+        Assert::AreEqual(std::string("0xH1234=16_0x 0008=2312_0xX0004=117835012"), vmTrigger.Serialize());
+
+        vmTrigger.mockWindowManager.MemoryInspector.Viewer().SetAddress(2);
+        vmTrigger.NewCondition();
+
+        Assert::AreEqual({ 4U }, vmTrigger.Conditions().Count());
+        Assert::AreEqual(std::string("0xH1234=16_0x 0008=2312_0xX0004=117835012_0xH0002=2"), vmTrigger.Serialize());
+    }
+
     TEST_METHOD(TestAddGroupNoAlts)
     {
         TriggerViewModelHarness vmTrigger;