Repository: RetroAchievements/RAIntegration
Pull request #854 (2022-01-03 21:50:56)

use viewmodel to convert entered values to string
Prevents exception when viewing the tooltip of an offset of a condition following an AddAddress after editing (but not changing) it.

For example, if the offset is `0x000c`, and you click on it to edit it, but click away without changing it, the offset would change to `0x0c`, which triggers rebuilding the achievement. Similar to #851, since the achievement didn't actually change, this leaves the achievement in an invalid state and trying to derive the indirect tooltip fails.


Patch:
diff --git a/src/ui/viewmodels/AssetEditorViewModel.cpp b/src/ui/viewmodels/AssetEditorViewModel.cpp
index 02662b39..788fdd44 100644
--- a/src/ui/viewmodels/AssetEditorViewModel.cpp
+++ b/src/ui/viewmodels/AssetEditorViewModel.cpp
@@ -695,7 +695,18 @@ void AssetEditorViewModel::OnTriggerChanged()
     }
 
     if (nSize < 0)
+    {
         m_bIgnoreTriggerUpdate = false;
+    }
+    else
+    {
+        auto* pGroup = Trigger().Groups().GetItemAt(Trigger().GetSelectedGroupIndex());
+        if (pGroup && pGroup->m_pConditionSet == nullptr)
+        {
+            // if the trigger didn't actually change, UpdateTriggerBinding won't get called, forcibly do so now.
+            UpdateTriggerBinding();
+        }
+    }
 }
 
 void AssetEditorViewModel::UpdateTriggerBinding()
diff --git a/src/ui/viewmodels/TriggerConditionViewModel.cpp b/src/ui/viewmodels/TriggerConditionViewModel.cpp
index cd39350e..117b68aa 100644
--- a/src/ui/viewmodels/TriggerConditionViewModel.cpp
+++ b/src/ui/viewmodels/TriggerConditionViewModel.cpp
@@ -263,7 +263,7 @@ void TriggerConditionViewModel::SerializeAppendOperand(std::string& sBuffer, Tri
     }
 }
 
-std::wstring FormatValue(rc_typed_value_t& pValue, TriggerOperandType nType)
+static std::wstring FormatTypedValue(rc_typed_value_t& pValue, TriggerOperandType nType)
 {
     switch (nType)
     {
@@ -298,6 +298,22 @@ std::wstring FormatValue(rc_typed_value_t& pValue, TriggerOperandType nType)
     }
 }
 
+std::wstring TriggerConditionViewModel::FormatValue(unsigned nValue, TriggerOperandType nType)
+{
+    rc_typed_value_t pValue;
+    pValue.type = RC_VALUE_TYPE_UNSIGNED;
+    pValue.value.u32 = nValue;
+    return FormatTypedValue(pValue, nType);
+}
+
+std::wstring TriggerConditionViewModel::FormatValue(float fValue, TriggerOperandType nType)
+{
+    rc_typed_value_t pValue;
+    pValue.type = RC_VALUE_TYPE_FLOAT;
+    pValue.value.f32 = fValue;
+    return FormatTypedValue(pValue, nType);
+}
+
 void TriggerConditionViewModel::ChangeOperandType(const StringModelProperty& sValueProperty, TriggerOperandType nOldType, TriggerOperandType nNewType)
 {
     const auto& sValue = GetValue(sValueProperty);
@@ -318,7 +334,7 @@ void TriggerConditionViewModel::ChangeOperandType(const StringModelProperty& sVa
             ra::ParseUnsignedInt(sValue, 0xFFFFFFFF, pValue.value.u32, sError);
     }
 
-    SetValue(sValueProperty, FormatValue(pValue, nNewType));
+    SetValue(sValueProperty, FormatTypedValue(pValue, nNewType));
 }
 
 void TriggerConditionViewModel::SetOperand(const IntModelProperty& pTypeProperty,
@@ -360,7 +376,7 @@ void TriggerConditionViewModel::SetOperand(const IntModelProperty& pTypeProperty
             break;
     }
 
-    SetValue(pValueProperty, FormatValue(pValue, nType));
+    SetValue(pValueProperty, FormatTypedValue(pValue, nType));
 }
 
 void TriggerConditionViewModel::InitializeFrom(const rc_condition_t& pCondition)
@@ -483,10 +499,7 @@ void TriggerConditionViewModel::OnValueChanged(const BoolModelProperty::ChangeAr
 
 void TriggerConditionViewModel::SetSourceValue(unsigned int nValue)
 {
-    rc_typed_value_t pValue;
-    pValue.type = RC_VALUE_TYPE_UNSIGNED;
-    pValue.value.u32 = nValue;
-    SetValue(SourceValueProperty, FormatValue(pValue, GetSourceType()));
+    SetValue(SourceValueProperty, FormatValue(nValue, GetSourceType()));
 }
 
 ra::ByteAddress TriggerConditionViewModel::GetSourceAddress() const
@@ -499,18 +512,12 @@ ra::ByteAddress TriggerConditionViewModel::GetSourceAddress() const
 
 void TriggerConditionViewModel::SetTargetValue(unsigned int nValue)
 {
-    rc_typed_value_t pValue;
-    pValue.type = RC_VALUE_TYPE_UNSIGNED;
-    pValue.value.u32 = nValue;
-    SetValue(TargetValueProperty, FormatValue(pValue, GetTargetType()));
+    SetValue(TargetValueProperty, FormatValue(nValue, GetTargetType()));
 }
 
 void TriggerConditionViewModel::SetTargetValue(float fValue)
 {
-    rc_typed_value_t pValue;
-    pValue.type = RC_VALUE_TYPE_FLOAT;
-    pValue.value.f32 = fValue;
-    SetValue(TargetValueProperty, FormatValue(pValue, GetTargetType()));
+    SetValue(TargetValueProperty, FormatValue(fValue, GetTargetType()));
 }
 
 ra::ByteAddress TriggerConditionViewModel::GetTargetAddress() const
diff --git a/src/ui/viewmodels/TriggerConditionViewModel.hh b/src/ui/viewmodels/TriggerConditionViewModel.hh
index 4563439b..6ea78338 100644
--- a/src/ui/viewmodels/TriggerConditionViewModel.hh
+++ b/src/ui/viewmodels/TriggerConditionViewModel.hh
@@ -136,6 +136,8 @@ public:
     bool IsModifying() const { return IsModifying(GetType()); }
 
     static bool IsComparisonVisible(const ViewModelBase& vmItem, int nValue);
+    static std::wstring FormatValue(unsigned nValue, TriggerOperandType nType);
+    static std::wstring FormatValue(float fValue, TriggerOperandType nType);
 
     static const IntModelProperty RowColorProperty;
     Color GetRowColor() const { return Color(ra::to_unsigned(GetValue(RowColorProperty))); }
diff --git a/src/ui/win32/AssetEditorDialog.cpp b/src/ui/win32/AssetEditorDialog.cpp
index 7f2e2c8f..5bd04c5a 100644
--- a/src/ui/win32/AssetEditorDialog.cpp
+++ b/src/ui/win32/AssetEditorDialog.cpp
@@ -154,7 +154,8 @@ public:
                     return false;
                 }
 
-                vmItems.SetItemValue(nIndex, *m_pBoundProperty, sValue);
+                vmItems.SetItemValue(nIndex, *m_pBoundProperty,
+                    ra::ui::viewmodels::TriggerConditionViewModel::FormatValue(fValue, nOperandType));
                 return true;
             }
 
@@ -172,7 +173,8 @@ public:
                         return false;
                     }
 
-                    vmItems.SetItemValue(nIndex, *m_pBoundProperty, sValue);
+                    vmItems.SetItemValue(nIndex, *m_pBoundProperty,
+                        ra::ui::viewmodels::TriggerConditionViewModel::FormatValue(nValue, nOperandType));
                     return true;
                 }
             }
@@ -189,7 +191,9 @@ public:
                     return false;
                 }
 
-                vmItems.SetItemValue(nIndex, *m_pBoundProperty, ra::StringPrintf(L"0x%02x", nValue));
+                vmItems.SetItemValue(nIndex, *m_pBoundProperty,
+                    ra::ui::viewmodels::TriggerConditionViewModel::FormatValue(nValue, nOperandType));
+
                 return true;
             }
         }


Test patch:
diff --git a/tests/ui/viewmodels/AssetEditorViewModel_Tests.cpp b/tests/ui/viewmodels/AssetEditorViewModel_Tests.cpp
index 54e212ca..af4882fc 100644
--- a/tests/ui/viewmodels/AssetEditorViewModel_Tests.cpp
+++ b/tests/ui/viewmodels/AssetEditorViewModel_Tests.cpp
@@ -1563,6 +1563,63 @@ public:
         Assert::IsNotNull(group.m_pConditionSet);
     }
 
+    TEST_METHOD(TestTriggerUnmodifiedUpdate)
+    {
+        AssetEditorViewModelHarness editor;
+        editor.mockConfiguration.SetFeatureEnabled(ra::services::Feature::PreferDecimal, true);
+        AchievementModel achievement;
+        achievement.SetID(1234U);
+        achievement.SetTrigger("0xH1234=1_0xH1235=2");
+        achievement.CreateServerCheckpoint();
+        achievement.CreateLocalCheckpoint();
+        achievement.Activate();
+
+        editor.LoadAsset(&achievement);
+
+        // make sure the record got loaded into the runtime
+        const auto* pTrigger = editor.mockRuntime.GetAchievementTrigger(1234U);
+        Expects(pTrigger != nullptr);
+
+        Assert::AreEqual({ 2U }, editor.Trigger().Conditions().Count());
+        editor.Trigger().Conditions().GetItemAt(1)->SetSelected(true);
+        editor.Trigger().MoveSelectedConditionsUp();
+
+        // make sure the trigger definition got updated
+        Assert::AreEqual(std::string("0xH1235=2_0xH1234=1"), achievement.GetTrigger());
+        Assert::IsNotNull(editor.Trigger().Groups().GetItemAt(0)->m_pConditionSet);
+
+        // make sure the UI picked up the updated value
+        Assert::AreEqual({ 2U }, editor.Trigger().Conditions().Count());
+        const auto* pCondition = editor.Trigger().Conditions().GetItemAt(0);
+        Expects(pCondition != nullptr);
+        Assert::AreEqual(std::wstring(L"0x1235"), pCondition->GetSourceValue());
+        Assert::AreEqual(std::wstring(L"2"), pCondition->GetTargetValue());
+        pCondition = editor.Trigger().Conditions().GetItemAt(1);
+        Expects(pCondition != nullptr);
+        Assert::AreEqual(std::wstring(L"0x1234"), pCondition->GetSourceValue());
+        Assert::AreEqual(std::wstring(L"1"), pCondition->GetTargetValue());
+
+        // second attempt to move will not actually do anything
+        Assert::IsTrue(editor.Trigger().Conditions().GetItemAt(0)->IsSelected());
+        Assert::IsFalse(editor.Trigger().Conditions().GetItemAt(1)->IsSelected());
+        editor.Trigger().MoveSelectedConditionsUp();
+
+        // make sure the trigger definition did not update
+        Assert::AreEqual(std::string("0xH1235=2_0xH1234=1"), achievement.GetTrigger());
+        Assert::IsNotNull(editor.Trigger().Groups().GetItemAt(0)->m_pConditionSet);
+
+        // make sure the UI did not update
+        Assert::AreEqual({ 2U }, editor.Trigger().Conditions().Count());
+        pCondition = editor.Trigger().Conditions().GetItemAt(0);
+        Expects(pCondition != nullptr);
+        Assert::AreEqual(std::wstring(L"0x1235"), pCondition->GetSourceValue());
+        Assert::AreEqual(std::wstring(L"2"), pCondition->GetTargetValue());
+        pCondition = editor.Trigger().Conditions().GetItemAt(1);
+        Expects(pCondition != nullptr);
+        Assert::AreEqual(std::wstring(L"0x1234"), pCondition->GetSourceValue());
+        Assert::AreEqual(std::wstring(L"1"), pCondition->GetTargetValue());
+    }
+
     TEST_METHOD(TestValueUpdated)
     {
         AssetEditorViewModelHarness editor;
diff --git a/tests/ui/viewmodels/TriggerConditionViewModel_Tests.cpp b/tests/ui/viewmodels/TriggerConditionViewModel_Tests.cpp
index de4388d4..e55f249b 100644
--- a/tests/ui/viewmodels/TriggerConditionViewModel_Tests.cpp
+++ b/tests/ui/viewmodels/TriggerConditionViewModel_Tests.cpp
@@ -995,6 +995,73 @@ public:
         Assert::IsFalse(TriggerConditionViewModel::IsComparisonVisible(condition, ra::etoi(TriggerOperatorType::BitwiseAnd)));
     }
 
+    TEST_METHOD(TestFormatValueNumber)
+    {
+        TriggerConditionViewModelHarness condition;
+        condition.mockConfiguration.SetFeatureEnabled(ra::services::Feature::PreferDecimal, true);
+
+        Assert::AreEqual(std::wstring(L"0"), TriggerConditionViewModel::FormatValue(0U, TriggerOperandType::Value));
+        Assert::AreEqual(std::wstring(L"1"), TriggerConditionViewModel::FormatValue(1U, TriggerOperandType::Value));
+        Assert::AreEqual(std::wstring(L"10"), TriggerConditionViewModel::FormatValue(10U, TriggerOperandType::Value));
+        Assert::AreEqual(std::wstring(L"100"), TriggerConditionViewModel::FormatValue(100U, TriggerOperandType::Value));
+        Assert::AreEqual(std::wstring(L"1000000000"), TriggerConditionViewModel::FormatValue(1000000000U, TriggerOperandType::Value));
+        Assert::AreEqual(std::wstring(L"4294967295"), TriggerConditionViewModel::FormatValue(0xFFFFFFFFU, TriggerOperandType::Value));
+        Assert::AreEqual(std::wstring(L"12"), TriggerConditionViewModel::FormatValue(12.34f, TriggerOperandType::Value));
+
+        condition.mockConfiguration.SetFeatureEnabled(ra::services::Feature::PreferDecimal, false);
+
+        Assert::AreEqual(std::wstring(L"0x00"), TriggerConditionViewModel::FormatValue(0U, TriggerOperandType::Value));
+        Assert::AreEqual(std::wstring(L"0x01"), TriggerConditionViewModel::FormatValue(1U, TriggerOperandType::Value));
+        Assert::AreEqual(std::wstring(L"0x0a"), TriggerConditionViewModel::FormatValue(10U, TriggerOperandType::Value));
+        Assert::AreEqual(std::wstring(L"0x64"), TriggerConditionViewModel::FormatValue(100U, TriggerOperandType::Value));
+        Assert::AreEqual(std::wstring(L"0x3b9aca00"), TriggerConditionViewModel::FormatValue(1000000000U, TriggerOperandType::Value));
+        Assert::AreEqual(std::wstring(L"0xffffffff"), TriggerConditionViewModel::FormatValue(0xFFFFFFFFU, TriggerOperandType::Value));
+        Assert::AreEqual(std::wstring(L"0x0c"), TriggerConditionViewModel::FormatValue(12.34f, TriggerOperandType::Value));
+    }
+
+    TEST_METHOD(TestFormatValueFloat)
+    {
+        TriggerConditionViewModelHarness condition;
+        condition.mockConfiguration.SetFeatureEnabled(ra::services::Feature::PreferDecimal, true);
+
+        Assert::AreEqual(std::wstring(L"0.0"), TriggerConditionViewModel::FormatValue(0.0f, TriggerOperandType::Float));
+        Assert::AreEqual(std::wstring(L"1.0"), TriggerConditionViewModel::FormatValue(1.0f, TriggerOperandType::Float));
+        Assert::AreEqual(std::wstring(L"1.23"), TriggerConditionViewModel::FormatValue(1.23f, TriggerOperandType::Float));
+        Assert::AreEqual(std::wstring(L"-3.14159"), TriggerConditionViewModel::FormatValue(-3.14159f, TriggerOperandType::Float));
+        Assert::AreEqual(std::wstring(L"960.75"), TriggerConditionViewModel::FormatValue(960.75f, TriggerOperandType::Float));
+
+        condition.mockConfiguration.SetFeatureEnabled(ra::services::Feature::PreferDecimal, false);
+
+        Assert::AreEqual(std::wstring(L"0.0"), TriggerConditionViewModel::FormatValue(0.0f, TriggerOperandType::Float));
+        Assert::AreEqual(std::wstring(L"1.0"), TriggerConditionViewModel::FormatValue(1.0f, TriggerOperandType::Float));
+        Assert::AreEqual(std::wstring(L"1.23"), TriggerConditionViewModel::FormatValue(1.23f, TriggerOperandType::Float));
+        Assert::AreEqual(std::wstring(L"-3.14159"), TriggerConditionViewModel::FormatValue(-3.14159f, TriggerOperandType::Float));
+        Assert::AreEqual(std::wstring(L"960.75"), TriggerConditionViewModel::FormatValue(960.75f, TriggerOperandType::Float));
+    }
+
+    TEST_METHOD(TestFormatValueAddress)
+    {
+        TriggerConditionViewModelHarness condition;
+        condition.mockConfiguration.SetFeatureEnabled(ra::services::Feature::PreferDecimal, true);
+
+        Assert::AreEqual(std::wstring(L"0x0000"), TriggerConditionViewModel::FormatValue(0U, TriggerOperandType::Address));
+        Assert::AreEqual(std::wstring(L"0x1234"), TriggerConditionViewModel::FormatValue(4660U, TriggerOperandType::Address));
+        Assert::AreEqual(std::wstring(L"0xc3500"), TriggerConditionViewModel::FormatValue(800000U, TriggerOperandType::Address));
+        Assert::AreEqual(std::wstring(L"0x1234"), TriggerConditionViewModel::FormatValue(4660U, TriggerOperandType::Delta));
+        Assert::AreEqual(std::wstring(L"0x1234"), TriggerConditionViewModel::FormatValue(4660U, TriggerOperandType::Prior));
+        Assert::AreEqual(std::wstring(L"0x1234"), TriggerConditionViewModel::FormatValue(4660U, TriggerOperandType::BCD));
+        Assert::AreEqual(std::wstring(L"0x1234"), TriggerConditionViewModel::FormatValue(4660.25f, TriggerOperandType::Address));
+
+        condition.mockConfiguration.SetFeatureEnabled(ra::services::Feature::PreferDecimal, false);
+
+        Assert::AreEqual(std::wstring(L"0x0000"), TriggerConditionViewModel::FormatValue(0U, TriggerOperandType::Address));
+        Assert::AreEqual(std::wstring(L"0x1234"), TriggerConditionViewModel::FormatValue(4660U, TriggerOperandType::Address));
+        Assert::AreEqual(std::wstring(L"0xc3500"), TriggerConditionViewModel::FormatValue(800000U, TriggerOperandType::Address));
+        Assert::AreEqual(std::wstring(L"0x1234"), TriggerConditionViewModel::FormatValue(4660U, TriggerOperandType::Delta));
+        Assert::AreEqual(std::wstring(L"0x1234"), TriggerConditionViewModel::FormatValue(4660U, TriggerOperandType::Prior));
+        Assert::AreEqual(std::wstring(L"0x1234"), TriggerConditionViewModel::FormatValue(4660U, TriggerOperandType::BCD));
+        Assert::AreEqual(std::wstring(L"0x1234"), TriggerConditionViewModel::FormatValue(4660.25f, TriggerOperandType::Address));
+    }
 };
 
 } // namespace tests