Repository: RetroAchievements/RAIntegration
Pull request #809 (2021-09-26 00:51:29)

fix disappearing results for singular text match
Fixes an issue where filtering text search results would sometimes cause a portion of the list to disappear.

If a matching block (chunk of memory) contained only one match, the adjustment to include the entire string was discarding the front of the string, so the lookup for the string was failing, and all entries after the failure were left uninitialized.

Also changes any values outside the ASCII range to the unicode replacement character.


Patch:
diff --git a/rcheevos b/rcheevos
index ee0ab901..d8431e92 160000
--- a/rcheevos
+++ b/rcheevos
@@ -1,1 +1,1 @@
-Subproject commit ee0ab901bbb5e93490e2c7587798eeacb15b986f
+Subproject commit d8431e92ea5305d244c0afccb0d69504893d6133
diff --git a/src/data/models/TriggerValidation.cpp b/src/data/models/TriggerValidation.cpp
index 6e2fb192..03d2cda1 100644
--- a/src/data/models/TriggerValidation.cpp
+++ b/src/data/models/TriggerValidation.cpp
@@ -8,25 +8,143 @@ namespace ra {
 namespace data {
 namespace models {
 
+MemSize TriggerValidation::MapRcheevosMemSize(char nSize) noexcept
+{
+    switch (nSize)
+    {
+        case RC_MEMSIZE_BIT_0: return MemSize::Bit_0;
+        case RC_MEMSIZE_BIT_1: return MemSize::Bit_1;
+        case RC_MEMSIZE_BIT_2: return MemSize::Bit_2;
+        case RC_MEMSIZE_BIT_3: return MemSize::Bit_3;
+        case RC_MEMSIZE_BIT_4: return MemSize::Bit_4;
+        case RC_MEMSIZE_BIT_5: return MemSize::Bit_5;
+        case RC_MEMSIZE_BIT_6: return MemSize::Bit_6;
+        case RC_MEMSIZE_BIT_7: return MemSize::Bit_7;
+        case RC_MEMSIZE_LOW: return MemSize::Nibble_Lower;
+        case RC_MEMSIZE_HIGH: return MemSize::Nibble_Upper;
+        case RC_MEMSIZE_8_BITS: return MemSize::EightBit;
+        case RC_MEMSIZE_16_BITS: return MemSize::SixteenBit;
+        case RC_MEMSIZE_24_BITS: return MemSize::TwentyFourBit;
+        case RC_MEMSIZE_32_BITS: return MemSize::ThirtyTwoBit;
+        case RC_MEMSIZE_BITCOUNT: return MemSize::BitCount;
+        case RC_MEMSIZE_16_BITS_BE: return MemSize::SixteenBitBigEndian;
+        case RC_MEMSIZE_24_BITS_BE: return MemSize::TwentyFourBitBigEndian;
+        case RC_MEMSIZE_32_BITS_BE: return MemSize::ThirtyTwoBitBigEndian;
+        default:
+            assert(!"Unsupported operand size");
+            return MemSize::EightBit;
+    }
+}
+
+static unsigned MaxValue(const rc_operand_t* pOperand)
+{
+    if (pOperand->type == RC_OPERAND_CONST)
+        return pOperand->value.num;
+
+    if (!rc_operand_is_memref(pOperand))
+        return 0xFFFFFFFF;
+
+    return MemSizeMax(TriggerValidation::MapRcheevosMemSize(pOperand->size));
+}
+
+static int ValidateRange(unsigned nMinValue, unsigned nMaxValue, char nOperator, unsigned nMax, std::wstring& sError) {
+    switch (nOperator) {
+        case RC_OPERATOR_AND:
+            if (nMinValue > nMax) {
+                sError = L"Mask has more bits than source";
+                return false;
+            }
+            else if (nMinValue == 0 && nMaxValue == 0) {
+                sError = L"Result of mask is always 0";
+                return false;
+            }
+            break;
+
+        case RC_OPERATOR_EQ:
+            if (nMinValue > nMax) {
+                sError = L"Comparison is never true";
+                return false;
+            }
+            break;
+
+        case RC_OPERATOR_NE:
+            if (nMinValue > nMax) {
+                sError = L"Comparison is always true";
+                return false;
+            }
+            break;
+
+        case RC_OPERATOR_GE:
+            if (nMinValue > nMax) {
+                sError = L"Comparison is never true";
+                return false;
+            }
+            if (nMaxValue == 0) {
+                sError = L"Comparison is always true";
+                return false;
+            }
+            break;
+
+        case RC_OPERATOR_GT:
+            if (nMinValue >= nMax) {
+                sError = L"Comparison is never true";
+                return false;
+            }
+            break;
+
+        case RC_OPERATOR_LE:
+            if (nMinValue >= nMax) {
+                sError = L"Comparison is always true";
+                return false;
+            }
+            break;
+
+        case RC_OPERATOR_LT:
+            if (nMinValue > nMax) {
+                sError = L"Comparison is always true";
+                return false;
+            }
+            if (nMaxValue == 0) {
+                sError = L"Comparison is never true";
+                return false;
+            }
+            break;
+    }
+
+    return true;
+}
+
 static bool ValidateCondSet(const rc_condset_t* pCondSet, std::wstring& sError)
 {
     bool bInAddHits = false;
     bool bIsCombining = false;
+    unsigned long long nAddSourceMax = 0;
 
     int nIndex = 1;
     const rc_condition_t* pCondition = pCondSet->conditions;
-    while (pCondition != nullptr)
+    for (; pCondition != nullptr; ++nIndex, pCondition = pCondition->next)
     {
+        unsigned nMax = MaxValue(&pCondition->operand1); // maximum possible calculated value for comparison
+
         switch (pCondition->type)
         {
+            case RC_CONDITION_ADD_SOURCE:
+                nAddSourceMax += nMax;
+                bIsCombining = true;
+                continue;
+
+            case RC_CONDITION_SUB_SOURCE:
+                if (nAddSourceMax < nMax) // ignore potential underflow - may be expected
+                    nAddSourceMax = 0xFFFFFFFF;
+                bIsCombining = true;
+                continue;
+
             case RC_CONDITION_ADD_HITS:
             case RC_CONDITION_SUB_HITS:
                 bIsCombining = true;
                 bInAddHits = true;
                 break;
 
-            case RC_CONDITION_ADD_SOURCE:
-            case RC_CONDITION_SUB_SOURCE:
             case RC_CONDITION_ADD_ADDRESS:
             case RC_CONDITION_AND_NEXT:
             case RC_CONDITION_OR_NEXT:
@@ -50,8 +168,43 @@ static bool ValidateCondSet(const rc_condset_t* pCondSet, std::wstring& sError)
                 break;
         }
 
-        ++nIndex;
-        pCondition = pCondition->next;
+        if (nAddSourceMax)
+        {
+            const unsigned long long overflow = nAddSourceMax + nMax;
+            if (overflow > 0xFFFFFFFFUL)
+                nMax = 0xFFFFFFFF;
+            else
+                nMax = gsl::narrow_cast<unsigned>(overflow);
+        }
+
+        // maximum comparison value
+        const unsigned nMaxValue = MaxValue(&pCondition->operand2);
+
+        // minimum comparison value
+        const unsigned nMinValue = (pCondition->operand2.type == RC_OPERAND_CONST) ? pCondition->operand2.value.num : 0;
+
+        // if comparing two memrefs outside of an AddSource chain, make sure their sizes match
+        const bool bIsMemRef = rc_operand_is_memref(&pCondition->operand1);
+        if (bIsMemRef && nMaxValue != nMax && nAddSourceMax == 0 && rc_operand_is_memref(&pCondition->operand2))
+        {
+            sError = ra::StringPrintf(L"Condition %u: Comparing different memory sizes", nIndex);
+            return false;
+        }
+
+        // if nMax is from a memory reference or AddSource chain, or the right side is a memory reference,
+        // validate that nMax can fall between bMinValue and nMaxValue
+        if (bIsMemRef || nMinValue == 0 || nAddSourceMax)
+        {
+            if (!ValidateRange(nMinValue, nMaxValue, pCondition->oper, nMax, sError))
+            {
+                sError.insert(0, L"Condition : ");
+                sError.insert(10, std::to_wstring(nIndex));
+                return false;
+            }
+        }
+
+        // reset AddSource chain
+        nAddSourceMax = 0;
     }
 
     if (bIsCombining)
diff --git a/src/data/models/TriggerValidation.hh b/src/data/models/TriggerValidation.hh
index 677719c8..6b2f9b45 100644
--- a/src/data/models/TriggerValidation.hh
+++ b/src/data/models/TriggerValidation.hh
@@ -2,6 +2,8 @@
 #define RA_DATA_TRIGGER_VALIDATION_H
 #pragma once
 
+#include "data\Types.hh"
+
 namespace ra {
 namespace data {
 namespace models {
@@ -10,6 +12,7 @@ class TriggerValidation
 {
 public:
     static bool Validate(const std::string& sTrigger, std::wstring& sError);
+    static MemSize MapRcheevosMemSize(char nSize) noexcept;
 };
 
 } // namespace models
diff --git a/src/services/GameIdentifier.cpp b/src/services/GameIdentifier.cpp
index 3e20aebc..12625c6f 100644
--- a/src/services/GameIdentifier.cpp
+++ b/src/services/GameIdentifier.cpp
@@ -1,7 +1,6 @@
 #include "GameIdentifier.hh"
 
 #include "RA_Log.h"
-#include "RA_md5factory.h"
 #include "RA_StringUtils.h"
 
 #include <rc_hash.h>
@@ -39,7 +38,7 @@ unsigned int GameIdentifier::IdentifyGame(const BYTE* pROM, size_t nROMSize)
 
     if (pROM == nullptr || nROMSize == 0)
     {
-        m_sPendingMD5.clear();
+        m_sPendingHash.clear();
         m_nPendingGameId = 0U;
         return 0U;
     }
@@ -50,7 +49,7 @@ unsigned int GameIdentifier::IdentifyGame(const BYTE* pROM, size_t nROMSize)
     return IdentifyHash(hash);
 }
 
-unsigned int GameIdentifier::IdentifyHash(const std::string& sMD5)
+unsigned int GameIdentifier::IdentifyHash(const std::string& sHash)
 {
     if (!ra::services::ServiceLocator::Get<ra::data::context::UserContext>().IsLoggedIn())
     {
@@ -62,53 +61,63 @@ unsigned int GameIdentifier::IdentifyHash(const std::string& sMD5)
     unsigned int nGameId = 0U;
     m_nPendingMode = ra::data::context::GameContext::Mode::Normal;
 
-    ra::api::ResolveHash::Request request;
-    request.Hash = sMD5;
-
-    const auto response = request.Call();
-    if (response.Succeeded())
+    const auto pIter = m_mKnownHashes.find(sHash);
+    if (pIter != m_mKnownHashes.end())
     {
-        nGameId = response.GameId;
-        if (nGameId == 0) // Unknown
+        RA_LOG_INFO("Using previously looked up game ID %u for hash %s", nGameId, sHash);
+        nGameId = pIter->second;
+    }
+    else
+    {
+        ra::api::ResolveHash::Request request;
+        request.Hash = sHash;
+
+        const auto response = request.Call();
+        if (response.Succeeded())
         {
-            RA_LOG_INFO("Could not identify game with hash %s", sMD5);
+            nGameId = response.GameId;
+            if (nGameId == 0) // Unknown
+            {
+                RA_LOG_INFO("Could not identify game with hash %s", sHash);
 
-            auto sEstimatedGameTitle = ra::services::ServiceLocator::Get<ra::data::context::EmulatorContext>().GetGameTitle();
+                auto sEstimatedGameTitle = ra::services::ServiceLocator::Get<ra::data::context::EmulatorContext>().GetGameTitle();
 
-            ra::ui::viewmodels::UnknownGameViewModel vmUnknownGame;
-            vmUnknownGame.InitializeGameTitles();
-            vmUnknownGame.SetSystemName(ra::services::ServiceLocator::Get<ra::data::context::ConsoleContext>().Name());
-            vmUnknownGame.SetChecksum(ra::Widen(sMD5));
-            vmUnknownGame.SetEstimatedGameName(ra::Widen(sEstimatedGameTitle));
-            vmUnknownGame.SetNewGameName(vmUnknownGame.GetEstimatedGameName());
+                ra::ui::viewmodels::UnknownGameViewModel vmUnknownGame;
+                vmUnknownGame.InitializeGameTitles();
+                vmUnknownGame.SetSystemName(ra::services::ServiceLocator::Get<ra::data::context::ConsoleContext>().Name());
+                vmUnknownGame.SetChecksum(ra::Widen(sHash));
+                vmUnknownGame.SetEstimatedGameName(ra::Widen(sEstimatedGameTitle));
+                vmUnknownGame.SetNewGameName(vmUnknownGame.GetEstimatedGameName());
 
-            if (vmUnknownGame.ShowModal() == ra::ui::DialogResult::OK)
-            {
-                nGameId = vmUnknownGame.GetSelectedGameId();
+                if (vmUnknownGame.ShowModal() == ra::ui::DialogResult::OK)
+                {
+                    nGameId = vmUnknownGame.GetSelectedGameId();
 
-                if (vmUnknownGame.GetTestMode())
-                    m_nPendingMode = ra::data::context::GameContext::Mode::CompatibilityTest;
+                    if (vmUnknownGame.GetTestMode())
+                        m_nPendingMode = ra::data::context::GameContext::Mode::CompatibilityTest;
+                }
+            }
+            else
+            {
+                RA_LOG_INFO("Successfully looked up game with ID %u", nGameId);
+                m_mKnownHashes.insert_or_assign(sHash, nGameId);
             }
         }
         else
         {
-            RA_LOG_INFO("Successfully looked up game with ID %u", nGameId);
-        }
-    }
-    else
-    {
-        std::wstring sErrorMessage = ra::Widen(response.ErrorMessage);
-        if (sErrorMessage.empty())
-        {
-            auto& pConfiguration = ra::services::ServiceLocator::Get<ra::services::IConfiguration>();
-            sErrorMessage = ra::StringPrintf(L"Error from %s", pConfiguration.GetHostName());
-        }
+            std::wstring sErrorMessage = ra::Widen(response.ErrorMessage);
+            if (sErrorMessage.empty())
+            {
+                auto& pConfiguration = ra::services::ServiceLocator::Get<ra::services::IConfiguration>();
+                sErrorMessage = ra::StringPrintf(L"Error from %s", pConfiguration.GetHostName());
+            }
 
-        ra::ui::viewmodels::MessageBoxViewModel::ShowErrorMessage(L"Could not identify game.", sErrorMessage);
+            ra::ui::viewmodels::MessageBoxViewModel::ShowErrorMessage(L"Could not identify game.", sErrorMessage);
+        }
     }
 
     // store the hash and game id - will be used by _RA_ActivateGame (if called)
-    m_sPendingMD5 = sMD5;
+    m_sPendingHash = sHash;
     m_nPendingGameId = nGameId;
 
     auto& pGameContext = ra::services::ServiceLocator::GetMutable<ra::data::context::GameContext>();
@@ -116,7 +125,7 @@ unsigned int GameIdentifier::IdentifyHash(const std::string& sMD5)
     {
         // same as currently loaded rom. assume user is switching disks and _RA_ActivateGame won't be called.
         // update the hash now. if it does get called, this is redundant.
-        pGameContext.SetGameHash(sMD5);
+        pGameContext.SetGameHash(sHash);
         pGameContext.SetMode(m_nPendingMode);
     }
 
@@ -141,7 +150,7 @@ void GameIdentifier::ActivateGame(unsigned int nGameId)
 
         auto& pGameContext = ra::services::ServiceLocator::GetMutable<ra::data::context::GameContext>();
         pGameContext.LoadGame(nGameId, m_nPendingMode);
-        pGameContext.SetGameHash((nGameId == m_nPendingGameId) ? m_sPendingMD5 : "");
+        pGameContext.SetGameHash((nGameId == m_nPendingGameId) ? m_sPendingHash : "");
 
         ra::services::ServiceLocator::GetMutable<ra::data::context::SessionTracker>().BeginSession(nGameId);
 
@@ -182,7 +191,7 @@ void GameIdentifier::ActivateGame(unsigned int nGameId)
 
         auto& pGameContext = ra::services::ServiceLocator::GetMutable<ra::data::context::GameContext>();
         pGameContext.LoadGame(0U, m_nPendingMode);
-        pGameContext.SetGameHash((m_nPendingGameId == 0) ? m_sPendingMD5 : "");
+        pGameContext.SetGameHash((m_nPendingGameId == 0) ? m_sPendingHash : "");
     }
 
     ra::services::ServiceLocator::GetMutable<ra::data::context::EmulatorContext>().ResetMemoryModified();
diff --git a/src/services/GameIdentifier.hh b/src/services/GameIdentifier.hh
index 51039757..6f009554 100644
--- a/src/services/GameIdentifier.hh
+++ b/src/services/GameIdentifier.hh
@@ -27,8 +27,8 @@ public:
     /// <summary>
     /// Identifies a game.
     /// </summary>
-    /// <param name="sMD5">The unique identifier of the game.</param>
-    unsigned int IdentifyHash(const std::string& sMD5);
+    /// <param name="sHash">The unique identifier of the game.</param>
+    unsigned int IdentifyHash(const std::string& sHash);
 
     /// <summary>
     /// Activates a game.
@@ -37,9 +37,10 @@ public:
     void ActivateGame(unsigned int nGameId);
 
 private:
-    std::string m_sPendingMD5;
+    std::string m_sPendingHash;
     unsigned int m_nPendingGameId{};
     ra::data::context::GameContext::Mode m_nPendingMode{};
+    std::map<std::string, unsigned> m_mKnownHashes;
 };
 
 } // namespace services
diff --git a/src/services/SearchResults.cpp b/src/services/SearchResults.cpp
index 45620638..f2cd8d3d 100644
--- a/src/services/SearchResults.cpp
+++ b/src/services/SearchResults.cpp
@@ -500,12 +500,20 @@ public:
     // populates a vector of addresses that match the specified filter when applied to a previous search result
     void ApplyFilter(SearchResults& srNew, const SearchResults& srPrevious) const override
     {
+        size_t nCompareLength = 16; // maximum length for generic search
         std::vector<unsigned char> vSearchText;
         GetSearchText(vSearchText, srNew.GetFilterString());
 
-        // can't match 0 characters!
         if (vSearchText.empty())
-            return;
+        {
+            // can't match 0 characters!
+            if (srNew.GetFilterType() == SearchFilterType::Constant)
+                return;
+        }
+        else
+        {
+            nCompareLength = vSearchText.size();
+        }
 
         unsigned int nLargestBlock = 0U;
         for (auto& block : GetBlocks(srPrevious))
@@ -530,7 +538,7 @@ public:
                 case SearchFilterType::Constant:
                     for (unsigned int i = 0; i < nStop; ++i)
                     {
-                        if (CompareMemory(&vMemory.at(i), &vSearchText.at(0), vSearchText.size(), srNew.GetFilterComparison()))
+                        if (CompareMemory(&vMemory.at(i), &vSearchText.at(0), nCompareLength, srNew.GetFilterComparison()))
                         {
                             const unsigned int nAddress = block.GetAddress() + i;
                             AddMatch(vMatches, srPrevious, nAddress);
@@ -542,7 +550,7 @@ public:
                 case SearchFilterType::LastKnownValue:
                     for (unsigned int i = 0; i < nStop; ++i)
                     {
-                        if (CompareMemory(&vMemory.at(i), block.GetBytes() + i, vSearchText.size(), srNew.GetFilterComparison()))
+                        if (CompareMemory(&vMemory.at(i), block.GetBytes() + i, nCompareLength, srNew.GetFilterComparison()))
                         {
                             const unsigned int nAddress = block.GetAddress() + i;
                             AddMatch(vMatches, srPrevious, nAddress);
@@ -556,10 +564,12 @@ public:
                 auto& vMatchingAddresses = GetMatchingAddresses(srNew);
                 vMatchingAddresses.insert(vMatchingAddresses.end(), vMatches.begin(), vMatches.end());
 
-                // adjust the last entry to account for the length of the string to ensure
+                // adjust the block size to account for the length of the string to ensure
                 // the block contains the whole string
-                vMatches.back() += gsl::narrow_cast<ra::ByteAddress>(vSearchText.size() - 1);
-                AddBlock(srNew, vMatches, vMemory, block.GetAddress());
+                // (duplicates the AddBlock function, but replaces "GetPadding()-1" with "nCompareLength")
+                const size_t nBlockSize = vMatches.back() - vMatches.front() + nCompareLength;
+                MemBlock& newBlock = AddBlock(srNew, vMatches.front(), gsl::narrow_cast<unsigned int>(nBlockSize));
+                memcpy(newBlock.GetBytes(), &vMemory.at(vMatches.front() - block.GetAddress()), nBlockSize);
 
                 vMatches.clear();
             }
diff --git a/src/ui/viewmodels/MemoryBookmarksViewModel.cpp b/src/ui/viewmodels/MemoryBookmarksViewModel.cpp
index 2e57cec4..e26263d8 100644
--- a/src/ui/viewmodels/MemoryBookmarksViewModel.cpp
+++ b/src/ui/viewmodels/MemoryBookmarksViewModel.cpp
@@ -5,6 +5,7 @@
 #include "RA_StringUtils.h"
 
 #include "data\context\EmulatorContext.hh"
+#include "data\models\TriggerValidation.hh"
 
 #include "services\FrameEventQueue.hh"
 #include "services\IConfiguration.hh"
@@ -246,7 +247,7 @@ void MemoryBookmarksViewModel::LoadBookmarks(ra::services::TextReader& sBookmark
                     if (rc_parse_memref(&memaddr, &size, &address) == RC_OK)
                     {
                         vmBookmark->SetAddress(address);
-                        vmBookmark->SetSize(TriggerConditionViewModel::MapRcheevosMemSize(size));
+                        vmBookmark->SetSize(ra::data::models::TriggerValidation::MapRcheevosMemSize(size));
                     }
                 }
                 else
diff --git a/src/ui/viewmodels/MemorySearchViewModel.cpp b/src/ui/viewmodels/MemorySearchViewModel.cpp
index cf73d2f0..4f00355d 100644
--- a/src/ui/viewmodels/MemorySearchViewModel.cpp
+++ b/src/ui/viewmodels/MemorySearchViewModel.cpp
@@ -824,6 +824,22 @@ void MemorySearchViewModel::UpdateResults()
     m_vResults.AddNotifyTarget(*this);
 }
 
+static void GetASCIIText(std::wstring& sText, const unsigned char* pBytes, size_t nBytes)
+{
+    sText.clear();
+    for (size_t i = 0; i < nBytes; ++i)
+    {
+        wchar_t c = gsl::narrow_cast<wchar_t>(*pBytes++);
+        if (c == 0)
+            break;
+
+        if (c < 0x20 || c > 0x7E)
+            c = L'\xFFFD';
+
+        sText.push_back(c);
+    }
+}
+
 void MemorySearchViewModel::UpdateResult(SearchResultViewModel& pRow, ra::services::SearchResults::Result& pResult,
     const ra::data::context::EmulatorContext& pEmulatorContext, bool bCapturePrevious)
 {
@@ -838,20 +854,16 @@ void MemorySearchViewModel::UpdateResult(SearchResultViewModel& pRow, ra::servic
             std::array<unsigned char, 16> pBuffer;
             pEmulatorContext.ReadMemory(pResult.nAddress, &pBuffer.at(0), pBuffer.size());
 
-            std::string sText;
-            for (size_t i = 0; i < pBuffer.size() && pBuffer.at(i); ++i)
-                sText.push_back(static_cast<char>(pBuffer.at(i)));
+            std::wstring sText;
+            GetASCIIText(sText, pBuffer.data(), pBuffer.size());
 
-            pRow.SetCurrentValue(ra::Widen(sText));
+            pRow.SetCurrentValue(sText);
 
             if (bCapturePrevious)
             {
                 pCompareResults.GetBytes(pResult.nAddress, &pBuffer.at(0), pBuffer.size());
-                sText.clear();
-                for (size_t i = 0; i < pBuffer.size() && pBuffer.at(i); ++i)
-                    sText.push_back(static_cast<char>(pBuffer.at(i)));
-
-                pRow.SetPreviousValue(ra::Widen(sText));
+                GetASCIIText(sText, pBuffer.data(), pBuffer.size());
+                pRow.SetPreviousValue(sText);
             }
             else if (!pRow.bHasBeenModified && pRow.GetCurrentValue() != pRow.GetPreviousValue())
             {
diff --git a/src/ui/viewmodels/TriggerConditionViewModel.cpp b/src/ui/viewmodels/TriggerConditionViewModel.cpp
index 769dce3f..69f88b7a 100644
--- a/src/ui/viewmodels/TriggerConditionViewModel.cpp
+++ b/src/ui/viewmodels/TriggerConditionViewModel.cpp
@@ -4,6 +4,7 @@
 #include "RA_StringUtils.h"
 
 #include "data\context\GameContext.hh"
+#include "data\models\TriggerValidation.hh"
 
 #include "services\AchievementRuntime.hh"
 #include "services\IConfiguration.hh"
@@ -202,40 +203,6 @@ void TriggerConditionViewModel::SerializeAppendOperand(std::string& sBuffer, Tri
     sBuffer.append(ra::ByteAddressToString(nValue), 2);
 }
 
-static constexpr MemSize MapMemSize(char nSize) noexcept
-{
-    switch (nSize)
-    {
-        case RC_MEMSIZE_BIT_0: return MemSize::Bit_0;
-        case RC_MEMSIZE_BIT_1: return MemSize::Bit_1;
-        case RC_MEMSIZE_BIT_2: return MemSize::Bit_2;
-        case RC_MEMSIZE_BIT_3: return MemSize::Bit_3;
-        case RC_MEMSIZE_BIT_4: return MemSize::Bit_4;
-        case RC_MEMSIZE_BIT_5: return MemSize::Bit_5;
-        case RC_MEMSIZE_BIT_6: return MemSize::Bit_6;
-        case RC_MEMSIZE_BIT_7: return MemSize::Bit_7;
-        case RC_MEMSIZE_LOW: return MemSize::Nibble_Lower;
-        case RC_MEMSIZE_HIGH: return MemSize::Nibble_Upper;
-        case RC_MEMSIZE_8_BITS: return MemSize::EightBit;
-        case RC_MEMSIZE_16_BITS: return MemSize::SixteenBit;
-        case RC_MEMSIZE_24_BITS: return MemSize::TwentyFourBit;
-        case RC_MEMSIZE_32_BITS: return MemSize::ThirtyTwoBit;
-        case RC_MEMSIZE_BITCOUNT: return MemSize::BitCount;
-        case RC_MEMSIZE_16_BITS_BE: return MemSize::SixteenBitBigEndian;
-        case RC_MEMSIZE_24_BITS_BE: return MemSize::TwentyFourBitBigEndian;
-        case RC_MEMSIZE_32_BITS_BE: return MemSize::ThirtyTwoBitBigEndian;
-        default:
-            assert(!"Unsupported operand size");
-            return MemSize::EightBit;
-    }
-}
-
-GSL_SUPPRESS_F4
-MemSize TriggerConditionViewModel::MapRcheevosMemSize(char nSize) noexcept
-{
-    return MapMemSize(nSize);
-}
-
 void TriggerConditionViewModel::SetOperand(const IntModelProperty& pTypeProperty,
     const IntModelProperty& pSizeProperty, const IntModelProperty& pValueProperty, const rc_operand_t& operand)
 {
@@ -254,7 +221,7 @@ void TriggerConditionViewModel::SetOperand(const IntModelProperty& pTypeProperty
         case TriggerOperandType::Prior:
         case TriggerOperandType::BCD:
         {
-            const auto nSize = MapMemSize(operand.size);
+            const auto nSize = ra::data::models::TriggerValidation::MapRcheevosMemSize(operand.size);
             SetValue(pSizeProperty, ra::etoi(nSize));
             SetValue(pValueProperty, operand.value.memref->address);
             break;
@@ -380,7 +347,7 @@ std::wstring TriggerConditionViewModel::GetValueTooltip(unsigned int nValue)
     return std::to_wstring(nValue);
 }
 
-static bool IsIndirectMemref(rc_operand_t& operand) noexcept
+static bool IsIndirectMemref(const rc_operand_t& operand) noexcept
 {
     return rc_operand_is_memref(&operand) && operand.value.memref->value.is_indirect;
 }
@@ -405,6 +372,7 @@ unsigned int TriggerConditionViewModel::GetIndirectAddress(unsigned int nAddress
     bool bIsIndirect = false;
     rc_eval_state_t oEvalState;
     memset(&oEvalState, 0, sizeof(oEvalState));
+    rc_typed_value_t value = {};
     oEvalState.peek = rc_peek_callback;
 
     gsl::index nConditionIndex = 0;
@@ -440,11 +408,15 @@ unsigned int TriggerConditionViewModel::GetIndirectAddress(unsigned int nAddress
                     oCondition.operand2.value.memref = &oTarget;
                 }
 
-                oEvalState.add_address = rc_evaluate_condition_value(&oCondition, &oEvalState);
+                rc_evaluate_condition_value(&value, &oCondition, &oEvalState);
+                rc_typed_value_convert(&value, RC_VALUE_TYPE_UNSIGNED);
+                oEvalState.add_address = value.value.u32;
             }
             else
             {
-                oEvalState.add_address = rc_evaluate_condition_value(pCondition, &oEvalState);
+                rc_evaluate_condition_value(&value, pCondition, &oEvalState);
+                rc_typed_value_convert(&value, RC_VALUE_TYPE_UNSIGNED);
+                oEvalState.add_address = value.value.u32;
                 bIsIndirect = true;
             }
         }
diff --git a/src/ui/viewmodels/TriggerConditionViewModel.hh b/src/ui/viewmodels/TriggerConditionViewModel.hh
index 00b2ab5f..83a760d0 100644
--- a/src/ui/viewmodels/TriggerConditionViewModel.hh
+++ b/src/ui/viewmodels/TriggerConditionViewModel.hh
@@ -137,8 +137,6 @@ public:
     void SetRowColor(Color value) { SetValue(RowColorProperty, ra::to_signed(value.ARGB)); }
     void UpdateRowColor(const rc_condition_t* pCondition);
 
-    static MemSize MapRcheevosMemSize(char nSize) noexcept;
-
 private:
     void OnValueChanged(const IntModelProperty::ChangeArgs& args) override;
     void OnValueChanged(const BoolModelProperty::ChangeArgs& args) override;


Test patch:
diff --git a/tests/data/models/TriggerValidation_Tests.cpp b/tests/data/models/TriggerValidation_Tests.cpp
index c441e2b7..378f6432 100644
--- a/tests/data/models/TriggerValidation_Tests.cpp
+++ b/tests/data/models/TriggerValidation_Tests.cpp
@@ -61,6 +61,59 @@ public:
         AssertValidation("C:0xH1234=1_0xH2345=2.1.", L"");
         AssertValidation("D:0xH1234=1_0xH2345=2.1.", L"");
     }
+
+    TEST_METHOD(TestRangeComparisons)
+    {
+        AssertValidation("0xH1234>1", L"");
+
+        AssertValidation("0xH1234=255", L"");
+        AssertValidation("0xH1234!=255", L"");
+        AssertValidation("0xH1234>255", L"Condition 1: Comparison is never true");
+        AssertValidation("0xH1234>=255", L"");
+        AssertValidation("0xH1234<255", L"");
+        AssertValidation("0xH1234<=255", L"Condition 1: Comparison is always true");
+
+        AssertValidation("R:0xH1234<255", L"");
+        AssertValidation("R:0xH1234<=255", L"Condition 1: Comparison is always true");
+
+        AssertValidation("0xH1234=256", L"Condition 1: Comparison is never true");
+        AssertValidation("0xH1234!=256", L"Condition 1: Comparison is always true");
+        AssertValidation("0xH1234>256", L"Condition 1: Comparison is never true");
+        AssertValidation("0xH1234>=256", L"Condition 1: Comparison is never true");
+        AssertValidation("0xH1234<256", L"Condition 1: Comparison is always true");
+        AssertValidation("0xH1234<=256", L"Condition 1: Comparison is always true");
+
+        AssertValidation("0x 1234>=65535", L"");
+        AssertValidation("0x 1234>=65536", L"Condition 1: Comparison is never true");
+
+        AssertValidation("0xW1234>=16777215", L"");
+        AssertValidation("0xW1234>=16777216", L"Condition 1: Comparison is never true");
+
+        AssertValidation("0xX1234>=4294967295", L"");
+        AssertValidation("0xX1234>4294967295", L"Condition 1: Comparison is never true");
+
+        AssertValidation("0xT1234>=1", L"");
+        AssertValidation("0xT1234>1", L"Condition 1: Comparison is never true");
+
+        // AddSource max is the sum of all parts (255+255=510)
+        AssertValidation("A:0xH1234_0xH1235<510", L"");
+        AssertValidation("A:0xH1234_0xH1235<=510", L"Condition 2: Comparison is always true");
+
+        // SubSource max is always 0xFFFFFFFF
+        AssertValidation("B:0xH1234_0xH1235<510", L"");
+        AssertValidation("B:0xH1234_0xH1235<=510", L"");
+    }
+
+    TEST_METHOD(TestSizeComparisons)
+    {
+        AssertValidation("0xH1234>0xH1235", L"");
+        AssertValidation("0xH1234>0x 1235", L"Condition 1: Comparing different memory sizes");
+
+        // AddSource chain may compare different sizes without warning as the chain changes the
+        // size of the final result.
+        AssertValidation("A:0xH1234_0xH1235=0xH2345", L"");
+        AssertValidation("A:0xH1234_0xH1235=0x 2345", L"");
+    }
 };
 
 
diff --git a/tests/services/GameIdentifier_Tests.cpp b/tests/services/GameIdentifier_Tests.cpp
index 0fc0cc04..cc1d9fd6 100644
--- a/tests/services/GameIdentifier_Tests.cpp
+++ b/tests/services/GameIdentifier_Tests.cpp
@@ -26,8 +26,9 @@ namespace ra {
 namespace services {
 namespace tests {
 
-std::array<BYTE, 16> ROM = { 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16 };
-std::string ROM_HASH = "190c4c105786a2121d85018939108a6c";
+static std::array<BYTE, 16> ROM = { 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16 };
+static std::array<BYTE, 16> ROM2 = { 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17 };
+static std::string ROM_HASH = "190c4c105786a2121d85018939108a6c";
 
 class GameIdentifierHarness : public GameIdentifier
 {
@@ -226,6 +227,85 @@ public:
         Assert::AreEqual(ROM_HASH, identifier.mockGameContext.GameHash());
     }
 
+    TEST_METHOD(TestIdentifyGameCached)
+    {
+        GameIdentifierHarness identifier;
+        identifier.mockUserContext.Initialize("User", "ApiToken");
+        identifier.MockResolveHashResponse(23U);
+
+        Assert::AreEqual(23U, identifier.IdentifyGame(&ROM.at(0), ROM.size()));
+
+        identifier.mockServer.HandleRequest<ra::api::ResolveHash>(
+            [](const ra::api::ResolveHash::Request&, ra::api::ResolveHash::Response& response)
+        {
+            response.Result = ra::api::ApiResult::Success;
+            response.GameId = 32U;
+            return true;
+        });
+        Assert::AreEqual(32U, identifier.IdentifyGame(&ROM2.at(0), ROM2.size()));
+
+        // switching back and forth between hashes should avoid the server request
+        identifier.mockServer.HandleRequest<ra::api::ResolveHash>(
+            [](const ra::api::ResolveHash::Request&, ra::api::ResolveHash::Response& response)
+        {
+            response.Result = ra::api::ApiResult::Success;
+            response.GameId = 99U;
+            return true;
+        });
+        Assert::AreEqual(23U, identifier.IdentifyGame(&ROM.at(0), ROM.size()));
+        Assert::AreEqual(32U, identifier.IdentifyGame(&ROM2.at(0), ROM2.size()));
+    }
+
+    TEST_METHOD(TestIdentifyGameUnknownNotCached)
+    {
+        GameIdentifierHarness identifier;
+        identifier.mockEmulatorContext.MockGameTitle("TestGame");
+        identifier.mockUserContext.Initialize("User", "ApiToken");
+
+        identifier.mockServer.HandleRequest<ra::api::ResolveHash>(
+            [](const ra::api::ResolveHash::Request&, ra::api::ResolveHash::Response& response)
+        {
+            response.Result = ra::api::ApiResult::Success;
+            response.GameId = 0U;
+            return true;
+        });
+
+        bool bDialogShown = false;
+        identifier.mockDesktop.ExpectWindow<ra::ui::viewmodels::UnknownGameViewModel>(
+            [&bDialogShown](ra::ui::viewmodels::UnknownGameViewModel& vmUnknownGame)
+        {
+            bDialogShown = true;
+            vmUnknownGame.SetSelectedGameId(23);
+            return ra::ui::DialogResult::OK;
+        });
+        Assert::AreEqual(23U, identifier.IdentifyGame(&ROM.at(0), ROM.size()));
+        Assert::IsTrue(bDialogShown);
+
+        bDialogShown = false;
+        identifier.mockDesktop.ResetExpectedWindows();
+        identifier.mockDesktop.ExpectWindow<ra::ui::viewmodels::UnknownGameViewModel>(
+            [&bDialogShown](ra::ui::viewmodels::UnknownGameViewModel& vmUnknownGame)
+        {
+            bDialogShown = true;
+            vmUnknownGame.SetSelectedGameId(32);
+            return ra::ui::DialogResult::OK;
+        });
+        Assert::AreEqual(32U, identifier.IdentifyGame(&ROM2.at(0), ROM2.size()));
+        Assert::IsTrue(bDialogShown);
+
+        // switching back to the first unidentified game will show the unknown game dialog again
+        bDialogShown = false;
+        identifier.mockDesktop.ResetExpectedWindows();
+        identifier.mockDesktop.ExpectWindow<ra::ui::viewmodels::UnknownGameViewModel>(
+            [&bDialogShown](ra::ui::viewmodels::UnknownGameViewModel& vmUnknownGame)
+        {
+            bDialogShown = true;
+            vmUnknownGame.SetSelectedGameId(99);
+            return ra::ui::DialogResult::OK;
+        });
+        Assert::AreEqual(99U, identifier.IdentifyGame(&ROM.at(0), ROM.size()));
+    }
+
     TEST_METHOD(TestActivateGameZeroGameNotLoaded)
     {
         GameIdentifierHarness identifier;
diff --git a/tests/services/SearchResults_Tests.cpp b/tests/services/SearchResults_Tests.cpp
index 09b4313b..2d64f4f3 100644
--- a/tests/services/SearchResults_Tests.cpp
+++ b/tests/services/SearchResults_Tests.cpp
@@ -1240,6 +1240,30 @@ public:
         Assert::AreEqual(24U, result.nAddress);
     }
 
+    TEST_METHOD(TestInitializeFromResultsAsciiTextSingleMatch)
+    {
+        std::array<unsigned char, 36> memory{
+            'S', 'h', 'e', ' ', 's', 'e', 'l', 'l', 's', ' ', 's', 'e', 'a', 's', 'h', 'e',
+            'l', 'l', 's', ' ', 'b', 'y', ' ', 't', 'h', 'e', ' ', 's', 'e', 'a', 's', 'h',
+            'o', 'r', 'e', '.'
+        };
+        ra::data::context::mocks::MockEmulatorContext mockEmulatorContext;
+        mockEmulatorContext.MockMemory(memory);
+
+        SearchResults results1;
+        results1.Initialize(0U, memory.size(), ra::services::SearchType::AsciiText);
+        Assert::AreEqual(memory.size(), results1.MatchingAddressCount());
+
+        SearchResults results2;
+        results2.Initialize(results1, ComparisonType::Equals, ra::services::SearchFilterType::Constant, L"by");
+        Assert::AreEqual({ 1U }, results2.MatchingAddressCount());
+        Assert::IsTrue(results2.ContainsAddress(20U));
+
+        SearchResults::Result result;
+        Assert::IsTrue(results2.GetMatchingAddress(0U, result));
+        Assert::AreEqual(20U, result.nAddress);
+    }
+
     TEST_METHOD(TestInitializeFromResultsAsciiTextComparison)
     {
         std::array<unsigned char, 36> memory{