Repository: RetroAchievements/RAIntegration
Pull request #999 (2023-05-11 00:58:51)

ensure popup surfaces exist when overlay is fading in/out
Fixes a crash (assert in PopupViewModelBase.hh:136) when a popup is modified while the overlay is open.

Steps to reproduce:
1) Create an achievement that measures a memory value
2) Modify the memory value to cause the progress tracker to appear
3) Open the overlay before the progress tracker disappears
4) Modify the memory value to cause the progress tracker to update
5) Close the overlay


Patch:
diff --git a/src/ui/viewmodels/OverlayManager.cpp b/src/ui/viewmodels/OverlayManager.cpp
index 4e530dca..e3a339be 100644
--- a/src/ui/viewmodels/OverlayManager.cpp
+++ b/src/ui/viewmodels/OverlayManager.cpp
@@ -132,84 +132,94 @@ void OverlayManager::Render(ra::ui::drawing::ISurface& pSurface, bool bRedrawAll
     m_bRenderRequestPending = false;
     m_bRedrawAll = bRedrawAll;
 
-    auto& pClock = ra::services::ServiceLocator::Get<ra::services::IClock>();
-    const auto tNow = pClock.UpTime();
-    const auto tElapsed = std::chrono::duration_cast<std::chrono::microseconds>(tNow - m_tLastRender);
-    const double fElapsed = tElapsed.count() / 1000000.0;
-
     bool bRequestRender = false;
-
-    if (m_vmOverlay.CurrentState() == OverlayViewModel::State::Hidden)
     {
         std::lock_guard<std::mutex> pGuard(m_pPopupQueueMutex);
 
-        PopupLocations pPopupLocations;
-        memset(&pPopupLocations, 0, sizeof(pPopupLocations));
-
-        // layout pass - stack items in desired order
+        auto& pClock = ra::services::ServiceLocator::Get<ra::services::IClock>();
+        const auto tNow = pClock.UpTime();
+        const auto tElapsed = std::chrono::duration_cast<std::chrono::microseconds>(tNow - m_tLastRender);
+        const double fElapsedReal = tElapsed.count() / 1000000.0;
 
-        if (!m_vPopupMessages.empty())
+        const auto nState = m_vmOverlay.CurrentState();
+        if (nState != OverlayViewModel::State::Visible)
         {
-            UpdateActiveMessage(pSurface, pPopupLocations, fElapsed);
+            // if the overlay isn't fully hidden, don't animate the popups.
+            // do update them though to ensure the surfaces are initialized.
+            const double fElapsed = (nState == OverlayViewModel::State::Hidden) ? fElapsedReal : 0.0;
 
-            // a visible popup may not set m_bRedrawAll if it's in the pause portion of its animation loop
-            // make sure we keep the render cycle going so it'll eventually handle when the pause ends
-            bRequestRender = true;
-        }
+            PopupLocations pPopupLocations;
+            memset(&pPopupLocations, 0, sizeof(pPopupLocations));
 
-        if (!m_vScoreboards.empty())
-        {
-            UpdateActiveScoreboard(pSurface, pPopupLocations, fElapsed);
+            // layout pass - stack items in desired order
 
-            // a visible scoreboard may not set m_bRedrawAll if it's in the pause portion of its animation loop
-            // make sure we keep the render cycle going so it'll eventually handle when the pause ends
-            bRequestRender = true;
-        }
+            if (!m_vPopupMessages.empty())
+            {
+                UpdateActiveMessage(pSurface, pPopupLocations, fElapsed);
 
-        if (!m_vScoreTrackers.empty())
-            UpdateScoreTrackers(pSurface, pPopupLocations, fElapsed);
+                // a visible popup may not set m_bRedrawAll if it's in the pause portion of its animation loop
+                // make sure we keep the render cycle going so it'll eventually handle when the pause ends
+                bRequestRender = true;
+            }
 
-        if (!m_vChallengeIndicators.empty())
-            UpdateChallengeIndicators(pSurface, pPopupLocations, fElapsed);
+            if (!m_vScoreboards.empty())
+            {
+                UpdateActiveScoreboard(pSurface, pPopupLocations, fElapsed);
 
-        if (m_vmProgressTracker != nullptr)
-        {
-            UpdateProgressTracker(pSurface, pPopupLocations, fElapsed);
+                // a visible scoreboard may not set m_bRedrawAll if it's in the pause portion of its animation loop
+                // make sure we keep the render cycle going so it'll eventually handle when the pause ends
+                bRequestRender = true;
+            }
 
-            // a visible progress tracker may not set m_bRedrawAll if it's in the pause portion of its animation loop
-            // make sure we keep the render cycle going so it'll eventually handle when the pause ends
-            bRequestRender = true;
+            if (!m_vScoreTrackers.empty())
+                UpdateScoreTrackers(pSurface, pPopupLocations, fElapsed);
+
+            if (!m_vChallengeIndicators.empty())
+                UpdateChallengeIndicators(pSurface, pPopupLocations, fElapsed);
+
+            if (m_vmProgressTracker != nullptr)
+            {
+                UpdateProgressTracker(pSurface, pPopupLocations, fElapsed);
+
+                // a visible progress tracker may not set m_bRedrawAll if it's in the pause portion of its animation
+                // loop make sure we keep the render cycle going so it'll eventually handle when the pause ends
+                bRequestRender = true;
+            }
         }
 
-        // if anything changed, or caller requested a repaint, do so now
-        if (m_bRedrawAll)
+        if (nState == OverlayViewModel::State::Hidden)
         {
-            // render pass - items that should appear over other items should be drawn last
-            if (!m_vScoreboards.empty())
-                RenderPopup(pSurface, m_vScoreboards.front());
-            for (const auto& pScoreTracker : m_vScoreTrackers)
-                RenderPopup(pSurface, *pScoreTracker);
-            for (const auto& pChallengeIndicator : m_vChallengeIndicators)
-                RenderPopup(pSurface, *pChallengeIndicator);
-            if (m_vmProgressTracker != nullptr)
-                RenderPopup(pSurface, *m_vmProgressTracker);
-            if (!m_vPopupMessages.empty())
-                RenderPopup(pSurface, *m_vPopupMessages.front());
+            // popups are unobscured. if anything changed, or caller requested a repaint, do so now
+            if (m_bRedrawAll)
+            {
+                // render pass - items that should appear over other items should be drawn last
+                if (!m_vScoreboards.empty())
+                    RenderPopup(pSurface, m_vScoreboards.front());
+                for (const auto& pScoreTracker : m_vScoreTrackers)
+                    RenderPopup(pSurface, *pScoreTracker);
+                for (const auto& pChallengeIndicator : m_vChallengeIndicators)
+                    RenderPopup(pSurface, *pChallengeIndicator);
+                if (m_vmProgressTracker != nullptr)
+                    RenderPopup(pSurface, *m_vmProgressTracker);
+                if (!m_vPopupMessages.empty())
+                    RenderPopup(pSurface, *m_vPopupMessages.front());
 
-            bRequestRender = true;
+                bRequestRender = true;
+            }
         }
-    }
-    else
-    {
-        UpdateOverlay(pSurface, fElapsed);
+        else
+        {
+            // popups are obscured. draw any partially visible ones, then the overlay on top
+            UpdateOverlay(pSurface, fElapsedReal);
 
-        if (m_vmOverlay.CurrentState() != OverlayViewModel::State::Visible)
-            bRequestRender = true;
-    }
+            // only request another render if the overlay is fading in or out (may have changed in UpdateOverlay)
+            bRequestRender = (m_vmOverlay.CurrentState() != OverlayViewModel::State::Visible);
+        }
 
-    // update state now, in case handlers cause recursion
-    m_tLastRender = tNow;
-    m_bRedrawAll = false;
+        // update state now, in case handlers cause recursion
+        m_tLastRender = tNow;
+        m_bRedrawAll = false;
+    }
 
     if (bRequestRender)
     {


Test patch:
diff --git a/tests/api/ConnectedServer_Tests.cpp b/tests/api/ConnectedServer_Tests.cpp
index 0e4e8b94..fb8f1898 100644
--- a/tests/api/ConnectedServer_Tests.cpp
+++ b/tests/api/ConnectedServer_Tests.cpp
@@ -295,6 +295,30 @@ public:
         Assert::AreEqual(std::string("HTTP error code: 429 (err429)"), response.ErrorMessage);
     }
 
+    TEST_METHOD(TestAwardAchievementFailed429Json)
+    {
+        MockUserContext mockUserContext;
+        mockUserContext.Initialize("Username", "ApiToken");
+
+        MockHttpRequester mockHttp([]([[maybe_unused]] const Http::Request& /*request*/)
+        {
+            return Http::Response(Http::StatusCode::TooManyRequests,
+                "{\"Success\": false,\"Error\": \"Too Many Attempts\"}");
+        });
+
+        ConnectedServer server("host.com");
+
+        AwardAchievement::Request request;
+        request.GameHash = "HASH";
+        request.AchievementId = 1234U;
+        request.Hardcore = true;
+
+        auto response = server.AwardAchievement(request);
+
+        Assert::AreEqual(ApiResult::Error, response.Result);
+        Assert::AreEqual(std::string("Too Many Attempts"), response.ErrorMessage);
+    }
+
     TEST_METHOD(TestLoginUnknownServer)
     {
         MockHttpRequester mockHttp([]([[maybe_unused]] const Http::Request& /*request*/)