Repository: RetroAchievements/RAIntegration (cpp)

Issue:
Double Input in Memory Bookmark value after invalid input
After an invalid input into a memory bookmark value, input is doubled until a valid value is entered. This occurs when I input an invalid value and click outside the input box on a window (Game, Memory Bookmarks, etc.)  
This doesn't occur if I press enter to confirm the invalid value. This also doesn't occur if I click on the desktop. How I interact with the error popup doesn't seem to affect the behavior.

Can also see the column headers disappear when resizing the Memory Bookmarks window, reappear on mouse hover.

Beta 1.2 dll

https://user-images.githubusercontent.com/12983268/221969236-996d1c61-75d2-4c79-a41e-4025a47d389f.mp4



Pull request description:
only force WM_CHAR translation for WM_GETDLGCODE messages associated to key messages
fixes #982 

Only sets the `bForceWmChar` flag if `WM_GETDLGCODE` is called with an `lParam`. Seems to fix the issue in RALibretro without breaking the functionality in BizHawk, though I'm making the change based on what I'm seeing in the debugger, rather than any concrete documentation indicating it's the right solution.

Could potentially make it even more strict and only set `bForceWmChar` if `((LPMSG)lParam)->msg == WM_KEYDOWN`

Patch:
diff --git a/src/data/models/TriggerValidation.cpp b/src/data/models/TriggerValidation.cpp
index 57f49830..6acacb19 100644
--- a/src/data/models/TriggerValidation.cpp
+++ b/src/data/models/TriggerValidation.cpp
@@ -3,6 +3,7 @@
 #include "RA_StringUtils.h"
 
 #include "data/context/ConsoleContext.hh"
+#include "data/context/EmulatorContext.hh"
 
 #include "services/ServiceLocator.hh"
 
@@ -103,6 +104,13 @@ bool TriggerValidation::Validate(const std::string& sTrigger, std::wstring& sErr
     {
         const auto& pConsoleContext = ra::services::ServiceLocator::Get<ra::data::context::ConsoleContext>();
         nMaxAddress = pConsoleContext.MaxAddress();
+
+        // if console definition doesn't specify the max address, see how much was exposed by the emulator
+        if (nMaxAddress == 0)
+        {
+            const auto& pEmulatorContext = ra::services::ServiceLocator::Get<ra::data::context::EmulatorContext>();
+            nMaxAddress = gsl::narrow_cast<unsigned>(pEmulatorContext.TotalMemorySize()) - 1;
+        }
     }
 
     char sErrorBuffer[256];
diff --git a/src/ui/viewmodels/OverlayManager.cpp b/src/ui/viewmodels/OverlayManager.cpp
index 3fc7ba30..4e530dca 100644
--- a/src/ui/viewmodels/OverlayManager.cpp
+++ b/src/ui/viewmodels/OverlayManager.cpp
@@ -484,7 +484,7 @@ void OverlayManager::UpdatePopup(ra::ui::drawing::ISurface& pSurface, const Popu
         if (nOldWidth != 0)
         {
             const int nHeight = pNewImage.GetHeight() + std::abs(nNewPos.Y - nOldY);
-            if (nNewPos.X > nOldX)
+            if (nNewPos.X > nOldX && nOldX + ra::to_signed(nOldWidth) >= 0)
                 pSurface.FillRectangle(nOldX - nFudge, nOldY, nNewPos.X - nOldX + nFudge, nHeight, ra::ui::Color::Transparent);
 
             const int nOldRightBound = nOldX + nOldWidth;
@@ -503,7 +503,7 @@ void OverlayManager::UpdatePopup(ra::ui::drawing::ISurface& pSurface, const Popu
 
         if (nOldHeight != 0)
         {
-            if (nNewPos.Y > nOldY)
+            if (nNewPos.Y > nOldY && nOldY + ra::to_signed(nOldHeight) >= 0)
                 pSurface.FillRectangle(nNewPos.X, nOldY - nFudge, pNewImage.GetWidth(), nNewPos.Y - nOldY + nFudge, ra::ui::Color::Transparent);
 
             const int nOldLowerBound = nOldY + nOldHeight;
@@ -605,11 +605,11 @@ void OverlayManager::UpdateScoreTrackers(ra::ui::drawing::ISurface& pSurface, Po
     {
         auto& vmTracker = **pIter;
 
-        if (bEnabled)
-            UpdatePopup(pSurface, pPopupLocations, fElapsed, vmTracker);
-
         if (vmTracker.IsDestroyPending())
         {
+            // call UpdatePopup to force it to "unpaint"
+            UpdatePopup(pSurface, pPopupLocations, fElapsed, vmTracker);
+
             pIter = m_vScoreTrackers.erase(pIter);
         }
         else
@@ -634,6 +634,7 @@ void OverlayManager::UpdateScoreTrackers(ra::ui::drawing::ISurface& pSurface, Po
                     }
                 }
 
+                UpdatePopup(pSurface, pPopupLocations, fElapsed, vmTracker);
                 AdjustLocationForPopup(pPopupLocations, vmTracker);
             }
 
@@ -690,7 +691,12 @@ void OverlayManager::UpdateChallengeIndicators(ra::ui::drawing::ISurface& pSurfa
 
 void OverlayManager::UpdateProgressTracker(ra::ui::drawing::ISurface& pSurface, PopupLocations& pPopupLocations, double fElapsed)
 {
-    assert(m_vmProgressTracker != nullptr);
+    Expects(m_vmProgressTracker != nullptr);
+
+    UpdatePopup(pSurface, pPopupLocations, fElapsed, *m_vmProgressTracker);
+
+    if (m_vmProgressTracker->IsAnimationComplete())
+        m_vmProgressTracker->SetDestroyPending();
 
     if (m_vmProgressTracker->IsDestroyPending())
     {
@@ -698,23 +704,14 @@ void OverlayManager::UpdateProgressTracker(ra::ui::drawing::ISurface& pSurface,
         UpdatePopup(pSurface, pPopupLocations, fElapsed, *m_vmProgressTracker);
 
         m_vmProgressTracker.reset();
-        return;
     }
-
-    const auto& pConfiguration = ra::services::ServiceLocator::Get<ra::services::IConfiguration>();
-    const auto bEnabled = (pConfiguration.GetPopupLocation(ra::ui::viewmodels::Popup::Progress) != ra::ui::viewmodels::PopupLocation::None);
-
-    if (bEnabled)
+    else
     {
-        UpdatePopup(pSurface, pPopupLocations, fElapsed, *m_vmProgressTracker);
-
-        if (m_vmProgressTracker->IsAnimationComplete())
-        {
-            m_vmProgressTracker->SetDestroyPending();
-            return;
-        }
+        const auto& pConfiguration = ra::services::ServiceLocator::Get<ra::services::IConfiguration>();
+        const auto bEnabled = (pConfiguration.GetPopupLocation(ra::ui::viewmodels::Popup::Progress) != ra::ui::viewmodels::PopupLocation::None);
 
-        AdjustLocationForPopup(pPopupLocations, *m_vmProgressTracker);
+        if (bEnabled)
+            AdjustLocationForPopup(pPopupLocations, *m_vmProgressTracker);
     }
 }
 
diff --git a/src/ui/win32/bindings/GridTextColumnBinding.cpp b/src/ui/win32/bindings/GridTextColumnBinding.cpp
index 4b3ea144..7fe1d8f6 100644
--- a/src/ui/win32/bindings/GridTextColumnBinding.cpp
+++ b/src/ui/win32/bindings/GridTextColumnBinding.cpp
@@ -85,9 +85,17 @@ static LRESULT CALLBACK EditBoxProc(HWND hwnd, UINT nMsg, WPARAM wParam, LPARAM
             break;
 
         case WM_GETDLGCODE:
-            // only reached if IsDialogMessage is used (also used as a signal to activate the above hack)
-            pInfo->bForceWmChar = true;
-            return DLGC_WANTALLKEYS;
+            if (lParam)
+            {
+                // If lParam is not null, a keyboard message is being processed. Assume we're in an IsDialogMessage
+                // call and set a flag indicating we need to manually convert the WM_KEYDOWN to WM_CHAR.
+                pInfo->bForceWmChar = true;
+                return DLGC_WANTALLKEYS;
+            }
+
+            // If lParam is null, don't set the flag - we're being called as the result of a focus change or
+            // something similar.
+            break;
     }
 
     return CallWindowProc(pInfo->pOriginalWndProc, hwnd, nMsg, wParam, lParam);


Test patch:
diff --git a/tests/data/models/TriggerValidation_Tests.cpp b/tests/data/models/TriggerValidation_Tests.cpp
index 7a0a42fc..9f5d70c4 100644
--- a/tests/data/models/TriggerValidation_Tests.cpp
+++ b/tests/data/models/TriggerValidation_Tests.cpp
@@ -3,6 +3,7 @@
 #include "data\models\TriggerValidation.hh"
 
 #include "tests\mocks\MockConsoleContext.hh"
+#include "tests\mocks\MockEmulatorContext.hh"
 
 using namespace Microsoft::VisualStudio::CppUnitTestFramework;
 
@@ -146,6 +147,29 @@ public:
         AssertValidation("I:0xX1234_0xH1234>5_0xHFFFFFF00>5", L"Condition 3: Address FFFFFF00 out of range (max FFFF)");
     }
 
+    TEST_METHOD(TestAddressRangeArcade)
+    {
+        // arcade doesn't provide a max address as each game can have a different amount of memory
+        // instead, max valid address is determined by how much memory the emulator/core exposes
+        ra::data::context::mocks::MockConsoleContext mockConsoleContext(Arcade, L"Arcade");
+        Assert::AreEqual(0U, mockConsoleContext.MaxAddress());
+        ra::data::context::mocks::MockEmulatorContext mockEmulatorContext;
+        unsigned char pMemory[256];
+        mockEmulatorContext.MockMemory(pMemory, sizeof(pMemory));
+        Assert::AreEqual({ 256U }, mockEmulatorContext.TotalMemorySize());
+
+        AssertValidation("0xH12>0xH34", L"");
+        AssertValidation("0xH123>0xH34", L"Condition 1: Address 0123 out of range (max 00FF)");
+        AssertValidation("0xH12>0xH345", L"Condition 1: Address 0345 out of range (max 00FF)");
+        AssertValidation("0xH123>0xH123", L"Condition 1: Address 0123 out of range (max 00FF)");
+        AssertValidation("0xX12>h123", L"");
+
+        // edge cases
+        AssertValidation("0xH00>5", L"");
+        AssertValidation("0xHFF>5", L"");
+        AssertValidation("0xH100>5", L"Condition 1: Address 0100 out of range (max 00FF)");
+    }
+
     TEST_METHOD(TestLeaderboardConditionTypes)
     {
         AssertLeaderboardValidation("0xH1234<99_0x2345<50", L"");