Repository: RetroAchievements/RAIntegration
Pull request #736 (2021-05-24 17:29:13)

prevent exception if selected group is removed when resetting achievement


Patch:
diff --git a/.github/workflows/c-cpp.yml b/.github/workflows/c-cpp.yml
new file mode 100644
index 00000000..ce9dc8c5
--- /dev/null
+++ b/.github/workflows/c-cpp.yml
@@ -0,0 +1,93 @@
+name: C/C++ CI
+
+on:
+  push:
+    branches: [ master ]
+  pull_request:
+    branches: [ master ]
+
+jobs:
+  Win32-DLL:
+    runs-on: windows-latest
+    steps:
+    - name: Checkout
+      uses: actions/checkout@v2
+      with:
+        submodules: recursive
+    - name: Install MSBuild
+      uses: microsoft/setup-msbuild@v1.0.2
+    - name: Build
+      run: msbuild.exe RA_Integration.sln -t:RA_Integration -p:Configuration=Release -p:Platform=Win32
+
+  Win64-DLL:
+    runs-on: windows-latest
+    steps:
+    - name: Checkout
+      uses: actions/checkout@v2
+      with:
+        submodules: recursive
+    - name: Install MSBuild
+      uses: microsoft/setup-msbuild@v1.0.2
+    - name: Build
+      run: msbuild.exe RA_Integration.sln -t:RA_Integration -p:Configuration=Release -p:Platform=x64
+
+  Win32-Tests:
+    runs-on: windows-latest
+    steps:
+    - name: Checkout
+      uses: actions/checkout@v2
+      with:
+        submodules: recursive
+    - name: Install MSBuild
+      uses: microsoft/setup-msbuild@v1.0.2
+    - name: Install VSTest
+      uses: Malcolmnixon/Setup-VSTest@v4
+    - name: Build
+      run: msbuild.exe -m RA_Integration.sln -p:Configuration=Release -p:Platform=Win32
+    - name: Run DLL Tests
+      run: vstest.console bin\Win32\Release\tests\RA_Integration\RA_Integration.Tests.dll
+    - name: Run Interface Tests
+      # requires the DLL to have been built
+      run: vstest.console bin\Win32\Release\tests\RA_Interface\RA_Interface.Tests.dll
+
+  Win64-Tests:
+    runs-on: windows-latest
+    steps:
+    - name: Checkout
+      uses: actions/checkout@v2
+      with:
+        submodules: recursive
+    - name: Install MSBuild
+      uses: microsoft/setup-msbuild@v1.0.2
+    - name: Install VSTest
+      uses: Malcolmnixon/Setup-VSTest@v4
+    - name: Build
+      run: msbuild.exe -m RA_Integration.sln -p:Configuration=Release -p:Platform=x64
+    - name: Run DLL Tests
+      run: vstest.console bin\x64\Release\tests\RA_Integration\RA_Integration.Tests.dll
+    - name: Run Interface Tests
+      run: vstest.console bin\x64\Release\tests\RA_Interface\RA_Interface.Tests.dll
+
+  Win32-Analysis:
+    runs-on: windows-latest
+    steps:
+    - name: Checkout
+      uses: actions/checkout@v2
+      with:
+        submodules: recursive
+    - name: Install MSBuild
+      uses: microsoft/setup-msbuild@v1.0.2
+    - name: Build
+      run: msbuild.exe RA_Integration.sln -p:Configuration=Analysis -p:Platform=Win32
+
+  Win64-Analysis:
+    runs-on: windows-latest
+    steps:
+    - name: Checkout
+      uses: actions/checkout@v2
+      with:
+        submodules: recursive
+    - name: Install MSBuild
+      uses: microsoft/setup-msbuild@v1.0.2
+    - name: Build
+      run: msbuild.exe RA_Integration.sln -p:Configuration=Analysis -p:Platform=x64
diff --git a/.travis.yml b/.travis.yml
deleted file mode 100644
index 0360b22a..00000000
--- a/.travis.yml
+++ /dev/null
@@ -1,59 +0,0 @@
-# Control file for continuous integration testing at http://travis-ci.org/
-
-# ignore release tags
-branches:
-  except:
-    /^RAIntegration.\d+\.\d+(\.\d+)?(-\S*)?$/  
-
-language: cpp
-
-
-jobs:
-  include:
-    - name: "Win32 DLL"
-      os: windows
-      # builds with visualstudio2017buildtools - implicitly supports v141_xp target
-      script: ./BuildAll.bat Release x86
-
-    - name: "Win64 DLL"
-      os: windows
-      # builds with visualstudio2017buildtools - implicitly supports v141_xp target
-      script: ./BuildAll.bat Release x64
-    
-    - name: "Win32 Tests"
-      os: windows
-      install:
-        # CppUnitTestFramework, v141_xp, afxres.h
-        - choco install visualstudio2019community --package-parameters "--add Microsoft.VisualStudio.Component.WinXP --add Microsoft.VisualStudio.Component.VC.ATLMFC"
-        # atlbase.h
-        - choco install visualstudio2019-workload-nativedesktop    
-      script: ./BuildAll.bat Tests x86
-
-    - name: "Win64 Tests"
-      os: windows
-      install:
-        # CppUnitTestFramework, v141_xp, afxres.h
-        - choco install visualstudio2019community --package-parameters "--add Microsoft.VisualStudio.Component.WinXP --add Microsoft.VisualStudio.Component.VC.ATLMFC"
-        # atlbase.h
-        - choco install visualstudio2019-workload-nativedesktop
-      script: ./BuildAll.bat Tests x64
-
-    - name: "Win32 Analysis"
-      os: windows
-      install:
-        # CppUnitTestFramework, atlbase.h, afxres.h
-        - choco install visualstudio2019community --package-parameters "--add Microsoft.VisualStudio.Component.VC.ATLMFC"
-        # Microsoft.Cpp.Default.props
-        - choco install visualstudio2019-workload-vctools
-        # atlbase.h
-        - choco install visualstudio2019-workload-nativedesktop
-      script: ./BuildAll.bat Analysis x86
-
-    - name: "Win64 Analysis"
-      os: windows
-      install:
-        # CppUnitTestFramework, atlbase.h, afxres.h
-        - choco install visualstudio2019community --package-parameters "--add Microsoft.VisualStudio.Component.VC.ATLMFC"
-        # Microsoft.Cpp.Default.props, atlbase.h
-        - choco install visualstudio2019-workload-nativedesktop
-      script: ./BuildAll.bat Analysis x64
diff --git a/src/data/models/AssetModelBase.cpp b/src/data/models/AssetModelBase.cpp
index 60a2a47f..f24515bb 100644
--- a/src/data/models/AssetModelBase.cpp
+++ b/src/data/models/AssetModelBase.cpp
@@ -425,9 +425,12 @@ void AssetModelBase::SetAssetDefinition(AssetDefinition& pAsset, const std::stri
     else
     {
         // after local checkpoint
+        const auto nState = GetAssetDefinitionState(pAsset);
+
         if (pAsset.m_bLocalModified && sValue == pAsset.m_sLocalDefinition)
         {
-            if (!pAsset.m_sCurrentDefinition.empty())
+            // value being set to unpublished value
+            if (nState != AssetChanges::Unpublished)
             {
                 pAsset.m_sCurrentDefinition.clear();
                 UpdateAssetDefinitionVersion(pAsset, AssetChanges::Unpublished);
@@ -435,7 +438,8 @@ void AssetModelBase::SetAssetDefinition(AssetDefinition& pAsset, const std::stri
         }
         else if (!pAsset.m_bLocalModified && sValue == pAsset.m_sCoreDefinition)
         {
-            if (!pAsset.m_sCurrentDefinition.empty())
+            // value being set to core value (and no unpublished value exists)
+            if (nState != AssetChanges::None)
             {
                 pAsset.m_sCurrentDefinition.clear();
                 UpdateAssetDefinitionVersion(pAsset, AssetChanges::None);
@@ -443,9 +447,15 @@ void AssetModelBase::SetAssetDefinition(AssetDefinition& pAsset, const std::stri
         }
         else if (pAsset.m_sCurrentDefinition != sValue)
         {
+            // value being changed
             pAsset.m_sCurrentDefinition = sValue;
             UpdateAssetDefinitionVersion(pAsset, AssetChanges::Modified);
         }
+        else if (sValue.empty() && !GetAssetDefinition(pAsset, nState).empty())
+        {
+            // value being changed to empty string
+            UpdateAssetDefinitionVersion(pAsset, AssetChanges::Modified);
+        }
     }
 }
 
diff --git a/src/ui/viewmodels/TriggerViewModel.cpp b/src/ui/viewmodels/TriggerViewModel.cpp
index 2d90049c..11110825 100644
--- a/src/ui/viewmodels/TriggerViewModel.cpp
+++ b/src/ui/viewmodels/TriggerViewModel.cpp
@@ -462,8 +462,16 @@ void TriggerViewModel::UpdateFrom(const rc_trigger_t& pTrigger)
     m_vGroups.EndUpdate();
     m_vGroups.AddNotifyTarget(*this);
 
-    // forcibly update the conditions from the selected group
-    UpdateConditions(m_vGroups.GetItemAt(nSelectedIndex));
+    if (nSelectedIndex < gsl::narrow_cast<int>(m_vGroups.Count()))
+    {
+        // forcibly update the conditions from the selected group
+        UpdateConditions(m_vGroups.GetItemAt(nSelectedIndex));
+    }
+    else
+    {
+        // selected group no longer exists, select core group
+        SetSelectedGroupIndex(0);
+    }
 }
 
 void TriggerViewModel::UpdateFrom(const std::string& sTrigger)
@@ -559,8 +567,8 @@ void TriggerViewModel::OnValueChanged(const IntModelProperty::ChangeArgs& args)
             if (args.tOldValue != -1)
             {
                 auto* pOldGroup = m_vGroups.GetItemAt(args.tOldValue);
-                Expects(pOldGroup != nullptr);
-                pOldGroup->SetSelected(false);
+                if (pOldGroup != nullptr) // group may have been deleted
+                    pOldGroup->SetSelected(false);
             }
 
             m_vGroups.AddNotifyTarget(*this);
diff --git a/src/ui/win32/AssetEditorDialog.cpp b/src/ui/win32/AssetEditorDialog.cpp
index 692e22c9..6b3ba82b 100644
--- a/src/ui/win32/AssetEditorDialog.cpp
+++ b/src/ui/win32/AssetEditorDialog.cpp
@@ -35,6 +35,17 @@ static constexpr int COLUMN_WIDTH_VALUE = 72;
 static constexpr int COLUMN_WIDTH_OPERATOR = 35;
 static constexpr int COLUMN_WIDTH_HITS = 84;
 
+using fnGetStockIconInfo = std::add_pointer_t<HRESULT WINAPI(SHSTOCKICONID, UINT, SHSTOCKICONINFO*)>;
+static fnGetStockIconInfo pGetStockIconInfo = nullptr;
+
+AssetEditorDialog::Presenter::Presenter() noexcept
+{
+    // SHGetStockIconInfo isn't supported on WinXP, so we have to dynamically find it.
+    auto hDll = LoadLibraryA("Shell32");
+    if (hDll)
+        GSL_SUPPRESS_TYPE1 pGetStockIconInfo = reinterpret_cast<fnGetStockIconInfo>(GetProcAddress(hDll, "SHGetStockIconInfo"));
+}
+
 bool AssetEditorDialog::Presenter::IsSupported(const ra::ui::WindowViewModelBase& vmViewModel) noexcept
 {
     return (dynamic_cast<const AssetEditorViewModel*>(&vmViewModel) != nullptr);
@@ -294,14 +305,20 @@ public:
     {
         if (pProperty == TriggerConditionViewModel::CurrentHitsProperty)
             return true;
-        if (pProperty == TriggerConditionViewModel::HasHitsProperty)
-            return true;
         if (pProperty == TriggerConditionViewModel::TotalHitsProperty)
             return true;
 
         return GridNumberColumnBinding::DependsOn(pProperty);
     }
 
+    bool DependsOn(const ra::ui::BoolModelProperty& pProperty) const override
+    {
+        if (pProperty == TriggerConditionViewModel::HasHitsProperty)
+            return true;
+
+        return GridNumberColumnBinding::DependsOn(pProperty);
+    }
+
     std::wstring GetTooltip(const ra::ui::ViewModelCollectionBase& vmItems, gsl::index nIndex) const override
     {
         const auto* vmConditions = dynamic_cast<const ViewModelCollection<TriggerConditionViewModel>*>(&vmItems);
@@ -660,10 +677,21 @@ BOOL AssetEditorDialog::OnInitDialog()
 
     if (!m_hErrorIcon)
     {
-        SHSTOCKICONINFO sii{};
-        sii.cbSize = sizeof(sii);
-        if (SUCCEEDED(::SHGetStockIconInfo(SIID_ERROR, SHGSI_ICON | SHGSI_SMALLICON, &sii)))
-            m_hErrorIcon = sii.hIcon;
+        if (pGetStockIconInfo != nullptr)
+        {
+            SHSTOCKICONINFO sii{};
+            sii.cbSize = sizeof(sii);
+            if (SUCCEEDED(pGetStockIconInfo(SIID_ERROR, SHGSI_ICON | SHGSI_SMALLICON, &sii)))
+                m_hErrorIcon = sii.hIcon;
+        }
+        else
+        {
+            // despite requesting a 16x16 icon, this returns a 32x32 one, which looks awkward in the space
+            // provided. GetStockIconInfo is prefered because it retusn an appropriately sized icon. this
+            // is fallback logic for WinXP.
+            GSL_SUPPRESS_TYPE1 m_hErrorIcon = reinterpret_cast<HICON>(
+                LoadImage(nullptr, MAKEINTRESOURCE(OIC_ERROR), IMAGE_ICON, 16, 16, LR_SHARED));
+        }
     }
 
     const auto hErrorIconControl = GetDlgItem(GetHWND(), IDC_RA_ERROR_INDICATOR);
diff --git a/src/ui/win32/AssetEditorDialog.hh b/src/ui/win32/AssetEditorDialog.hh
index e44b8935..52de2233 100644
--- a/src/ui/win32/AssetEditorDialog.hh
+++ b/src/ui/win32/AssetEditorDialog.hh
@@ -31,6 +31,8 @@ public:
     class Presenter : public IClosableDialogPresenter
     {
     public:
+        Presenter() noexcept;
+
         bool IsSupported(const ra::ui::WindowViewModelBase& viewModel) noexcept override;
         void ShowWindow(ra::ui::WindowViewModelBase& viewModel) override;
         void ShowModal(ra::ui::WindowViewModelBase& viewModel, HWND hParentWnd) override;
diff --git a/src/ui/win32/bindings/GridNumberColumnBinding.hh b/src/ui/win32/bindings/GridNumberColumnBinding.hh
index 9ae3b220..cfe3eaa7 100644
--- a/src/ui/win32/bindings/GridNumberColumnBinding.hh
+++ b/src/ui/win32/bindings/GridNumberColumnBinding.hh
@@ -36,6 +36,7 @@ public:
     {
         return pProperty == *m_pBoundProperty;
     }
+    using GridTextColumnBindingBase::DependsOn; // inherit other overloads
 
     unsigned int Maximum() const noexcept { return m_nMaximum; }
     void SetMaximum(unsigned int nValue) noexcept { m_nMaximum = nValue; }
diff --git a/src/ui/win32/bindings/GridTextColumnBinding.hh b/src/ui/win32/bindings/GridTextColumnBinding.hh
index c4be5937..64555948 100644
--- a/src/ui/win32/bindings/GridTextColumnBinding.hh
+++ b/src/ui/win32/bindings/GridTextColumnBinding.hh
@@ -40,6 +40,7 @@ public:
     {
         return pProperty == *m_pBoundProperty;
     }
+    using GridColumnBinding::DependsOn; // inherit other overloads
 
     const StringModelProperty& GetBoundProperty() const noexcept { return *m_pBoundProperty; }
 


Test patch:
diff --git a/tests/data/models/AssetModelBase_Tests.cpp b/tests/data/models/AssetModelBase_Tests.cpp
index 1290bb4b..e351a1f5 100644
--- a/tests/data/models/AssetModelBase_Tests.cpp
+++ b/tests/data/models/AssetModelBase_Tests.cpp
@@ -435,6 +435,36 @@ public:
         Assert::AreEqual(std::wstring(L"Name"), asset.GetName());
         Assert::AreEqual(AssetChanges::Unpublished, asset.GetChanges());
     }
+
+    TEST_METHOD(TestSetDefinitionToBlank)
+    {
+        AssetDefinitionViewModelHarness asset;
+        asset.SetName(L"ServerName");
+        asset.SetCategory(AssetCategory::Core);
+        asset.SetDefinition("ServerDefinition");
+        asset.CreateServerCheckpoint();
+        asset.CreateLocalCheckpoint();
+
+        Assert::AreEqual(AssetChanges::None, asset.GetChanges());
+
+        asset.SetDefinition("");
+        Assert::AreEqual(AssetChanges::Modified, asset.GetChanges());
+
+        asset.SetDefinition("ServerDefinition");
+        Assert::AreEqual(AssetChanges::None, asset.GetChanges());
+
+        asset.SetDefinition("LocalDefinition");
+        Assert::AreEqual(AssetChanges::Modified, asset.GetChanges());
+
+        asset.UpdateLocalCheckpoint();
+        Assert::AreEqual(AssetChanges::Unpublished, asset.GetChanges());
+
+        asset.SetDefinition("");
+        Assert::AreEqual(AssetChanges::Modified, asset.GetChanges());
+
+        asset.SetDefinition("LocalDefinition");
+        Assert::AreEqual(AssetChanges::Unpublished, asset.GetChanges());
+    }
 };
 
 const StringModelProperty AssetModelBase_Tests::AssetModelHarness::StringProperty("AssetModelHarness", "String", L"");
diff --git a/tests/ui/viewmodels/TriggerViewModel_Tests.cpp b/tests/ui/viewmodels/TriggerViewModel_Tests.cpp
index b7620bc8..5255cdb8 100644
--- a/tests/ui/viewmodels/TriggerViewModel_Tests.cpp
+++ b/tests/ui/viewmodels/TriggerViewModel_Tests.cpp
@@ -264,6 +264,26 @@ public:
         Assert::AreEqual(std::string("0xH1234=0xH2345_0xH04d2=0S0xX5555=1"), pMonitor.sNewTrigger);
     }
 
+    TEST_METHOD(TestTriggerUpdateSelectedGroupRemoved)
+    {
+        TriggerViewModel vmTrigger;
+        Parse(vmTrigger, "0xH1234=0xH2345S0xX5555=1");
+
+        vmTrigger.SetSelectedGroupIndex(1);
+        Assert::AreEqual({ 1 }, vmTrigger.Conditions().Count());
+        Assert::AreEqual({ 0x5555 }, vmTrigger.Conditions().GetItemAt(0)->GetSourceValue());
+
+        vmTrigger.UpdateFrom("0xH1234=0xH2345");
+
+        Assert::AreEqual(0, vmTrigger.GetSelectedGroupIndex());
+        Assert::AreEqual({ 1 }, vmTrigger.Conditions().Count());
+        Assert::AreEqual({ 0x1234 }, vmTrigger.Conditions().GetItemAt(0)->GetSourceValue());
+
+        // calling UpdateFrom does not change the version, so we can't use a monitor as it
+        // won't get the event. but we can check the serialized value
+        Assert::AreEqual(std::string("0xH1234=0xH2345"), vmTrigger.Serialize());
+    }
+
     TEST_METHOD(TestTriggerUpdateConditionsResetAndReapplied)
     {
         TriggerViewModel vmTrigger;