Repository: RetroAchievements/RAIntegration
Pull request #892 (2022-05-08 22:03:54)

perform invalid check on Save All
Prevents accidentally saving an achievement in an invalid state.

The existing behavior worked fine for "Save". This just expands it for "Save All".

https://discord.com/channels/310192285306454017/310195377993416714/972932561905000548


Patch:
diff --git a/src/data/context/GameContext.cpp b/src/data/context/GameContext.cpp
index be878f47..b3008e5c 100644
--- a/src/data/context/GameContext.cpp
+++ b/src/data/context/GameContext.cpp
@@ -88,6 +88,19 @@ void GameContext::LoadGame(unsigned int nGameId, Mode nMode)
     pLocalBadges->CreateLocalCheckpoint();
     m_vAssets.Append(std::move(pLocalBadges));
 
+    // capture the old rich presence data so we can tell if it changed on the server
+    std::string sOldRichPresence = "[NONE]";
+    {
+        auto& pLocalStorage = ra::services::ServiceLocator::GetMutable<ra::services::ILocalStorage>();
+        auto pData = pLocalStorage.ReadText(ra::services::StorageItemType::GameData, std::to_wstring(nGameId));
+        if (pData != nullptr)
+        {
+            rapidjson::Document pDocument;
+            if (LoadDocument(pDocument, *pData) && pDocument.HasMember("RichPresencePatch"))
+                sOldRichPresence = pDocument["RichPresencePatch"].GetString();
+        }
+    }
+
     // download the game data
     ra::api::FetchGameData::Request request;
     request.GameId = nGameId;
@@ -225,7 +238,20 @@ void GameContext::LoadGame(unsigned int nGameId, Mode nMode)
     auto* pRichPresence = m_vAssets.FindRichPresence();
     if (pRichPresence)
     {
-        if (pRichPresence->GetScript().empty() && !pRichPresence->IsModified())
+        // if the server value differs from the local value, the model will appear as Unpublished
+        if (pRichPresence->GetChanges() != ra::data::models::AssetChanges::None)
+        {
+            // populate another model with the old script so the string gets normalized correctly
+            ra::data::models::RichPresenceModel pOldRichPresenceModel;
+            pOldRichPresenceModel.SetScript(sOldRichPresence);
+
+            // if the old value matches the current value, then the value on the server changed and
+            // there are no local modifications. revert to the server state.
+            if (pRichPresence->GetScript() == pOldRichPresenceModel.GetScript())
+                pRichPresence->RestoreServerCheckpoint();
+        }
+
+        if (pRichPresence->GetScript().empty() && pRichPresence->GetChanges() == ra::data::models::AssetChanges::None)
         {
             const auto nIndex = m_vAssets.FindItemIndex(ra::data::models::AssetModelBase::TypeProperty,
                                                         ra::etoi(ra::data::models::AssetType::RichPresence));
diff --git a/src/ui/viewmodels/AssetListViewModel.cpp b/src/ui/viewmodels/AssetListViewModel.cpp
index d83f7101..a579adfd 100644
--- a/src/ui/viewmodels/AssetListViewModel.cpp
+++ b/src/ui/viewmodels/AssetListViewModel.cpp
@@ -408,7 +408,7 @@ void AssetListViewModel::EnsureAppearsInFilteredList(const ra::data::models::Ass
         SetSpecialFilter(ra::ui::viewmodels::AssetListViewModel::SpecialFilter::All);
 }
 
-bool AssetListViewModel::MatchesFilter(const ra::data::models::AssetModelBase& pAsset)
+bool AssetListViewModel::MatchesFilter(const ra::data::models::AssetModelBase& pAsset) const
 {
     const auto nFilterCategory = GetFilterCategory();
     if (nFilterCategory != FilterCategory::All)
@@ -909,16 +909,32 @@ bool AssetListViewModel::SelectionContainsInvalidAsset(const std::vector<ra::dat
     if (pWindowManager.AssetEditor.HasAssetValidationError())
     {
         const auto* vmInvalidAsset = pWindowManager.AssetEditor.GetAsset();
-        for (const auto* vmItem : vSelectedAssets)
+
+        bool bInvalidAssetInSelection = false;
+        if (vSelectedAssets.empty())
         {
-            if (vmItem == vmInvalidAsset)
+            // no selection, all assets will be saved. if there's an invalid one, abort.
+            bInvalidAssetInSelection = true;
+        }
+        else
+        {
+            for (const auto* vmItem : vSelectedAssets)
             {
-                sErrorMessage = ra::StringPrintf(L"The following errors must be corrected:\n* %s",
-                    pWindowManager.AssetEditor.GetAssetValidationError());
-
-                return true;
+                if (vmItem == vmInvalidAsset)
+                {
+                    bInvalidAssetInSelection = true;
+                    break;
+                }
             }
         }
+
+        if (bInvalidAssetInSelection)
+        {
+            sErrorMessage = ra::StringPrintf(L"The following errors must be corrected:\n* %s",
+                                             pWindowManager.AssetEditor.GetAssetValidationError());
+
+            return true;
+        }
     }
 
     sErrorMessage.clear();
@@ -1617,6 +1633,7 @@ void AssetListViewModel::CreateNew()
     }
 
     FilteredAssets().BeginUpdate();
+    bool bUpdateAsset = true;
 
     auto& pGameContext = ra::services::ServiceLocator::GetMutable<ra::data::context::GameContext>();
     ra::data::models::AssetModelBase* pNewAsset = nullptr;
@@ -1632,17 +1649,36 @@ void AssetListViewModel::CreateNew()
             pNewAsset = &pGameContext.Assets().NewLeaderboard();
             break;
 
+        case ra::data::models::AssetType::RichPresence:
+            pNewAsset = pGameContext.Assets().FindRichPresence();
+            if (pNewAsset != nullptr)
+            {
+                bUpdateAsset = false;
+            }
+            else
+            {
+                RA_LOG_INFO("Creating rich presence");
+                auto pRichPresence = std::make_unique<ra::data::models::RichPresenceModel>();
+                pRichPresence->CreateServerCheckpoint();
+                pRichPresence->CreateLocalCheckpoint();
+                pNewAsset = &pGameContext.Assets().Append(std::move(pRichPresence));
+            }
+            break;
+
         default:
             break;
     }
 
     Expects(pNewAsset != nullptr);
 
-    const auto& pUserContext = ra::services::ServiceLocator::GetMutable<ra::data::context::UserContext>();
-    pNewAsset->SetAuthor(ra::Widen(pUserContext.GetDisplayName()));
-    pNewAsset->SetCategory(ra::data::models::AssetCategory::Local);
-    pNewAsset->UpdateServerCheckpoint();
-    pNewAsset->SetNew();
+    if (bUpdateAsset)
+    {
+        const auto& pUserContext = ra::services::ServiceLocator::GetMutable<ra::data::context::UserContext>();
+        pNewAsset->SetAuthor(ra::Widen(pUserContext.GetDisplayName()));
+        pNewAsset->SetCategory(ra::data::models::AssetCategory::Local);
+        pNewAsset->UpdateServerCheckpoint();
+        pNewAsset->SetNew();
+    }
 
     EnsureAppearsInFilteredList(*pNewAsset);
 
diff --git a/src/ui/viewmodels/AssetListViewModel.hh b/src/ui/viewmodels/AssetListViewModel.hh
index 734b9e2b..14c3e1b8 100644
--- a/src/ui/viewmodels/AssetListViewModel.hh
+++ b/src/ui/viewmodels/AssetListViewModel.hh
@@ -309,7 +309,7 @@ private:
     std::atomic_bool m_bNeedToUpdateButtons = false;
 
     void EnsureAppearsInFilteredList(const ra::data::models::AssetModelBase& pAsset);
-    bool MatchesFilter(const ra::data::models::AssetModelBase& pAsset);
+    bool MatchesFilter(const ra::data::models::AssetModelBase& pAsset) const;
     void AddOrRemoveFilteredItem(gsl::index nAssetIndex);
     bool AddOrRemoveFilteredItem(const ra::data::models::AssetModelBase& pAsset);
     gsl::index GetFilteredAssetIndex(const ra::data::models::AssetModelBase& pAsset) const;
diff --git a/src/ui/viewmodels/RichPresenceMonitorViewModel.cpp b/src/ui/viewmodels/RichPresenceMonitorViewModel.cpp
index d836f49d..b1d0631d 100644
--- a/src/ui/viewmodels/RichPresenceMonitorViewModel.cpp
+++ b/src/ui/viewmodels/RichPresenceMonitorViewModel.cpp
@@ -44,8 +44,18 @@ void RichPresenceMonitorViewModel::StartMonitoring()
         case MonitorState::None:
             // not monitoring - start doing so.
             m_nState = MonitorState::Active;
-            UpdateDisplayString();
             m_tRichPresenceFileTime = GetRichPresenceModified();
+            if (m_tRichPresenceFileTime)
+            {
+                auto& pGameContext = ra::services::ServiceLocator::GetMutable<ra::data::context::GameContext>();
+                auto* pRichPresence = pGameContext.Assets().FindRichPresence();
+                if (pRichPresence != nullptr)
+                {
+                    pRichPresence->ReloadRichPresenceScript();
+                    pRichPresence->Activate();
+                }
+            }
+            UpdateDisplayString();
             ScheduleUpdateDisplayString();
             break;
 


Test patch:
diff --git a/tests/data/context/GameContext_Tests.cpp b/tests/data/context/GameContext_Tests.cpp
index d9eafc51..90ee9a02 100644
--- a/tests/data/context/GameContext_Tests.cpp
+++ b/tests/data/context/GameContext_Tests.cpp
@@ -438,6 +438,56 @@ public:
         Assert::AreEqual(ra::data::models::AssetChanges::Unpublished, pRichPresence->GetChanges());
     }
 
+    TEST_METHOD(TestLoadGameRichPresenceUpdatedFromServer)
+    {
+        GameContextHarness game;
+        game.mockStorage.MockStoredData(ra::services::StorageItemType::GameData, L"1", "{\"RichPresencePatch\": \"Display:\\nOld\\n\"}");
+        game.mockStorage.MockStoredData(ra::services::StorageItemType::RichPresence, L"1", "Display:\nOld\n");
+        game.mockServer.HandleRequest<ra::api::FetchGameData>([](const ra::api::FetchGameData::Request&, ra::api::FetchGameData::Response& response)
+        {
+            response.RichPresence = "Display:\nHello, World";
+            return true;
+        });
+
+        game.LoadGame(1U);
+
+        Assert::IsTrue(game.HasRichPresence());
+        Assert::AreEqual(std::string("Display:\nHello, World\n"),
+            game.mockStorage.GetStoredData(ra::services::StorageItemType::RichPresence, L"1"));
+        Assert::IsFalse(game.IsRichPresenceFromFile());
+
+        const auto* pRichPresence = game.Assets().FindRichPresence();
+        Assert::IsNotNull(pRichPresence);
+        Ensures(pRichPresence != nullptr);
+        Assert::AreEqual(ra::data::models::AssetCategory::Core, pRichPresence->GetCategory());
+        Assert::AreEqual(ra::data::models::AssetChanges::None, pRichPresence->GetChanges());
+    }
+
+    TEST_METHOD(TestLoadGameRichPresenceUpdatedFromServerLocalModifications)
+    {
+        GameContextHarness game;
+        game.mockStorage.MockStoredData(ra::services::StorageItemType::GameData, L"1", "{\"RichPresencePatch\": \"Display:\\nOld\\n\"}");
+        game.mockStorage.MockStoredData(ra::services::StorageItemType::RichPresence, L"1", "Display:\nLocal\n");
+        game.mockServer.HandleRequest<ra::api::FetchGameData>([](const ra::api::FetchGameData::Request&, ra::api::FetchGameData::Response& response)
+        {
+            response.RichPresence = "Display:\nHello, World";
+            return true;
+        });
+
+        game.LoadGame(1U);
+
+        Assert::IsTrue(game.HasRichPresence());
+        Assert::AreEqual(std::string("Display:\nLocal\n"),
+            game.mockStorage.GetStoredData(ra::services::StorageItemType::RichPresence, L"1"));
+        Assert::IsTrue(game.IsRichPresenceFromFile());
+
+        const auto* pRichPresence = game.Assets().FindRichPresence();
+        Assert::IsNotNull(pRichPresence);
+        Ensures(pRichPresence != nullptr);
+        Assert::AreEqual(ra::data::models::AssetCategory::Core, pRichPresence->GetCategory());
+        Assert::AreEqual(ra::data::models::AssetChanges::Unpublished, pRichPresence->GetChanges());
+    }
+
     TEST_METHOD(TestLoadGameNoRichPresence)
     {
         GameContextHarness game;
diff --git a/tests/ui/viewmodels/AssetListViewModel_Tests.cpp b/tests/ui/viewmodels/AssetListViewModel_Tests.cpp
index 8df44448..5c7e8d78 100644
--- a/tests/ui/viewmodels/AssetListViewModel_Tests.cpp
+++ b/tests/ui/viewmodels/AssetListViewModel_Tests.cpp
@@ -493,6 +493,12 @@ private:
 
         bool SelectionContainsInvalidAsset(const std::vector<ra::data::models::AssetModelBase*>& vSelectedAssets, _Out_ std::wstring& sErrorMessage) const override
         {
+            if (vSelectedAssets.empty() && !m_mValidationErrors.empty())
+            {
+                sErrorMessage = m_mValidationErrors.begin()->second;
+                return true;
+            }
+
             for (const auto* pAsset : vSelectedAssets)
             {
                 Expects(pAsset != nullptr);
@@ -2203,6 +2209,36 @@ public:
         Assert::AreEqual(AssetChanges::Modified, pItem->GetChanges());
     }
 
+    TEST_METHOD(TestSaveSelectedInvalidAll)
+    {
+        AssetListViewModelHarness vmAssetList;
+        vmAssetList.MockGameId(1U);
+        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");
+
+        auto* pItem = dynamic_cast<ra::data::models::AchievementModel*>(vmAssetList.mockGameContext.Assets().GetItemAt(0));
+        Expects(pItem != nullptr);
+        pItem->SetName(L"Test1b");
+        vmAssetList.SetValidationError(pItem->GetID(), L"Error message goes here.");
+        vmAssetList.ForceUpdateButtons();
+        vmAssetList.AssertButtonState(SaveButtonState::SaveAll);
+
+        bool bMessageSeen = false;
+        vmAssetList.mockDesktop.ExpectWindow<ra::ui::viewmodels::MessageBoxViewModel>([&bMessageSeen](ra::ui::viewmodels::MessageBoxViewModel& vmMessage)
+        {
+            bMessageSeen = true;
+            Assert::AreEqual(std::wstring(L"Unable to save"), vmMessage.GetHeader());
+            Assert::AreEqual(std::wstring(L"Error message goes here."), vmMessage.GetMessage());
+
+            return DialogResult::OK;
+        });
+
+        Assert::AreEqual(AssetChanges::Modified, pItem->GetChanges());
+        vmAssetList.SaveSelected();
+        Assert::IsTrue(bMessageSeen);
+        Assert::AreEqual(AssetChanges::Modified, pItem->GetChanges());
+    }
+
     TEST_METHOD(TestSaveSelectedValidationWarning)
     {
         AssetListViewModelHarness vmAssetList;
@@ -3290,6 +3326,89 @@ public:
         Assert::IsFalse(bEditorShown);
     }
 
+    TEST_METHOD(TestCreateNewRichPresenceFilterTypeRichPresence)
+    {
+        AssetListViewModelHarness vmAssetList;
+        ra::services::mocks::MockFileSystem mockFileSystem;
+        ra::services::impl::FileLocalStorage storage(mockFileSystem);
+        ra::services::ServiceLocator::ServiceOverride<ra::services::ILocalStorage> storageOverride(&storage);
+
+        vmAssetList.mockUserContext.Initialize("User1", "FOO");
+        vmAssetList.MockGameId(22U);
+        vmAssetList.AddAchievement(AssetCategory::Core, 5, L"Test1", L"Desc1", L"12345", "0xH1234=1");
+        vmAssetList.AddLeaderboard(ra::data::models::AssetCategory::Core, L"Leaderboard1");
+        vmAssetList.SetAssetTypeFilter(ra::data::models::AssetType::RichPresence);
+        vmAssetList.ForceUpdateButtons();
+
+        Assert::AreEqual({ 2U }, vmAssetList.mockGameContext.Assets().Count());
+        Assert::AreEqual({ 0U }, vmAssetList.FilteredAssets().Count());
+
+        Assert::IsTrue(vmAssetList.CanCreate());
+        vmAssetList.CreateNew();
+
+        // new Local rich presence should be created and focused
+        Assert::AreEqual({ 3U }, vmAssetList.mockGameContext.Assets().Count());
+        Assert::AreEqual({ 1U }, vmAssetList.FilteredAssets().Count());
+        Assert::AreEqual(AssetListViewModel::FilterCategory::Local, vmAssetList.GetFilterCategory());
+        Assert::AreEqual(ra::data::models::AssetType::RichPresence, vmAssetList.GetAssetTypeFilter());
+
+        const auto* pAsset = vmAssetList.FilteredAssets().GetItemAt(0);
+        Expects(pAsset != nullptr);
+        Assert::IsTrue(pAsset->IsSelected());
+        Assert::AreEqual(std::wstring(L"Rich Presence"), pAsset->GetLabel());
+        Assert::AreEqual(AssetCategory::Local, pAsset->GetCategory());
+        Assert::AreEqual(AssetState::Inactive, pAsset->GetState());
+        Assert::AreEqual(AssetChanges::New, pAsset->GetChanges());
+        Assert::AreEqual({ 0 }, pAsset->GetId());
+        Assert::AreEqual(0, pAsset->GetPoints());
+
+        // and loaded externally
+        Assert::IsFalse(vmAssetList.mockWindowManager.AssetEditor.IsVisible());
+        Assert::AreEqual(std::string("file://localhost/./RACache/Data/22-Rich.txt"), vmAssetList.mockDesktop.LastOpenedUrl());
+    }
+
+    TEST_METHOD(TestCreateNewRichPresenceFilterTypeRichPresenceExisting)
+    {
+        AssetListViewModelHarness vmAssetList;
+        ra::services::mocks::MockFileSystem mockFileSystem;
+        ra::services::impl::FileLocalStorage storage(mockFileSystem);
+        ra::services::ServiceLocator::ServiceOverride<ra::services::ILocalStorage> storageOverride(&storage);
+
+        vmAssetList.mockUserContext.Initialize("User1", "FOO");
+        vmAssetList.MockGameId(22U);
+        vmAssetList.AddAchievement(AssetCategory::Core, 5, L"Test1", L"Desc1", L"12345", "0xH1234=1");
+        vmAssetList.AddLeaderboard(ra::data::models::AssetCategory::Core, L"Leaderboard1");
+        vmAssetList.AddRichPresence("Display\nTest\n");
+        vmAssetList.SetAssetTypeFilter(ra::data::models::AssetType::RichPresence);
+        vmAssetList.ForceUpdateButtons();
+
+        Assert::AreEqual({ 3U }, vmAssetList.mockGameContext.Assets().Count());
+        Assert::AreEqual({ 1U }, vmAssetList.FilteredAssets().Count());
+
+        Assert::IsTrue(vmAssetList.CanCreate());
+        vmAssetList.CreateNew();
+
+        // existing Local rich presence should be focused
+        Assert::AreEqual({ 3U }, vmAssetList.mockGameContext.Assets().Count());
+        Assert::AreEqual({ 1U }, vmAssetList.FilteredAssets().Count());
+        Assert::AreEqual(AssetListViewModel::FilterCategory::Core, vmAssetList.GetFilterCategory());
+        Assert::AreEqual(ra::data::models::AssetType::RichPresence, vmAssetList.GetAssetTypeFilter());
+
+        const auto* pAsset = vmAssetList.FilteredAssets().GetItemAt(0);
+        Expects(pAsset != nullptr);
+        Assert::IsTrue(pAsset->IsSelected());
+        Assert::AreEqual(std::wstring(L"Rich Presence"), pAsset->GetLabel());
+        Assert::AreEqual(AssetCategory::Core, pAsset->GetCategory());
+        Assert::AreEqual(AssetState::Inactive, pAsset->GetState());
+        Assert::AreEqual(AssetChanges::None, pAsset->GetChanges());
+        Assert::AreEqual({ 0 }, pAsset->GetId());
+        Assert::AreEqual(0, pAsset->GetPoints());
+
+        // and loaded externally
+        Assert::IsFalse(vmAssetList.mockWindowManager.AssetEditor.IsVisible());
+        Assert::AreEqual(std::string("file://localhost/./RACache/Data/22-Rich.txt"), vmAssetList.mockDesktop.LastOpenedUrl());
+    }
+
     TEST_METHOD(TestCloneSingle)
     {
         AssetListViewModelHarness vmAssetList;