Repository: RetroAchievements/RAIntegration
Pull request #671 (2021-01-30 21:34:45)

ensure selected index is valid after rebuilding list
fixes https://github.com/RetroAchievements/RALibretro/issues/207

when rebuilding the overlay, if the selected index is not valid, it's reset to 0.


Patch:
diff --git a/src/Exports.cpp b/src/Exports.cpp
index 900f79bb..08496bd8 100644
--- a/src/Exports.cpp
+++ b/src/Exports.cpp
@@ -80,6 +80,21 @@ static BOOL InitCommon([[maybe_unused]] HWND hMainHWND, [[maybe_unused]] int nEm
     ra::services::Initialization::RegisterServices(ra::itoe<EmulatorID>(nEmulatorID), sClientName);
 
     _RA_UpdateHWnd(hMainHWND);
+
+    // When using SDL, the Windows message queue is never empty (there's a flood of WM_PAINT messages for the
+    // SDL window). InvalidateRect only generates a WM_PAINT when the message queue is empty, so we have to
+    // explicitly generate (and dispatch) a WM_PAINT message by calling UpdateWindow.
+    switch (ra::itoe<EmulatorID>(nEmulatorID))
+    {
+        case RA_Libretro:
+        case RA_Oricutron:
+            ra::ui::win32::bindings::ControlBinding::SetNeedsUpdateWindow(true);
+            break;
+
+        default:
+            ra::ui::win32::bindings::ControlBinding::SetNeedsUpdateWindow(false);
+            break;
+    }
 #endif
 
     if (bOffline)
@@ -512,6 +527,13 @@ API void CCONV _RA_DoAchievementsFrame()
     CHECK_PERFORMANCE();
 }
 
+API void CCONV _RA_SetForceRepaint([[maybe_unused]] bool bEnable)
+{
+#ifndef RA_UTEST
+    ra::ui::win32::bindings::ControlBinding::SetNeedsUpdateWindow(bEnable);
+#endif
+}
+
 API void CCONV _RA_SuspendRepaint()
 {
 #ifndef RA_UTEST
diff --git a/src/Exports.hh b/src/Exports.hh
index e6849044..39d02936 100644
--- a/src/Exports.hh
+++ b/src/Exports.hh
@@ -88,6 +88,10 @@ extern "C" {
     // Perform one test for all achievements in the current set. Call this once per frame/cycle.
     API void CCONV _RA_DoAchievementsFrame();
 
+    // Enables forced repainting for controls when InvalidateWindow doesn't cause them to update
+    // properly. Seems to primarily be an issue when integrating with an application using SDL.
+    API void CCONV _RA_SetForceRepaint(bool bEnable);
+
     // Temporarily disable forced redraws
     API void CCONV _RA_SuspendRepaint();
 
diff --git a/src/ui/viewmodels/OverlayAchievementsPageViewModel.cpp b/src/ui/viewmodels/OverlayAchievementsPageViewModel.cpp
index 64b375e1..e849697c 100644
--- a/src/ui/viewmodels/OverlayAchievementsPageViewModel.cpp
+++ b/src/ui/viewmodels/OverlayAchievementsPageViewModel.cpp
@@ -376,22 +376,7 @@ void OverlayAchievementsPageViewModel::Refresh()
         m_fElapsed = static_cast<double>(nPlayTimeSeconds.count() % 60);
     }
 
-    // ensure selected index is valid
-    auto nSelectedIndex = GetSelectedItemIndex();
-    const auto* vmItem = m_vItems.GetItemAt(nSelectedIndex);
-    while (vmItem && vmItem->IsHeader())
-        vmItem = m_vItems.GetItemAt(++nSelectedIndex);
-
-    if (!vmItem)
-    {
-        nSelectedIndex = 0;
-        vmItem = m_vItems.GetItemAt(nSelectedIndex);
-        while (vmItem && vmItem->IsHeader())
-            vmItem = m_vItems.GetItemAt(++nSelectedIndex);
-    }
-
-    if (nSelectedIndex < ra::to_signed(m_vItems.Count()))
-        SetSelectedItemIndex(nSelectedIndex);
+    EnsureSelectedItemIndexValid();
 }
 
 bool OverlayAchievementsPageViewModel::Update(double fElapsed)
diff --git a/src/ui/viewmodels/OverlayFriendsPageViewModel.cpp b/src/ui/viewmodels/OverlayFriendsPageViewModel.cpp
index 05aaee7b..44316a42 100644
--- a/src/ui/viewmodels/OverlayFriendsPageViewModel.cpp
+++ b/src/ui/viewmodels/OverlayFriendsPageViewModel.cpp
@@ -49,8 +49,9 @@ void OverlayFriendsPageViewModel::Refresh()
                 if (pvmFriend == nullptr)
                 {
                     pvmFriend = &m_vItems.Add();
-                    pvmFriend->Image.ChangeReference(ra::ui::ImageType::UserPic, pFriend.User);
                     Ensures(pvmFriend != nullptr);
+                    pvmFriend->SetId(gsl::narrow_cast<int>(m_vItems.Count())); // prevent it from looking like a header
+                    pvmFriend->Image.ChangeReference(ra::ui::ImageType::UserPic, pFriend.User);
                 }
 
                 pvmFriend->SetLabel(ra::StringPrintf(L"%s (%u)", pFriend.User, pFriend.Score));
diff --git a/src/ui/viewmodels/OverlayListPageViewModel.cpp b/src/ui/viewmodels/OverlayListPageViewModel.cpp
index c9207056..c2ca99d9 100644
--- a/src/ui/viewmodels/OverlayListPageViewModel.cpp
+++ b/src/ui/viewmodels/OverlayListPageViewModel.cpp
@@ -28,6 +28,27 @@ void OverlayListPageViewModel::Refresh()
     m_nImagesPending = 99;
 }
 
+void OverlayListPageViewModel::EnsureSelectedItemIndexValid()
+{
+    auto nSelectedIndex = GetSelectedItemIndex();
+    const auto* vmItem = m_vItems.GetItemAt(nSelectedIndex);
+    while (vmItem && vmItem->IsHeader())
+        vmItem = m_vItems.GetItemAt(++nSelectedIndex);
+
+    if (!vmItem)
+    {
+        nSelectedIndex = 0;
+        vmItem = m_vItems.GetItemAt(nSelectedIndex);
+        while (vmItem && vmItem->IsHeader())
+            vmItem = m_vItems.GetItemAt(++nSelectedIndex);
+
+        m_nScrollOffset = 0;
+    }
+
+    if (nSelectedIndex < ra::to_signed(m_vItems.Count()))
+        SetSelectedItemIndex(nSelectedIndex);
+}
+
 bool OverlayListPageViewModel::Update(double fElapsed)
 {
     m_fElapsed += fElapsed;
@@ -208,6 +229,7 @@ bool OverlayListPageViewModel::ProcessInput(const ControllerInput& pInput)
         if (nSelectedItemIndex > 0)
         {
             auto* vmItem = m_vItems.GetItemAt(gsl::narrow_cast<size_t>(--nSelectedItemIndex));
+            const auto nScrollOfffset = m_nScrollOffset;
 
             // skip over header items
             while (vmItem && vmItem->IsHeader())
@@ -217,7 +239,7 @@ bool OverlayListPageViewModel::ProcessInput(const ControllerInput& pInput)
                 m_nScrollOffset = std::max(nSelectedItemIndex, 0);
 
             if (!vmItem)
-                return false;
+                return (m_nScrollOffset != nScrollOfffset);
 
             SetSelectedItemIndex(nSelectedItemIndex);
 
diff --git a/src/ui/viewmodels/OverlayListPageViewModel.hh b/src/ui/viewmodels/OverlayListPageViewModel.hh
index b226fc26..1d7409c9 100644
--- a/src/ui/viewmodels/OverlayListPageViewModel.hh
+++ b/src/ui/viewmodels/OverlayListPageViewModel.hh
@@ -135,6 +135,7 @@ public:
     };
 
 protected:
+    void EnsureSelectedItemIndexValid();
     bool SetDetail(bool bDetail);
     void ForceRedraw();
     virtual void FetchItemDetail(_UNUSED ItemViewModel& vmItem) noexcept(false) {};
diff --git a/src/ui/win32/bindings/ControlBinding.cpp b/src/ui/win32/bindings/ControlBinding.cpp
index d26d1f62..a0cadef6 100644
--- a/src/ui/win32/bindings/ControlBinding.cpp
+++ b/src/ui/win32/bindings/ControlBinding.cpp
@@ -16,6 +16,7 @@ namespace bindings {
 
 static std::atomic_int g_nSuspendRepaintCount = 0;
 static std::set<HWND> g_vSuspendedRepaintHWnds;
+bool ControlBinding::s_bNeedsUpdateWindow = false;
 
 ControlBinding::~ControlBinding() noexcept
 {
@@ -28,29 +29,13 @@ ControlBinding::~ControlBinding() noexcept
     }
 }
 
-static bool NeedsUpdate()
-{
-    // When using SDL, the Windows message queue is never empty (there's a flood of WM_PAINT messages for the
-    // SDL window). InvalidateRect only generates a WM_PAINT when the message queue is empty, so we have to
-    // explicitly generate (and dispatch) a WM_PAINT message by calling UpdateWindow.
-    switch (ra::services::ServiceLocator::Get<ra::data::context::EmulatorContext>().GetEmulatorId())
-    {
-        case RA_Libretro:
-        case RA_Oricutron:
-            return true;
-
-        default:
-            return false;
-    }
-}
-
 void ControlBinding::ForceRepaint(HWND hWnd)
 {
     if (g_nSuspendRepaintCount == 0)
     {
         ::InvalidateRect(hWnd, nullptr, FALSE);
 
-        if (NeedsUpdate())
+        if (NeedsUpdateWindow())
             ::UpdateWindow(hWnd);
     }
     else
@@ -81,7 +66,7 @@ void ControlBinding::ResumeRepaint()
             for (HWND hWnd : vSuspendedRepaintWnds)
                 ::InvalidateRect(hWnd, nullptr, FALSE);
 
-            if (NeedsUpdate())
+            if (NeedsUpdateWindow())
             {
                 for (HWND hWnd : vSuspendedRepaintWnds)
                     ::UpdateWindow(hWnd);
diff --git a/src/ui/win32/bindings/ControlBinding.hh b/src/ui/win32/bindings/ControlBinding.hh
index ec219c25..50e9d320 100644
--- a/src/ui/win32/bindings/ControlBinding.hh
+++ b/src/ui/win32/bindings/ControlBinding.hh
@@ -82,6 +82,16 @@ public:
         return CDRF_DODEFAULT;
     }
 
+    /// <summary>
+    /// If set to true, UpdateWindow will be called when a control is Invalidated.
+    /// </summary>
+    static void SetNeedsUpdateWindow(bool bValue) noexcept { s_bNeedsUpdateWindow = bValue; }
+
+    /// <summary>
+    /// Determines if UpdateWindow should be called when invalidating a control.
+    /// </summary>
+    static bool NeedsUpdateWindow() noexcept { return s_bNeedsUpdateWindow; }
+
     /// <summary>
     /// Calls Invalidate() or UpdateWindow() to cause the control to repaint
     /// </summary>
@@ -161,6 +171,8 @@ protected:
 private:
     DialogBase* m_pDialog = nullptr;
     WNDPROC m_pOriginalWndProc = nullptr;
+
+    static bool s_bNeedsUpdateWindow;
 };
 
 } // namespace bindings


Test patch:
diff --git a/tests/ui/viewmodels/OverlayFriendsPageViewModel_Tests.cpp b/tests/ui/viewmodels/OverlayFriendsPageViewModel_Tests.cpp
index 9a26796e..ab1db7ad 100644
--- a/tests/ui/viewmodels/OverlayFriendsPageViewModel_Tests.cpp
+++ b/tests/ui/viewmodels/OverlayFriendsPageViewModel_Tests.cpp
@@ -89,18 +89,21 @@ public:
         Ensures(pFriend1 != nullptr);
         Assert::AreEqual(std::wstring(L"User1 (20)"), pFriend1->GetLabel());
         Assert::AreEqual(std::wstring(L"Activity1"), pFriend1->GetDetail());
+        Assert::IsFalse(pFriend1->IsHeader());
 
         const auto* pFriend2 = friendsPage.GetItem(1);
         Assert::IsNotNull(pFriend2);
         Ensures(pFriend2 != nullptr);
         Assert::AreEqual(std::wstring(L"User2 (60)"), pFriend2->GetLabel());
         Assert::AreEqual(std::wstring(L"Activity2"), pFriend2->GetDetail());
+        Assert::IsFalse(pFriend2->IsHeader());
 
         const auto* pFriend3 = friendsPage.GetItem(2);
         Assert::IsNotNull(pFriend3);
         Ensures(pFriend3 != nullptr);
         Assert::AreEqual(std::wstring(L"User3 (40)"), pFriend3->GetLabel());
         Assert::AreEqual(std::wstring(L"Activity3"), pFriend3->GetDetail());
+        Assert::IsFalse(pFriend3->IsHeader());
     }
 
     TEST_METHOD(TestRefreshError)
@@ -178,24 +181,28 @@ public:
         Ensures(pFriend1 != nullptr);
         Assert::AreEqual(std::wstring(L"User1 (30)"), pFriend1->GetLabel());
         Assert::AreEqual(std::wstring(L"Activity1b"), pFriend1->GetDetail());
+        Assert::IsFalse(pFriend1->IsHeader());
 
         const auto* pFriend2 = friendsPage.GetItem(1);
         Assert::IsNotNull(pFriend2);
         Ensures(pFriend2 != nullptr);
         Assert::AreEqual(std::wstring(L"User2 (60)"), pFriend2->GetLabel());
         Assert::AreEqual(std::wstring(L"Activity2"), pFriend2->GetDetail());
+        Assert::IsFalse(pFriend2->IsHeader());
 
         const auto* pFriend3 = friendsPage.GetItem(2);
         Assert::IsNotNull(pFriend3);
         Ensures(pFriend3 != nullptr);
         Assert::AreEqual(std::wstring(L"User3 (45)"), pFriend3->GetLabel());
         Assert::AreEqual(std::wstring(L"Activity3b"), pFriend3->GetDetail());
+        Assert::IsFalse(pFriend3->IsHeader());
 
         const auto* pFriend4 = friendsPage.GetItem(3);
         Assert::IsNotNull(pFriend4);
         Ensures(pFriend4 != nullptr);
         Assert::AreEqual(std::wstring(L"User4 (90)"), pFriend4->GetLabel());
         Assert::AreEqual(std::wstring(L"Activity4"), pFriend4->GetDetail());
+        Assert::IsFalse(pFriend4->IsHeader());
     }
 };
 
diff --git a/tests/ui/viewmodels/OverlayListPageViewModel_Tests.cpp b/tests/ui/viewmodels/OverlayListPageViewModel_Tests.cpp
index 01f930b8..8325a9df 100644
--- a/tests/ui/viewmodels/OverlayListPageViewModel_Tests.cpp
+++ b/tests/ui/viewmodels/OverlayListPageViewModel_Tests.cpp
@@ -44,6 +44,31 @@ private:
             m_nVisibleItems = 5;
         }
 
+        void RefreshWithHeader()
+        {
+            m_sTitle = L"Title";
+            m_sDetailTitle = L"Detail Title";
+            OverlayListPageViewModel::Refresh();
+
+            m_vItems.Clear();
+            AddItem(0, L"Header 1");
+            AddItem(1, L"One");
+            AddItem(2, L"Two");
+            AddItem(0, L"Header 2");
+            AddItem(3, L"Three");
+            AddItem(4, L"Four");
+            AddItem(0, L"Header 3");
+            AddItem(5, L"Five");
+            AddItem(6, L"Six");
+            AddItem(7, L"Seven");
+            AddItem(8, L"Eight");
+            AddItem(9, L"Nine");
+            AddItem(10, L"Ten");
+
+            m_nVisibleItems = 5;
+            EnsureSelectedItemIndexValid();
+        }
+
         void AddItem(int nId, const std::wstring&& sLabel)
         {
             auto& vmItem = m_vItems.Add();
@@ -274,6 +299,144 @@ public:
         Assert::AreEqual({ 0 }, vmListPage.GetScrollOffset());
     }
 
+    TEST_METHOD(TestProcessInputUpDownWithHeader)
+    {
+        OverlayListPageViewModelHarness vmListPage;
+        vmListPage.RefreshWithHeader();
+
+        Assert::AreEqual(1, vmListPage.GetSelectedItemIndex());
+        Assert::AreEqual({ 0 }, vmListPage.GetScrollOffset());
+
+        ControllerInput pInput{};
+        pInput.m_bDownPressed = true;
+        Assert::IsTrue(vmListPage.ProcessInput(pInput));
+        Assert::AreEqual(2, vmListPage.GetSelectedItemIndex());
+        Assert::AreEqual({ 0 }, vmListPage.GetScrollOffset());
+
+        Assert::IsTrue(vmListPage.ProcessInput(pInput));
+        Assert::AreEqual(4, vmListPage.GetSelectedItemIndex());
+        Assert::AreEqual({ 0 }, vmListPage.GetScrollOffset());
+
+        // five visible items, list should start scrolling now
+        Assert::IsTrue(vmListPage.ProcessInput(pInput));
+        Assert::AreEqual(5, vmListPage.GetSelectedItemIndex());
+        Assert::AreEqual({ 1 }, vmListPage.GetScrollOffset());
+
+        Assert::IsTrue(vmListPage.ProcessInput(pInput));
+        Assert::AreEqual(7, vmListPage.GetSelectedItemIndex());
+        Assert::AreEqual({ 3 }, vmListPage.GetScrollOffset());
+
+        Assert::IsTrue(vmListPage.ProcessInput(pInput));
+        Assert::AreEqual(8, vmListPage.GetSelectedItemIndex());
+        Assert::AreEqual({ 4 }, vmListPage.GetScrollOffset());
+
+        Assert::IsTrue(vmListPage.ProcessInput(pInput));
+        Assert::AreEqual(9, vmListPage.GetSelectedItemIndex());
+        Assert::AreEqual({ 5 }, vmListPage.GetScrollOffset());
+
+        Assert::IsTrue(vmListPage.ProcessInput(pInput));
+        Assert::AreEqual(10, vmListPage.GetSelectedItemIndex());
+        Assert::AreEqual({ 6 }, vmListPage.GetScrollOffset());
+
+        Assert::IsTrue(vmListPage.ProcessInput(pInput));
+        Assert::AreEqual(11, vmListPage.GetSelectedItemIndex());
+        Assert::AreEqual({ 7 }, vmListPage.GetScrollOffset());
+
+        Assert::IsTrue(vmListPage.ProcessInput(pInput));
+        Assert::AreEqual(12, vmListPage.GetSelectedItemIndex());
+        Assert::AreEqual({ 8 }, vmListPage.GetScrollOffset());
+
+        // end of list
+        Assert::IsFalse(vmListPage.ProcessInput(pInput));
+        Assert::AreEqual(12, vmListPage.GetSelectedItemIndex());
+        Assert::AreEqual({ 8 }, vmListPage.GetScrollOffset());
+
+        // going up shouldn't immediately update the scroll offset
+        pInput.m_bDownPressed = false;
+        pInput.m_bUpPressed = true;
+        Assert::IsTrue(vmListPage.ProcessInput(pInput));
+        Assert::AreEqual(11, vmListPage.GetSelectedItemIndex());
+        Assert::AreEqual({ 8 }, vmListPage.GetScrollOffset());
+
+        Assert::IsTrue(vmListPage.ProcessInput(pInput));
+        Assert::AreEqual(10, vmListPage.GetSelectedItemIndex());
+        Assert::AreEqual({ 8 }, vmListPage.GetScrollOffset());
+
+        Assert::IsTrue(vmListPage.ProcessInput(pInput));
+        Assert::AreEqual(9, vmListPage.GetSelectedItemIndex());
+        Assert::AreEqual({ 8 }, vmListPage.GetScrollOffset());
+
+        Assert::IsTrue(vmListPage.ProcessInput(pInput));
+        Assert::AreEqual(8, vmListPage.GetSelectedItemIndex());
+        Assert::AreEqual({ 8 }, vmListPage.GetScrollOffset());
+
+        // now we should start scrolling again
+        Assert::IsTrue(vmListPage.ProcessInput(pInput));
+        Assert::AreEqual(7, vmListPage.GetSelectedItemIndex());
+        Assert::AreEqual({ 7 }, vmListPage.GetScrollOffset());
+
+        Assert::IsTrue(vmListPage.ProcessInput(pInput));
+        Assert::AreEqual(5, vmListPage.GetSelectedItemIndex());
+        Assert::AreEqual({ 5 }, vmListPage.GetScrollOffset());
+
+        Assert::IsTrue(vmListPage.ProcessInput(pInput));
+        Assert::AreEqual(4, vmListPage.GetSelectedItemIndex());
+        Assert::AreEqual({ 4 }, vmListPage.GetScrollOffset());
+
+        Assert::IsTrue(vmListPage.ProcessInput(pInput));
+        Assert::AreEqual(2, vmListPage.GetSelectedItemIndex());
+        Assert::AreEqual({ 2 }, vmListPage.GetScrollOffset());
+
+        Assert::IsTrue(vmListPage.ProcessInput(pInput));
+        Assert::AreEqual(1, vmListPage.GetSelectedItemIndex());
+        Assert::AreEqual({ 1 }, vmListPage.GetScrollOffset());
+
+        Assert::IsTrue(vmListPage.ProcessInput(pInput));
+        Assert::AreEqual(1, vmListPage.GetSelectedItemIndex());
+        Assert::AreEqual({ 0 }, vmListPage.GetScrollOffset());
+
+        // end of list
+        Assert::IsFalse(vmListPage.ProcessInput(pInput));
+        Assert::AreEqual(1, vmListPage.GetSelectedItemIndex());
+        Assert::AreEqual({ 0 }, vmListPage.GetScrollOffset());
+    }
+
+    TEST_METHOD(TestRefreshFewer)
+    {
+        OverlayListPageViewModelHarness vmListPage;
+        vmListPage.RefreshWithHeader(); // 13 items
+
+        // scroll to bottom
+        ControllerInput pInput{};
+        pInput.m_bDownPressed = true;
+        for (size_t i = 0; i < vmListPage.GetItemCount(); ++i)
+            vmListPage.ProcessInput(pInput);
+
+        // add some items
+        for (int i = 13; i < 20; ++i)
+            vmListPage.AddItem(i, std::to_wstring(i));
+
+        Assert::AreEqual(12, vmListPage.GetSelectedItemIndex());
+        Assert::AreEqual({ 8 }, vmListPage.GetScrollOffset());
+
+        // reset to 13 items - cursor position should be remembered
+        vmListPage.RefreshWithHeader();
+        Assert::AreEqual(12, vmListPage.GetSelectedItemIndex());
+        Assert::AreEqual({ 8 }, vmListPage.GetScrollOffset());
+
+        // add some items and scroll down a couple
+        for (int i = 13; i < 20; ++i)
+            vmListPage.AddItem(i, std::to_wstring(i));
+        vmListPage.ProcessInput(pInput);
+        vmListPage.ProcessInput(pInput);
+        vmListPage.ProcessInput(pInput);
+
+        // reset to 13 items - cursor position is invalid, so it should be reset
+        vmListPage.RefreshWithHeader();
+        Assert::AreEqual(1, vmListPage.GetSelectedItemIndex());
+        Assert::AreEqual({ 0 }, vmListPage.GetScrollOffset());
+    }
+
     TEST_METHOD(TestProcessInputDetailToggle)
     {
         OverlayListPageViewModelHarness vmListPage;