Repository: RetroAchievements/RAIntegration
Pull request #851 (2021-12-26 18:36:39)

prevent crash viewing tooltip for address chain after modification
Ensures only one change event is raised when multiple properties of a condition change at the same time.

When changing a field from Value to a memory type, the size field is updated as well as the Type field. This generates two events for the trigger changing, but the serialized form did not actually change on the second event, leaving the backing data uninitialized. When trying to view the Indirect tooltip without backing data, the application would crash.


Patch:
diff --git a/src/data/context/GameContext.cpp b/src/data/context/GameContext.cpp
index b30c58dd..6468e283 100644
--- a/src/data/context/GameContext.cpp
+++ b/src/data/context/GameContext.cpp
@@ -46,7 +46,7 @@ void GameContext::LoadGame(unsigned int nGameId, Mode nMode)
 {
     // remove the current asset from the asset editor
     auto& vmWindowManager = ra::services::ServiceLocator::GetMutable<ra::ui::viewmodels::WindowManager>();
-    vmWindowManager.AssetEditor.LoadAsset(nullptr);
+    vmWindowManager.AssetEditor.LoadAsset(nullptr, true);
 
     // reset the runtime
     auto& pRuntime = ra::services::ServiceLocator::GetMutable<ra::services::AchievementRuntime>();
diff --git a/src/ui/viewmodels/AssetEditorViewModel.cpp b/src/ui/viewmodels/AssetEditorViewModel.cpp
index b11659b3..02662b39 100644
--- a/src/ui/viewmodels/AssetEditorViewModel.cpp
+++ b/src/ui/viewmodels/AssetEditorViewModel.cpp
@@ -122,12 +122,12 @@ void AssetEditorViewModel::SelectBadgeFile()
     SetBadge(ra::Widen(sBadgeName));
 }
 
-void AssetEditorViewModel::LoadAsset(ra::data::models::AssetModelBase* pAsset)
+void AssetEditorViewModel::LoadAsset(ra::data::models::AssetModelBase* pAsset, bool bForce)
 {
     if (m_pAsset == pAsset)
         return;
 
-    if (HasAssetValidationError() && m_pAsset->GetChanges() != ra::data::models::AssetChanges::Deleted)
+    if (!bForce && HasAssetValidationError() && m_pAsset->GetChanges() != ra::data::models::AssetChanges::Deleted)
     {
         if (ra::ui::viewmodels::MessageBoxViewModel::ShowWarningMessage(L"Discard changes?",
             L"The currently loaded asset has an error that cannot be saved. If you switch to another asset, your changes will be lost.",
diff --git a/src/ui/viewmodels/AssetEditorViewModel.hh b/src/ui/viewmodels/AssetEditorViewModel.hh
index 72bc021c..6266a271 100644
--- a/src/ui/viewmodels/AssetEditorViewModel.hh
+++ b/src/ui/viewmodels/AssetEditorViewModel.hh
@@ -413,7 +413,7 @@ public:
 
     // ===== Functions =====
 
-    void LoadAsset(ra::data::models::AssetModelBase* pAsset);
+    void LoadAsset(ra::data::models::AssetModelBase* pAsset, bool bForce = false);
     const ra::data::models::AssetModelBase* GetAsset() const noexcept { return m_pAsset; }
 
     void DoFrame();
diff --git a/src/ui/viewmodels/TriggerConditionViewModel.cpp b/src/ui/viewmodels/TriggerConditionViewModel.cpp
index 1f93fb2d..cd39350e 100644
--- a/src/ui/viewmodels/TriggerConditionViewModel.cpp
+++ b/src/ui/viewmodels/TriggerConditionViewModel.cpp
@@ -386,6 +386,11 @@ bool TriggerConditionViewModel::IsForValue() const noexcept
 
 void TriggerConditionViewModel::OnValueChanged(const IntModelProperty::ChangeArgs& args)
 {
+    // suspend the condition monitor so it only handles one change event regardless of how many properties we change
+    const auto* pTriggerViewModel = dynamic_cast<const TriggerViewModel*>(m_pTriggerViewModel);
+    if (pTriggerViewModel != nullptr)
+        pTriggerViewModel->SuspendConditionMonitor();
+
     if (args.Property == SourceTypeProperty)
     {
         const auto nOldType = ra::itoe<TriggerOperandType>(args.tOldValue);
@@ -439,6 +444,10 @@ void TriggerConditionViewModel::OnValueChanged(const IntModelProperty::ChangeArg
     }
 
     ViewModelBase::OnValueChanged(args);
+
+    // resume after calling base, as that's where the ConditionMonitor will handle the current event
+    if (pTriggerViewModel != nullptr)
+        pTriggerViewModel->ResumeConditionMonitor();
 }
 
 void TriggerConditionViewModel::UpdateHasHits()
@@ -584,6 +593,8 @@ unsigned int TriggerConditionViewModel::GetIndirectAddress(unsigned int nAddress
     rc_typed_value_t value = {};
     oEvalState.peek = rc_peek_callback;
 
+    Expects(pGroup->m_pConditionSet != nullptr);
+
     gsl::index nConditionIndex = 0;
     rc_condition_t* pCondition = pGroup->m_pConditionSet->conditions;
     for (; pCondition != nullptr; pCondition = pCondition->next)
diff --git a/src/ui/viewmodels/TriggerViewModel.cpp b/src/ui/viewmodels/TriggerViewModel.cpp
index 79542251..2c3968fc 100644
--- a/src/ui/viewmodels/TriggerViewModel.cpp
+++ b/src/ui/viewmodels/TriggerViewModel.cpp
@@ -639,6 +639,12 @@ void TriggerViewModel::ConditionsMonitor::OnEndViewModelCollectionUpdate()
 
 void TriggerViewModel::ConditionsMonitor::UpdateCurrentGroup()
 {
+    if (m_nUpdateCount > 0)
+    {
+        m_bUpdatePending = true;
+        return;
+    }
+
     auto* pGroup = m_vmTrigger->m_vGroups.GetItemAt(m_vmTrigger->GetSelectedGroupIndex());
     if (pGroup)
     {
@@ -647,6 +653,31 @@ void TriggerViewModel::ConditionsMonitor::UpdateCurrentGroup()
     }
 }
 
+void TriggerViewModel::ConditionsMonitor::EndUpdate()
+{
+    if (m_nUpdateCount > 0)
+    {
+        if (--m_nUpdateCount == 0)
+        {
+            if (m_bUpdatePending)
+            {
+                m_bUpdatePending = false;
+                UpdateCurrentGroup();
+            }
+        }
+    }
+}
+
+void TriggerViewModel::SuspendConditionMonitor() const noexcept
+{
+    m_pConditionsMonitor.BeginUpdate();
+}
+
+void TriggerViewModel::ResumeConditionMonitor() const
+{
+    m_pConditionsMonitor.EndUpdate();
+}
+
 void TriggerViewModel::UpdateVersion()
 {
     SetValue(VersionProperty, GetValue(VersionProperty) + 1);
@@ -810,7 +841,7 @@ void TriggerViewModel::AddGroup()
 {
     const int nGroups = gsl::narrow_cast<int>(m_vGroups.Count());
 
-    if (nGroups == 1)
+    if (nGroups == 1 && !m_bIsValue)
     {
         // only a core group, add two alt groups
         AddAltGroup(m_vGroups, nGroups, nullptr);
diff --git a/src/ui/viewmodels/TriggerViewModel.hh b/src/ui/viewmodels/TriggerViewModel.hh
index b993b9fa..e2a0328a 100644
--- a/src/ui/viewmodels/TriggerViewModel.hh
+++ b/src/ui/viewmodels/TriggerViewModel.hh
@@ -92,6 +92,9 @@ public:
     void InitializeFrom(const rc_value_t& pValue);
     void UpdateFrom(const rc_value_t& pValue);
 
+    void SuspendConditionMonitor() const noexcept;
+    void ResumeConditionMonitor() const;
+
     void CopyToClipboard();
 
     void RemoveSelectedConditions();
@@ -180,6 +183,9 @@ private:
             m_vmTrigger = &vmTrigger;
         }
 
+        void BeginUpdate() noexcept { ++m_nUpdateCount; }
+        void EndUpdate();
+
     protected:
         void OnViewModelIntValueChanged(gsl::index nIndex, const IntModelProperty::ChangeArgs& args) override;
         void OnViewModelStringValueChanged(gsl::index nIndex, const StringModelProperty::ChangeArgs& args) override;
@@ -190,9 +196,12 @@ private:
         void UpdateCurrentGroup();
 
         TriggerViewModel* m_vmTrigger;
+
+        int m_nUpdateCount = 0;
+        bool m_bUpdatePending = false;
     };
 
-    ConditionsMonitor m_pConditionsMonitor;
+    mutable ConditionsMonitor m_pConditionsMonitor;
 
     LookupItemViewModelCollection m_vConditionTypes;
     LookupItemViewModelCollection m_vOperandTypes;
diff --git a/src/ui/win32/AssetEditorDialog.cpp b/src/ui/win32/AssetEditorDialog.cpp
index 41ba38c1..7f2e2c8f 100644
--- a/src/ui/win32/AssetEditorDialog.cpp
+++ b/src/ui/win32/AssetEditorDialog.cpp
@@ -189,7 +189,7 @@ public:
                     return false;
                 }
 
-                vmItems.SetItemValue(nIndex, *m_pBoundProperty, sValue);
+                vmItems.SetItemValue(nIndex, *m_pBoundProperty, ra::StringPrintf(L"0x%02x", nValue));
                 return true;
             }
         }


Test patch:
diff --git a/tests/ui/viewmodels/AssetEditorViewModel_Tests.cpp b/tests/ui/viewmodels/AssetEditorViewModel_Tests.cpp
index c3e425db..54e212ca 100644
--- a/tests/ui/viewmodels/AssetEditorViewModel_Tests.cpp
+++ b/tests/ui/viewmodels/AssetEditorViewModel_Tests.cpp
@@ -2,6 +2,7 @@
 
 #include "ui\EditorTheme.hh"
 
+#include "ui\BindingBase.hh"
 #include "ui\viewmodels\AssetEditorViewModel.hh"
 #include "ui\viewmodels\FileDialogViewModel.hh"
 #include "ui\viewmodels\MessageBoxViewModel.hh"
@@ -80,8 +81,24 @@ private:
             SetValue(AssetValidationErrorProperty, sValue);
         }
 
+        int GetTriggerVersion()
+        {
+            TriggerVersionBinding binding(Trigger());
+            return binding.GetVersion();
+        }
+
     private:
         ra::services::ServiceLocator::ServiceOverride<ra::ui::EditorTheme> m_pMockThemeOverride;
+
+        class TriggerVersionBinding : public ui::BindingBase
+        {
+        public:
+            TriggerVersionBinding(TriggerViewModel& vmTrigger) noexcept : ui::BindingBase(vmTrigger)
+            {
+            }
+
+            int GetVersion() const { return GetValue(TriggerViewModel::VersionProperty); }
+        };
     };
 
 
@@ -578,6 +595,38 @@ public:
         Assert::AreEqual(std::wstring(), editor.GetAssetValidationWarning());
     }
 
+    TEST_METHOD(TestLoadAchievementForcedValidationError)
+    {
+        AssetEditorViewModelHarness editor;
+        AchievementModel achievement;
+        achievement.SetName(L"Test Achievement");
+        achievement.SetID(1234U);
+        achievement.SetState(AssetState::Active);
+        achievement.SetDescription(L"Do something cool");
+        achievement.SetCategory(AssetCategory::Unofficial);
+        achievement.SetPoints(10);
+        achievement.SetBadge(L"58329");
+        achievement.SetTrigger("M:0x1234=10");
+        achievement.CreateServerCheckpoint();
+        achievement.CreateLocalCheckpoint();
+
+        editor.LoadAsset(&achievement);
+        editor.SetAssetValidationError(L"Multiple Measured"); // not really, just fake it
+
+        Assert::IsTrue(editor.HasAssetValidationError());
+        Assert::AreEqual(std::wstring(L"Multiple Measured"), editor.GetAssetValidationError());
+
+        Assert::IsFalse(editor.HasAssetValidationWarning());
+        Assert::AreEqual(std::wstring(), editor.GetAssetValidationWarning());
+
+        editor.LoadAsset(nullptr, true);
+        Assert::IsFalse(editor.mockDesktop.WasDialogShown());
+        Assert::IsFalse(editor.HasAssetValidationError());
+        Assert::AreEqual(std::wstring(), editor.GetAssetValidationError());
+        Assert::IsFalse(editor.HasAssetValidationWarning());
+        Assert::AreEqual(std::wstring(), editor.GetAssetValidationWarning());
+    }
+
     TEST_METHOD(TestLoadLeaderboard)
     {
         AssetEditorViewModelHarness editor;
@@ -1479,6 +1528,41 @@ public:
         Assert::AreEqual(2U, pCondition->GetRequiredHits());
     }
 
+    TEST_METHOD(TestTriggerUpdatedValueToDelta)
+    {
+        // When a condition changes from value to delta, two properties are changed (type and size)
+        // Expect a single change notification and make sure the condition is referenced by the updated condition
+
+        AssetEditorViewModelHarness editor;
+        editor.mockConfiguration.SetFeatureEnabled(ra::services::Feature::PreferDecimal, true);
+        AchievementModel achievement;
+        achievement.SetID(1234U);
+        achievement.SetTrigger("0xH1234=1");
+        achievement.CreateServerCheckpoint();
+        achievement.CreateLocalCheckpoint();
+        achievement.Activate();
+
+        editor.LoadAsset(&achievement);
+
+        editor.Trigger().SetSelectedGroupIndex(0);
+        auto& condition = *editor.Trigger().Conditions().GetItemAt(0);
+        auto& group = *editor.Trigger().Groups().GetItemAt(0);
+        const auto initialVersion = editor.GetTriggerVersion();
+        Assert::IsNotNull(group.m_pConditionSet);
+
+        // change to address switches size to match source
+        condition.SetTargetType(TriggerOperandType::Delta);
+        Assert::AreEqual(std::string("0xH1234=d0xH0001"), achievement.GetTrigger());
+        Assert::AreEqual(initialVersion + 1, editor.GetTriggerVersion());
+        Assert::IsNotNull(group.m_pConditionSet);
+
+        // change to value sets size back to 32-bit
+        condition.SetTargetType(TriggerOperandType::Value);
+        Assert::AreEqual(std::string("0xH1234=1"), achievement.GetTrigger());
+        Assert::AreEqual(initialVersion + 2, editor.GetTriggerVersion());
+        Assert::IsNotNull(group.m_pConditionSet);
+    }
+
     TEST_METHOD(TestValueUpdated)
     {
         AssetEditorViewModelHarness editor;
diff --git a/tests/ui/viewmodels/TriggerConditionViewModel_Tests.cpp b/tests/ui/viewmodels/TriggerConditionViewModel_Tests.cpp
index 110fceaf..de4388d4 100644
--- a/tests/ui/viewmodels/TriggerConditionViewModel_Tests.cpp
+++ b/tests/ui/viewmodels/TriggerConditionViewModel_Tests.cpp
@@ -844,6 +844,28 @@ public:
         Assert::IsFalse(condition.IsModifying());
     }
 
+    TEST_METHOD(TestSwitchBetweenValueAndAddress)
+    {
+        TriggerConditionViewModelHarness condition;
+        condition.SetSourceValue(0x1234U);
+        condition.SetOperator(TriggerOperatorType::NotEquals);
+        condition.SetTargetValue(8U);
+        Assert::AreEqual(std::string("0xH1234!=8"), condition.Serialize());
+        Assert::IsFalse(condition.HasTargetSize());
+
+        // change to address switches size to match source
+        condition.SetTargetType(TriggerOperandType::Delta);
+        Assert::IsTrue(condition.HasTargetSize());
+        Assert::AreEqual(MemSize::EightBit, condition.GetTargetSize());
+        Assert::AreEqual(std::string("0xH1234!=d0xH0008"), condition.Serialize());
+
+        // change to value sets size back to 32-bit
+        condition.SetTargetType(TriggerOperandType::Value);
+        Assert::IsFalse(condition.HasTargetSize());
+        Assert::AreEqual(MemSize::ThirtyTwoBit, condition.GetTargetSize());
+        Assert::AreEqual(std::string("0xH1234!=8"), condition.Serialize());
+    }
+
     TEST_METHOD(TestChangeOperand)
     {
         TriggerConditionViewModelHarness condition;
diff --git a/tests/ui/viewmodels/TriggerViewModel_Tests.cpp b/tests/ui/viewmodels/TriggerViewModel_Tests.cpp
index ee008de1..8001bb8c 100644
--- a/tests/ui/viewmodels/TriggerViewModel_Tests.cpp
+++ b/tests/ui/viewmodels/TriggerViewModel_Tests.cpp
@@ -924,6 +924,24 @@ public:
         Assert::AreEqual(1, vmTrigger.GetVersion());
     }
 
+    TEST_METHOD(TestAddGroupNoAltsValue)
+    {
+        TriggerViewModelHarness vmTrigger;
+        vmTrigger.SetIsValue(true);
+        Parse(vmTrigger, "");
+        Assert::AreEqual({ 1U }, vmTrigger.Groups().Count());
+
+        vmTrigger.AddGroup();
+        Assert::AreEqual({ 2U }, vmTrigger.Groups().Count());
+
+        Assert::AreEqual(std::wstring(L"Value"), vmTrigger.Groups().GetItemAt(0)->GetLabel());
+        Assert::AreEqual(std::wstring(L"Alt 1"), vmTrigger.Groups().GetItemAt(1)->GetLabel());
+
+        Assert::AreEqual(1, vmTrigger.GetSelectedGroupIndex());
+        Assert::AreEqual(1, vmTrigger.GetEnsureVisibleGroupIndex());
+        Assert::AreEqual(1, vmTrigger.GetVersion());
+    }
+
     TEST_METHOD(TestRemoveGroupCore)
     {
         TriggerViewModelHarness vmTrigger;