Repository: RetroAchievements/RAIntegration
Pull request #846 (2021-12-12 03:26:08)

treat missing fields as warnings instead of errors


Patch:
diff --git a/src/data/models/LeaderboardModel.cpp b/src/data/models/LeaderboardModel.cpp
index 0f14d723..59183739 100644
--- a/src/data/models/LeaderboardModel.cpp
+++ b/src/data/models/LeaderboardModel.cpp
@@ -70,24 +70,46 @@ void LeaderboardModel::OnValueChanged(const IntModelProperty::ChangeArgs& args)
 
 bool LeaderboardModel::ValidateAsset(std::wstring& sError)
 {
-    const auto& sStartTrigger = GetLocalAssetDefinition(m_pStartTrigger);
+    const auto& sStartTrigger = GetAssetDefinition(m_pStartTrigger);
+    if (sStartTrigger.empty())
+    {
+        sError = L"No Start condition";
+        return false;
+    }
     if (!TriggerValidation::Validate(sStartTrigger, sError, AssetType::Leaderboard))
     {
         sError.insert(0, L"Start: ");
         return false;
     }
 
-    const auto& sSubmitTrigger = GetLocalAssetDefinition(m_pSubmitTrigger);
+    const auto& sCancelTrigger = GetAssetDefinition(m_pCancelTrigger);
+    if (sCancelTrigger.empty())
+    {
+        sError = L"No Cancel condition";
+        return false;
+    }
+    if (!TriggerValidation::Validate(sCancelTrigger, sError, AssetType::Leaderboard))
+    {
+        sError.insert(0, L"Cancel: ");
+        return false;
+    }
+
+    const auto& sSubmitTrigger = GetAssetDefinition(m_pSubmitTrigger);
+    if (sSubmitTrigger.empty())
+    {
+        sError = L"No Submit condition";
+        return false;
+    }
     if (!TriggerValidation::Validate(sSubmitTrigger, sError, AssetType::Leaderboard))
     {
         sError.insert(0, L"Submit: ");
         return false;
     }
 
-    const auto& sCancelTrigger = GetLocalAssetDefinition(m_pCancelTrigger);
-    if (!TriggerValidation::Validate(sCancelTrigger, sError, AssetType::Leaderboard))
+    const auto& sValueDefinition = GetAssetDefinition(m_pValueDefinition);
+    if (sValueDefinition.empty())
     {
-        sError.insert(0, L"Cancel: ");
+        sError = L"No Value definition";
         return false;
     }
 
diff --git a/src/ui/viewmodels/AssetEditorViewModel.cpp b/src/ui/viewmodels/AssetEditorViewModel.cpp
index 0f172a15..b11659b3 100644
--- a/src/ui/viewmodels/AssetEditorViewModel.cpp
+++ b/src/ui/viewmodels/AssetEditorViewModel.cpp
@@ -127,7 +127,7 @@ void AssetEditorViewModel::LoadAsset(ra::data::models::AssetModelBase* pAsset)
     if (m_pAsset == pAsset)
         return;
 
-    if (HasAssetValidationError())
+    if (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.",
@@ -138,14 +138,14 @@ void AssetEditorViewModel::LoadAsset(ra::data::models::AssetModelBase* pAsset)
 
         m_pAsset->RemoveNotifyTarget(*this);
         m_pAsset->RestoreLocalCheckpoint();
-
-        SetValue(AssetValidationErrorProperty, L"");
     }
     else if (m_pAsset)
     {
         m_pAsset->RemoveNotifyTarget(*this);
     }
 
+    SetValue(AssetValidationErrorProperty, L"");
+
     // null out internal pointer while binding to prevent change triggers
     m_pAsset = nullptr;
 
@@ -601,6 +601,14 @@ void AssetEditorViewModel::OnValueChanged(const IntModelProperty::ChangeArgs& ar
     WindowViewModelBase::OnValueChanged(args);
 }
 
+static void AppendTrigger(std::string& sDefinition, const std::string& sTrigger)
+{
+    if (sTrigger.empty())
+        sDefinition += "0=1";
+    else
+        sDefinition += sTrigger;
+}
+
 void AssetEditorViewModel::OnTriggerChanged()
 {
     const std::string& sTrigger = Trigger().Serialize();
@@ -622,25 +630,25 @@ void AssetEditorViewModel::OnTriggerChanged()
         if (nSelectedPart == LeaderboardPart::Start)
             sDefinition += sTrigger;
         else
-            sDefinition += pLeaderboard->GetStartTrigger();
+            AppendTrigger(sDefinition, pLeaderboard->GetStartTrigger());
 
         sDefinition.append("::SUB:");
         if (nSelectedPart == LeaderboardPart::Submit)
             sDefinition += sTrigger;
         else
-            sDefinition += pLeaderboard->GetSubmitTrigger();
+            AppendTrigger(sDefinition, pLeaderboard->GetSubmitTrigger());
 
         sDefinition.append("::CAN:");
         if (nSelectedPart == LeaderboardPart::Cancel)
             sDefinition += sTrigger;
         else
-            sDefinition += pLeaderboard->GetCancelTrigger();
+            AppendTrigger(sDefinition, pLeaderboard->GetCancelTrigger());
 
         sDefinition.append("::VAL:");
         if (nSelectedPart == LeaderboardPart::Value)
             sDefinition += sTrigger;
         else
-            sDefinition += pLeaderboard->GetValueDefinition();
+            AppendTrigger(sDefinition, pLeaderboard->GetValueDefinition());
 
         nSize = rc_lboard_size(sDefinition.c_str());
     }
diff --git a/src/ui/viewmodels/AssetListViewModel.cpp b/src/ui/viewmodels/AssetListViewModel.cpp
index ccc6e83c..0fab0a8a 100644
--- a/src/ui/viewmodels/AssetListViewModel.cpp
+++ b/src/ui/viewmodels/AssetListViewModel.cpp
@@ -1207,7 +1207,18 @@ bool AssetListViewModel::ValidateAssetsForCore(std::vector<ra::data::models::Ass
         {
             const auto* pLeaderboard = dynamic_cast<const ra::data::models::LeaderboardModel*>(pAsset);
             if (pLeaderboard != nullptr)
+            {
+                if (pLeaderboard->GetStartTrigger().empty())
+                    sError.append(ra::StringPrintf(L"\n* %s: No Start condition", pLeaderboard->GetName()));
+                if (pLeaderboard->GetCancelTrigger().empty())
+                    sError.append(ra::StringPrintf(L"\n* %s: No Cancel condition", pLeaderboard->GetName()));
+                if (pLeaderboard->GetSubmitTrigger().empty())
+                    sError.append(ra::StringPrintf(L"\n* %s: No Submit condition", pLeaderboard->GetName()));
+                if (pLeaderboard->GetValueDefinition().empty())
+                    sError.append(ra::StringPrintf(L"\n* %s: No Value definition", pLeaderboard->GetName()));
+
                 ValidateLeaderboardForCore(sError, *pLeaderboard);
+            }
         }
     }
 
diff --git a/src/ui/viewmodels/TriggerViewModel.cpp b/src/ui/viewmodels/TriggerViewModel.cpp
index 680c0231..79542251 100644
--- a/src/ui/viewmodels/TriggerViewModel.cpp
+++ b/src/ui/viewmodels/TriggerViewModel.cpp
@@ -472,6 +472,12 @@ void TriggerViewModel::InitializeFrom(const std::string& sTrigger, const ra::dat
         pCapturedHits.Restore(m_pTrigger, sTrigger);
         InitializeGroups(*m_pTrigger);
     }
+    else if (m_bIsValue)
+    {
+        rc_value_t pValue;
+        memset(&pValue, 0, sizeof(pValue));
+        InitializeFrom(pValue);
+    }
     else
     {
         rc_trigger_t pTrigger;
@@ -487,7 +493,7 @@ void TriggerViewModel::InitializeFrom(const rc_value_t& pValue)
     GSL_SUPPRESS_TYPE1 m_pTrigger = reinterpret_cast<rc_trigger_t*>(m_sTriggerBuffer.data());
     memset(m_pTrigger, 0, sizeof(rc_trigger_t));
     m_pTrigger->requirement = pValue.conditions;
-    m_pTrigger->alternative = pValue.conditions->next;
+    m_pTrigger->alternative = (pValue.conditions) ? pValue.conditions->next : nullptr;
     InitializeGroups(*m_pTrigger);
 }
 


Test patch:
diff --git a/tests/data/models/LeaderboardModel_Tests.cpp b/tests/data/models/LeaderboardModel_Tests.cpp
index 21917ea4..5d478a3a 100644
--- a/tests/data/models/LeaderboardModel_Tests.cpp
+++ b/tests/data/models/LeaderboardModel_Tests.cpp
@@ -355,6 +355,65 @@ public:
         Ensures(pLeaderboard != nullptr);
         Assert::AreEqual(0x5555U, pLeaderboard->value.conditions->conditions->operand1.value.memref->address);
     }
+
+    TEST_METHOD(TestValidateMissingField)
+    {
+        LeaderboardModelHarness leaderboard;
+        leaderboard.SetID(1U);
+        leaderboard.SetName(L"Title");
+        leaderboard.SetDescription(L"Desc");
+        leaderboard.SetValueFormat(ValueFormat::Value);
+        leaderboard.SetLowerIsBetter(true);
+        leaderboard.CreateServerCheckpoint();
+        leaderboard.CreateLocalCheckpoint();
+
+        Assert::IsFalse(leaderboard.Validate());
+        Assert::AreEqual(std::wstring(L"No Start condition"), leaderboard.GetValidationError());
+
+        leaderboard.SetStartTrigger("0xH1234=1");
+        Assert::IsFalse(leaderboard.Validate());
+        Assert::AreEqual(std::wstring(L"No Cancel condition"), leaderboard.GetValidationError());
+
+        leaderboard.SetCancelTrigger("0xH1234=2");
+        Assert::IsFalse(leaderboard.Validate());
+        Assert::AreEqual(std::wstring(L"No Submit condition"), leaderboard.GetValidationError());
+
+        leaderboard.SetSubmitTrigger("0xH1234=3");
+        Assert::IsFalse(leaderboard.Validate());
+        Assert::AreEqual(std::wstring(L"No Value definition"), leaderboard.GetValidationError());
+
+        leaderboard.SetValueDefinition("0xH2345");
+        Assert::IsTrue(leaderboard.Validate());
+        Assert::AreEqual(std::wstring(), leaderboard.GetValidationError());
+    }
+
+    TEST_METHOD(TestValidateInvalidField)
+    {
+        LeaderboardModelHarness leaderboard;
+        leaderboard.SetID(1U);
+        leaderboard.SetName(L"Title");
+        leaderboard.SetDescription(L"Desc");
+        leaderboard.SetDefinition("STA:N:0xH1234=1::SUB:N:0xH1234=2::CAN:N:0xH1234=3::VAL:0xH1234");
+        leaderboard.SetValueFormat(ValueFormat::Value);
+        leaderboard.SetLowerIsBetter(true);
+        leaderboard.CreateServerCheckpoint();
+        leaderboard.CreateLocalCheckpoint();
+
+        Assert::IsFalse(leaderboard.Validate());
+        Assert::AreEqual(std::wstring(L"Start: Final condition type expects another condition to follow"), leaderboard.GetValidationError());
+
+        leaderboard.SetStartTrigger("0xH1234=1");
+        Assert::IsFalse(leaderboard.Validate());
+        Assert::AreEqual(std::wstring(L"Cancel: Final condition type expects another condition to follow"), leaderboard.GetValidationError());
+
+        leaderboard.SetCancelTrigger("0xH1234=2");
+        Assert::IsFalse(leaderboard.Validate());
+        Assert::AreEqual(std::wstring(L"Submit: Final condition type expects another condition to follow"), leaderboard.GetValidationError());
+
+        leaderboard.SetSubmitTrigger("0xH1234=3");
+        Assert::IsTrue(leaderboard.Validate());
+        Assert::AreEqual(std::wstring(), leaderboard.GetValidationError());
+    }
 };
 
 } // namespace tests
diff --git a/tests/ui/viewmodels/AssetEditorViewModel_Tests.cpp b/tests/ui/viewmodels/AssetEditorViewModel_Tests.cpp
index a3aa7139..c3e425db 100644
--- a/tests/ui/viewmodels/AssetEditorViewModel_Tests.cpp
+++ b/tests/ui/viewmodels/AssetEditorViewModel_Tests.cpp
@@ -433,7 +433,7 @@ public:
         Assert::IsFalse(editor.HasMeasured());
     }
 
-    TEST_METHOD(TestLoadAchievementValidationError)
+    TEST_METHOD(TestLoadAchievementValidationWarning)
     {
         AssetEditorViewModelHarness editor;
         AchievementModel achievement;
@@ -457,6 +457,121 @@ public:
         Assert::AreEqual(std::wstring(L"Final condition type expects another condition to follow"), editor.GetAssetValidationWarning());
 
         editor.LoadAsset(nullptr);
+        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(TestLoadAchievementValidationError)
+    {
+        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.mockDesktop.ExpectWindow<ra::ui::viewmodels::MessageBoxViewModel>([](ra::ui::viewmodels::MessageBoxViewModel& vmMessageBox)
+        {
+            Assert::AreEqual(std::wstring(L"Discard changes?"), vmMessageBox.GetHeader());
+            Assert::AreEqual(std::wstring(L"The currently loaded asset has an error that cannot be saved. If you switch to another asset, your changes will be lost."), vmMessageBox.GetMessage());
+            return ra::ui::DialogResult::Yes;
+        });
+
+        editor.LoadAsset(nullptr);
+        Assert::IsTrue(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(TestLoadAchievementValidationErrorCancel)
+    {
+        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.mockDesktop.ExpectWindow<ra::ui::viewmodels::MessageBoxViewModel>([](ra::ui::viewmodels::MessageBoxViewModel& vmMessageBox)
+        {
+            Assert::AreEqual(std::wstring(L"Discard changes?"), vmMessageBox.GetHeader());
+            Assert::AreEqual(std::wstring(L"The currently loaded asset has an error that cannot be saved. If you switch to another asset, your changes will be lost."), vmMessageBox.GetMessage());
+            return ra::ui::DialogResult::No;
+        });
+
+        editor.LoadAsset(nullptr);
+        Assert::IsTrue(editor.mockDesktop.WasDialogShown());
+
+        Assert::IsTrue(editor.HasAssetValidationError());
+        Assert::AreEqual(std::wstring(L"Multiple Measured"), editor.GetAssetValidationError());
+        Assert::IsFalse(editor.HasAssetValidationWarning());
+        Assert::AreEqual(std::wstring(), editor.GetAssetValidationWarning());
+        Assert::IsTrue(editor.GetAsset() == &achievement);
+    }
+
+    TEST_METHOD(TestLoadAchievementValidationErrorDeleted)
+    {
+        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());
+
+        achievement.SetDeleted();
+
+        editor.LoadAsset(nullptr);
+        Assert::IsFalse(editor.mockDesktop.WasDialogShown());
         Assert::IsFalse(editor.HasAssetValidationError());
         Assert::AreEqual(std::wstring(), editor.GetAssetValidationError());
         Assert::IsFalse(editor.HasAssetValidationWarning());
@@ -472,6 +587,10 @@ public:
         leaderboard.SetState(AssetState::Active);
         leaderboard.SetDescription(L"Do something cool");
         leaderboard.SetCategory(AssetCategory::Unofficial);
+        leaderboard.SetStartTrigger("0xH1234=1");
+        leaderboard.SetCancelTrigger("0xH1234=2");
+        leaderboard.SetSubmitTrigger("0xH1234=3");
+        leaderboard.SetValueDefinition("0xH2345");
         leaderboard.SetValueFormat(ra::data::ValueFormat::Centiseconds);
         leaderboard.SetLowerIsBetter(false);
         leaderboard.CreateServerCheckpoint();
@@ -531,6 +650,51 @@ public:
         Assert::IsFalse(leaderboard.IsModified());
     }
 
+    TEST_METHOD(TestLoadNewLeaderboard)
+    {
+        AssetEditorViewModelHarness editor;
+        LeaderboardModel leaderboard;
+        leaderboard.SetID(1110000001U);
+        leaderboard.CreateServerCheckpoint();
+        leaderboard.SetState(AssetState::Inactive);
+        leaderboard.SetCategory(AssetCategory::Local);
+        leaderboard.SetNew();
+        leaderboard.CreateLocalCheckpoint();
+
+        editor.LoadAsset(&leaderboard);
+
+        Assert::AreEqual(std::wstring(L"Leaderboard Editor"), editor.GetWindowTitle());
+        Assert::AreEqual(0U, editor.GetID());
+        Assert::AreEqual(std::wstring(L""), editor.GetName());
+        Assert::AreEqual(std::wstring(L""), editor.GetDescription());
+        Assert::AreEqual(AssetCategory::Local, editor.GetCategory());
+        Assert::AreEqual(AssetState::Inactive, editor.GetState());
+        Assert::AreEqual(ra::data::ValueFormat::Value, editor.GetValueFormat());
+        Assert::IsFalse(editor.IsLowerBetter());
+        Assert::IsFalse(editor.IsPauseOnReset());
+        Assert::IsFalse(editor.IsPauseOnTrigger());
+        Assert::IsTrue(editor.IsAssetLoaded());
+        Assert::IsFalse(editor.IsAchievement());
+        Assert::IsTrue(editor.IsLeaderboard());
+        Assert::IsTrue(editor.IsTrigger());
+        Assert::IsTrue(editor.HasMeasured());
+        Assert::IsFalse(editor.HasAssetValidationError());
+        Assert::IsTrue(editor.HasAssetValidationWarning());
+        Assert::AreEqual(std::wstring(L"No Start condition"), editor.GetAssetValidationWarning());
+        Assert::AreEqual(std::wstring(L"Groups:"), editor.GetGroupsHeaderLabel());
+
+        editor.Trigger().Conditions().Add();
+        Assert::IsFalse(editor.HasAssetValidationError());
+        Assert::IsTrue(editor.HasAssetValidationWarning());
+        Assert::AreEqual(std::wstring(L"No Start condition"), editor.GetAssetValidationWarning());
+
+        // warning is only updated when record is saved to prevent spam when constructing a record
+        leaderboard.Validate();
+        Assert::IsFalse(editor.HasAssetValidationError());
+        Assert::IsTrue(editor.HasAssetValidationWarning());
+        Assert::AreEqual(std::wstring(L"No Cancel condition"), editor.GetAssetValidationWarning());
+    }
+
     TEST_METHOD(TestSyncId)
     {
         AssetEditorViewModelHarness editor;
diff --git a/tests/ui/viewmodels/TriggerViewModel_Tests.cpp b/tests/ui/viewmodels/TriggerViewModel_Tests.cpp
index 9ac90bfd..ee008de1 100644
--- a/tests/ui/viewmodels/TriggerViewModel_Tests.cpp
+++ b/tests/ui/viewmodels/TriggerViewModel_Tests.cpp
@@ -73,6 +73,7 @@ private:
 
         const std::string sOutput = vmTrigger.Serialize();
         Assert::AreEqual(sInput, sOutput);
+        Assert::IsFalse(vmTrigger.IsValue());
     }
 
     void ParseAndRegenerateValue(const std::string& sInput)
@@ -83,6 +84,7 @@ private:
 
         const std::string sOutput = vmTrigger.Serialize();
         Assert::AreEqual(sInput, sOutput);
+        Assert::IsTrue(vmTrigger.IsValue());
     }
 
     class TriggerMonitor : ViewModelBase::NotifyTarget
@@ -225,6 +227,7 @@ public:
 
     TEST_METHOD(TestParseAndRegenerateCoreOnly)
     {
+        ParseAndRegenerate(""); // no conditions
         ParseAndRegenerate("0xH1234=0xH2345"); // one condition
         ParseAndRegenerate("0xH1234=0xH2345_0xX5555=1.3._R:0x face=678"); // several conditions
         ParseAndRegenerate("I:0x 1234_A:0xH2345_0xH7777=345"); // addsource/addaddress chain
@@ -240,6 +243,7 @@ public:
 
     TEST_METHOD(TestParseAndRegenerateValue)
     {
+        ParseAndRegenerateValue(""); // empty
         ParseAndRegenerateValue("A:0xH1234*10_M:0xH1235"); // multiplication
         ParseAndRegenerateValue("M:0xH1234$M:0xX5555"); // one alt
         ParseAndRegenerateValue("M:0xH1234$M:0xX5555$M:b0xL3333"); // several alts