Repository: RetroAchievements/RAIntegration
Pull request #744 (2021-06-05 12:55:33)

prevent exception trying to remove group when no group selected


Patch:
diff --git a/src/ui/viewmodels/AssetListViewModel.cpp b/src/ui/viewmodels/AssetListViewModel.cpp
index 36d6db08..b1cf2b66 100644
--- a/src/ui/viewmodels/AssetListViewModel.cpp
+++ b/src/ui/viewmodels/AssetListViewModel.cpp
@@ -354,6 +354,22 @@ void AssetListViewModel::ApplyFilter()
     UpdateButtons();
 }
 
+void AssetListViewModel::EnsureAppearsInFilteredList(const ra::data::models::AssetModelBase& pAsset)
+{
+    // if the filter category is not all, ensure it matches the asset
+    const auto nFilterCategory = GetFilterCategory();
+    if (nFilterCategory != FilterCategory::All)
+    {
+        const auto nAssetCategory = ra::itoe<ra::data::models::AssetCategory>(ra::etoi(nFilterCategory));
+        if (pAsset.GetCategory() != nAssetCategory)
+            SetValue(FilterCategoryProperty, ra::etoi(pAsset.GetCategory()));
+    }
+
+    // if there's a special filter and it doesn't match, clear the special filter
+    if (GetSpecialFilter() != SpecialFilter::All && !MatchesFilter(pAsset))
+        SetSpecialFilter(ra::ui::viewmodels::AssetListViewModel::SpecialFilter::All);
+}
+
 bool AssetListViewModel::MatchesFilter(const ra::data::models::AssetModelBase& pAsset)
 {
     const auto nFilterCategory = GetFilterCategory();
@@ -1343,8 +1359,6 @@ void AssetListViewModel::CreateNew()
 
     FilteredAssets().BeginUpdate();
 
-    SetFilterCategory(FilterCategory::Local);
-
     auto& vmAchievement = pGameContext.Assets().NewAchievement();
     vmAchievement.SetCategory(ra::data::models::AssetCategory::Local);
     vmAchievement.SetPoints(0);
@@ -1352,6 +1366,8 @@ void AssetListViewModel::CreateNew()
     vmAchievement.UpdateServerCheckpoint();
     vmAchievement.SetNew();
 
+    EnsureAppearsInFilteredList(vmAchievement);
+
     const auto nId = ra::to_signed(vmAchievement.GetID());
 
     // select the new viewmodel, and deselect everything else
@@ -1416,8 +1432,6 @@ void AssetListViewModel::CloneSelected()
 
     FilteredAssets().BeginUpdate();
 
-    SetFilterCategory(FilterCategory::Local);
-
     // add the cloned items
     std::vector<int> vNewIDs;
     for (const auto* pAsset : vSelectedAssets)
@@ -1436,6 +1450,8 @@ void AssetListViewModel::CloneSelected()
             vmAchievement.SetTrigger(pSourceAchievement->GetTrigger());
             vmAchievement.SetNew();
 
+            EnsureAppearsInFilteredList(vmAchievement);
+
             vNewIDs.push_back(vmAchievement.GetID());
         }
     }
diff --git a/src/ui/viewmodels/AssetListViewModel.hh b/src/ui/viewmodels/AssetListViewModel.hh
index b0945424..786a7df8 100644
--- a/src/ui/viewmodels/AssetListViewModel.hh
+++ b/src/ui/viewmodels/AssetListViewModel.hh
@@ -281,6 +281,7 @@ private:
     void DoUpdateButtons();
     std::atomic_bool m_bNeedToUpdateButtons = false;
 
+    void EnsureAppearsInFilteredList(const ra::data::models::AssetModelBase& pAsset);
     bool MatchesFilter(const ra::data::models::AssetModelBase& pAsset);
     void AddOrRemoveFilteredItem(gsl::index nAssetIndex);
     bool AddOrRemoveFilteredItem(const ra::data::models::AssetModelBase& pAsset);
diff --git a/src/ui/viewmodels/TriggerViewModel.cpp b/src/ui/viewmodels/TriggerViewModel.cpp
index 89fa0735..8464d70c 100644
--- a/src/ui/viewmodels/TriggerViewModel.cpp
+++ b/src/ui/viewmodels/TriggerViewModel.cpp
@@ -646,6 +646,9 @@ void TriggerViewModel::AddGroup()
 void TriggerViewModel::RemoveGroup()
 {
     int nIndex = GetSelectedGroupIndex();
+    if (nIndex < 0) // no selection
+        return;
+
     if (nIndex == 0)
     {
         ra::ui::viewmodels::MessageBoxViewModel::ShowWarningMessage(L"The core group cannot be removed.");


Test patch:
diff --git a/tests/ui/viewmodels/AssetListViewModel_Tests.cpp b/tests/ui/viewmodels/AssetListViewModel_Tests.cpp
index 53baab04..42587a55 100644
--- a/tests/ui/viewmodels/AssetListViewModel_Tests.cpp
+++ b/tests/ui/viewmodels/AssetListViewModel_Tests.cpp
@@ -55,6 +55,29 @@ std::wstring ToString<ra::ui::viewmodels::AssetListViewModel::FilterCategory>(
     }
 }
 
+template<>
+std::wstring ToString<ra::ui::viewmodels::AssetListViewModel::SpecialFilter>(
+    const ra::ui::viewmodels::AssetListViewModel::SpecialFilter& filter)
+{
+    switch (filter)
+    {
+        case ra::ui::viewmodels::AssetListViewModel::SpecialFilter::Active:
+            return L"Active";
+        case ra::ui::viewmodels::AssetListViewModel::SpecialFilter::Inactive:
+            return L"Inactive";
+        case ra::ui::viewmodels::AssetListViewModel::SpecialFilter::Modified:
+            return L"Modified";
+        case ra::ui::viewmodels::AssetListViewModel::SpecialFilter::Unpublished:
+            return L"Unpublished";
+        case ra::ui::viewmodels::AssetListViewModel::SpecialFilter::Authored:
+            return L"Authored";
+        case ra::ui::viewmodels::AssetListViewModel::SpecialFilter::All:
+            return L"All";
+        default:
+            return std::to_wstring(ra::etoi(filter));
+    }
+}
+
 } // namespace CppUnitTestFramework
 } // namespace VisualStudio
 } // namespace Microsoft
@@ -2510,6 +2533,72 @@ public:
         Assert::IsTrue(pAsset->IsSelected());
     }
 
+    TEST_METHOD(TestCreateNewFilterCategoryAll)
+    {
+        AssetListViewModelHarness vmAssetList;
+        vmAssetList.mockUserContext.Initialize("User1", "FOO");
+        vmAssetList.MockGameId(22U);
+        vmAssetList.AddAchievement(AssetCategory::Core, 5, L"Test1", L"Desc1", L"12345", "0xH1234=1");
+        vmAssetList.AddAchievement(AssetCategory::Core, 7, L"Test2", L"Desc2", L"11111", "0xH1111=1");
+        vmAssetList.SetFilterCategory(AssetListViewModel::FilterCategory::All);
+        vmAssetList.ForceUpdateButtons();
+
+        Assert::AreEqual({ 2U }, vmAssetList.mockGameContext.Assets().Count());
+        Assert::AreEqual({ 2U }, vmAssetList.FilteredAssets().Count());
+        Assert::AreEqual(AssetListViewModel::FilterCategory::All, vmAssetList.GetFilterCategory());
+
+        vmAssetList.CreateNew();
+
+        // new Local achievement should be created and focused
+        Assert::AreEqual({ 3U }, vmAssetList.mockGameContext.Assets().Count());
+        Assert::AreEqual({ 3U }, vmAssetList.FilteredAssets().Count());
+        Assert::AreEqual(AssetListViewModel::FilterCategory::All, vmAssetList.GetFilterCategory());
+
+        const auto* pAsset = vmAssetList.FilteredAssets().GetItemAt(2);
+        Expects(pAsset != nullptr);
+        Assert::IsTrue(pAsset->IsSelected());
+        Assert::AreEqual(std::wstring(), pAsset->GetLabel());
+        Assert::AreEqual(AssetCategory::Local, pAsset->GetCategory());
+        Assert::AreEqual(AssetState::Inactive, pAsset->GetState());
+        Assert::AreEqual(AssetChanges::New, pAsset->GetChanges());
+        Assert::AreEqual({ 111000001U }, pAsset->GetId());
+        Assert::AreEqual(0, pAsset->GetPoints());
+    }
+
+    TEST_METHOD(TestCreateNewSpecialFilterActive)
+    {
+        AssetListViewModelHarness vmAssetList;
+        vmAssetList.mockUserContext.Initialize("User1", "FOO");
+        vmAssetList.MockGameId(22U);
+        vmAssetList.AddAchievement(AssetCategory::Core, 5, L"Test1", L"Desc1", L"12345", "0xH1234=1");
+        vmAssetList.mockGameContext.Assets().FindAchievement(1)->SetState(AssetState::Active);
+        vmAssetList.AddAchievement(AssetCategory::Core, 7, L"Test2", L"Desc2", L"11111", "0xH1111=1");
+        vmAssetList.mockGameContext.Assets().FindAchievement(2)->SetState(AssetState::Active);
+        vmAssetList.SetSpecialFilter(AssetListViewModel::SpecialFilter::Active);
+        vmAssetList.ForceUpdateButtons();
+
+        Assert::AreEqual({ 2U }, vmAssetList.mockGameContext.Assets().Count());
+        Assert::AreEqual({ 2U }, vmAssetList.FilteredAssets().Count());
+        Assert::AreEqual(AssetListViewModel::SpecialFilter::Active, vmAssetList.GetSpecialFilter());
+
+        vmAssetList.CreateNew();
+
+        // new Local achievement should be created and focused
+        Assert::AreEqual({ 3U }, vmAssetList.mockGameContext.Assets().Count());
+        Assert::AreEqual({ 1U }, vmAssetList.FilteredAssets().Count());
+        Assert::AreEqual(AssetListViewModel::SpecialFilter::All, vmAssetList.GetSpecialFilter());
+
+        const auto* pAsset = vmAssetList.FilteredAssets().GetItemAt(0);
+        Expects(pAsset != nullptr);
+        Assert::IsTrue(pAsset->IsSelected());
+        Assert::AreEqual(std::wstring(), pAsset->GetLabel());
+        Assert::AreEqual(AssetCategory::Local, pAsset->GetCategory());
+        Assert::AreEqual(AssetState::Inactive, pAsset->GetState());
+        Assert::AreEqual(AssetChanges::New, pAsset->GetChanges());
+        Assert::AreEqual({ 111000001U }, pAsset->GetId());
+        Assert::AreEqual(0, pAsset->GetPoints());
+    }
+
     TEST_METHOD(TestCloneSingle)
     {
         AssetListViewModelHarness vmAssetList;
@@ -2633,6 +2722,81 @@ public:
         Assert::AreEqual(2, pLocalBadges->GetReferenceCount(sLocalBadgeName, false));
     }
 
+    TEST_METHOD(TestCloneSingleFilterCategoryAll)
+    {
+        AssetListViewModelHarness vmAssetList;
+        vmAssetList.SetGameId(22U);
+        vmAssetList.AddAchievement(AssetCategory::Core, 5, L"Test1", L"Desc1", L"12345", "0xH1234=1");
+        vmAssetList.AddAchievement(AssetCategory::Core, 7, L"Test2", L"Desc2", L"11111", "0xH1111=1");
+        vmAssetList.SetFilterCategory(AssetListViewModel::FilterCategory::All);
+
+        Assert::AreEqual({ 2U }, vmAssetList.mockGameContext.Assets().Count());
+        Assert::AreEqual({ 2U }, vmAssetList.FilteredAssets().Count());
+        Assert::AreEqual(AssetListViewModel::FilterCategory::All, vmAssetList.GetFilterCategory());
+
+        vmAssetList.FilteredAssets().GetItemAt(1)->SetSelected(true);
+        vmAssetList.ForceUpdateButtons();
+
+        vmAssetList.CloneSelected();
+
+        // new Local achievement should be created and focused without changing the filter category
+        Assert::AreEqual({ 3U }, vmAssetList.mockGameContext.Assets().Count());
+        Assert::AreEqual({ 3U }, vmAssetList.FilteredAssets().Count());
+        Assert::AreEqual(AssetListViewModel::FilterCategory::All, vmAssetList.GetFilterCategory());
+
+        const auto* pAsset = vmAssetList.FilteredAssets().GetItemAt(0);
+        Expects(pAsset != nullptr);
+        Assert::AreEqual({ 1U }, pAsset->GetId());
+        Assert::IsFalse(pAsset->IsSelected());
+
+        pAsset = vmAssetList.FilteredAssets().GetItemAt(2);
+        Expects(pAsset != nullptr);
+        Assert::AreEqual({ 111000001U }, pAsset->GetId());
+        Assert::AreEqual(AssetCategory::Local, pAsset->GetCategory());
+        Assert::IsTrue(pAsset->IsSelected());
+    }
+
+    TEST_METHOD(TestCloneSingleSpecialFilterActive)
+    {
+        AssetListViewModelHarness vmAssetList;
+
+        vmAssetList.SetGameId(22U);
+        vmAssetList.SetFilterCategory(AssetListViewModel::FilterCategory::Local);
+        vmAssetList.AddAchievement(AssetCategory::Local, 5, L"Test1", L"Desc1", L"12345", "0xH1234=1");
+        auto* pSourceAchievement = vmAssetList.mockGameContext.Assets().FindAchievement(1);
+        Expects(pSourceAchievement != nullptr);
+        pSourceAchievement->SetState(ra::data::models::AssetState::Active);
+        vmAssetList.SetSpecialFilter(AssetListViewModel::SpecialFilter::Active);
+        Assert::AreEqual(AssetListViewModel::SpecialFilter::Active, vmAssetList.GetSpecialFilter());
+
+        Assert::AreEqual({ 1U }, vmAssetList.mockGameContext.Assets().Count());
+        Assert::AreEqual({ 1U }, vmAssetList.FilteredAssets().Count());
+
+        vmAssetList.FilteredAssets().GetItemAt(0)->SetSelected(true);
+        vmAssetList.ForceUpdateButtons();
+
+        vmAssetList.CloneSelected();
+
+        // new Local achievement should be created and focused
+        Assert::AreEqual({ 2U }, vmAssetList.mockGameContext.Assets().Count());
+        Assert::AreEqual({ 2U }, vmAssetList.FilteredAssets().Count());
+        Assert::AreEqual(AssetListViewModel::FilterCategory::Local, vmAssetList.GetFilterCategory());
+        Assert::AreEqual(AssetListViewModel::SpecialFilter::All, vmAssetList.GetSpecialFilter());
+
+        const auto* pAsset = vmAssetList.FilteredAssets().GetItemAt(0);
+        Expects(pAsset != nullptr);
+        Assert::AreEqual({ 1U }, pAsset->GetId());
+        Assert::AreEqual(AssetState::Active, pAsset->GetState());
+        Assert::IsFalse(pAsset->IsSelected());
+
+        pAsset = vmAssetList.FilteredAssets().GetItemAt(1);
+        Expects(pAsset != nullptr);
+        Assert::AreEqual({ 111000001U }, pAsset->GetId());
+        Assert::AreEqual(AssetCategory::Local, pAsset->GetCategory());
+        Assert::AreEqual(AssetState::Inactive, pAsset->GetState());
+        Assert::IsTrue(pAsset->IsSelected());
+    }
+
     TEST_METHOD(TestCloneMultiple)
     {
         AssetListViewModelHarness vmAssetList;
diff --git a/tests/ui/viewmodels/TriggerViewModel_Tests.cpp b/tests/ui/viewmodels/TriggerViewModel_Tests.cpp
index 5255cdb8..fedefa52 100644
--- a/tests/ui/viewmodels/TriggerViewModel_Tests.cpp
+++ b/tests/ui/viewmodels/TriggerViewModel_Tests.cpp
@@ -959,6 +959,23 @@ public:
         Assert::AreEqual(std::string("S"), vmTrigger.Serialize());
     }
 
+    TEST_METHOD(TestRemoveGroupNoSelection)
+    {
+        TriggerViewModelHarness vmTrigger;
+        Parse(vmTrigger, "SS");
+        Assert::AreEqual({ 3U }, vmTrigger.Groups().Count());
+
+        vmTrigger.SetSelectedGroupIndex(-1);
+        Assert::AreEqual(-1, vmTrigger.GetSelectedGroupIndex());
+
+        vmTrigger.RemoveGroup();
+        Assert::IsFalse(vmTrigger.mockDesktop.WasDialogShown());
+        Assert::AreEqual({ 3U }, vmTrigger.Groups().Count());
+
+        Assert::AreEqual(-1, vmTrigger.GetSelectedGroupIndex());
+        Assert::AreEqual(0, vmTrigger.GetVersion());
+    }
+
     TEST_METHOD(TestDoFrameUpdatesHits)
     {
         TriggerViewModelHarness vmTrigger;