Repository: RetroAchievements/RAIntegration
Pull request #944 (2022-12-13 14:01:15)

ensure cursor is visible when modifying memory
Prevents crash if the user tries to modify memory after scrolling the viewer such that the cursor is no longer visible by resetting the scroll position such that the cursor is visible.


Patch:
diff --git a/src/ui/viewmodels/MemoryViewerViewModel.cpp b/src/ui/viewmodels/MemoryViewerViewModel.cpp
index 7bdcf68c..3e86752b 100644
--- a/src/ui/viewmodels/MemoryViewerViewModel.cpp
+++ b/src/ui/viewmodels/MemoryViewerViewModel.cpp
@@ -835,18 +835,33 @@ bool MemoryViewerViewModel::OnChar(char c)
             return false;
     }
 
-    auto nIndex = GetAddress() - GetFirstAddress();
+    auto nAddress = GetAddress();
+    auto nFirstAddress = GetFirstAddress();
+    const auto nVisibleLines = GetNumVisibleLines();
+    const auto nMaxAddress = nFirstAddress + nVisibleLines * 16;
+
+    // make sure the cursor is on screen (same logic as in OnValueChanged(AddressProperty))
+    if (nAddress < nFirstAddress || nAddress >= nMaxAddress)
+    {
+        nFirstAddress = (nAddress & ~0x0F) - ((nVisibleLines / 2) * 16);
+        SetFirstAddress(nFirstAddress);
 
+        nFirstAddress = GetFirstAddress();
+    }
+
+    // adjust for 16-bit and 32-bit views
     const auto nNibblesPerWord = NibblesPerWord();
     auto nSelectedNibble = (nNibblesPerWord - m_nSelectedNibble - 1);
     while (nSelectedNibble > 1)
     {
-        ++nIndex;
+        ++nAddress;
         nSelectedNibble -= 2;
     }
 
+    auto nIndex = nAddress - nFirstAddress;
     if (m_pInvalid[nIndex])
     {
+        // can't update "--"
         AdvanceCursor();
         return true;
     }
@@ -867,8 +882,6 @@ bool MemoryViewerViewModel::OnChar(char c)
     m_pColor[nIndex] |= STALE_COLOR;
     m_nNeedsRedraw |= REDRAW_MEMORY;
 
-    const auto nAddress = GetFirstAddress() + nIndex;
-
     // push the updated value to the emulator
     auto& pEmulatorContext = ra::services::ServiceLocator::GetMutable<ra::data::context::EmulatorContext>();
     pEmulatorContext.WriteMemoryByte(nAddress, nByte);


Test patch:
diff --git a/tests/ui/viewmodels/CodeNotesViewModel_Tests.cpp b/tests/ui/viewmodels/CodeNotesViewModel_Tests.cpp
index 158732e4..13a2d2a2 100644
--- a/tests/ui/viewmodels/CodeNotesViewModel_Tests.cpp
+++ b/tests/ui/viewmodels/CodeNotesViewModel_Tests.cpp
@@ -7,6 +7,7 @@
 #include "tests\ui\UIAsserts.hh"
 #include "tests\RA_UnitTestHelpers.h"
 #include "tests\mocks\MockConfiguration.hh"
+#include "tests\mocks\MockConsoleContext.hh"
 #include "tests\mocks\MockDesktop.hh"
 #include "tests\mocks\MockEmulatorContext.hh"
 #include "tests\mocks\MockGameContext.hh"
@@ -30,6 +31,7 @@ private:
     {
     public:
         ra::api::mocks::MockServer mockServer;
+        ra::data::context::mocks::MockConsoleContext mockConsoleContext;
         ra::data::context::mocks::MockEmulatorContext mockEmulatorContext;
         ra::data::context::mocks::MockGameContext mockGameContext;
         ra::data::context::mocks::MockUserContext mockUserContext;
diff --git a/tests/ui/viewmodels/MemoryViewerViewModel_Tests.cpp b/tests/ui/viewmodels/MemoryViewerViewModel_Tests.cpp
index a1e779a0..1a440989 100644
--- a/tests/ui/viewmodels/MemoryViewerViewModel_Tests.cpp
+++ b/tests/ui/viewmodels/MemoryViewerViewModel_Tests.cpp
@@ -1802,6 +1802,53 @@ public:
         viewer.SetFirstAddress(0x60U);
         Assert::AreEqual({ 0x5CU }, viewer.GetAddress());
     }
+
+    TEST_METHOD(TestOnCharOffscreen)
+    {
+        MemoryViewerViewModelHarness viewer;
+        viewer.InitializeMemory(256);
+        viewer.MockRender();
+        viewer.SetAddress(20U); // set address first, as it updates FirstAddress
+        viewer.SetFirstAddress(64U); // showing $0040-$00BF (8 lines)
+        viewer.SetReadOnly(false);
+        Assert::IsTrue(viewer.NeedsRedraw());
+        viewer.MockRender();
+
+        Assert::AreEqual({ 20U }, viewer.GetAddress());
+        Assert::AreEqual({ 64U }, viewer.GetFirstAddress());
+        Assert::AreEqual({ 8U }, viewer.GetNumVisibleLines());
+        Assert::AreEqual({ 0U }, viewer.GetSelectedNibble());
+        Assert::IsFalse(viewer.NeedsRedraw());
+
+        // cursor should be moved onscreen and '6' should be set as upper nibble of selected byte
+        Assert::IsTrue(viewer.OnChar('6'));
+        Assert::AreEqual({ 20U }, viewer.GetAddress());
+        Assert::AreEqual({ 0U }, viewer.GetFirstAddress());
+        Assert::AreEqual({ 1U }, viewer.GetSelectedNibble());
+        Assert::AreEqual({ 0x64U }, viewer.mockEmulatorContext.ReadMemoryByte(20U));
+        Assert::AreEqual({ COLOR_RED | COLOR_REDRAW }, viewer.GetColor(20U));
+        Assert::IsTrue(viewer.NeedsRedraw());
+        viewer.MockRender();
+
+        // jump to a later address, scroll it below the visible window
+        viewer.SetAddress(0xE3U);
+        viewer.SetFirstAddress(64U); // showing $0040-$00BF (8 lines)
+        Assert::AreEqual({ 0xE3U }, viewer.GetAddress());
+        Assert::AreEqual({ 64U }, viewer.GetFirstAddress());
+        Assert::AreEqual({ 8U }, viewer.GetNumVisibleLines());
+        Assert::AreEqual({ 0U }, viewer.GetSelectedNibble());
+        Assert::IsTrue(viewer.NeedsRedraw());
+        viewer.MockRender();
+
+        viewer.OnChar('0');
+        Assert::AreEqual({ 0xE3U }, viewer.GetAddress());
+        Assert::AreEqual({ 0x80U }, viewer.GetFirstAddress());
+        Assert::AreEqual({ 1U }, viewer.GetSelectedNibble());
+        Assert::AreEqual({ 0x03U }, viewer.mockEmulatorContext.ReadMemoryByte(0xE3U));
+        Assert::AreEqual({ COLOR_RED | COLOR_REDRAW }, viewer.GetColor(0xE3U));
+        Assert::IsTrue(viewer.NeedsRedraw());
+        viewer.MockRender();
+    }
 };
 
 } // namespace tests