Repository: RetroAchievements/RAIntegration
Pull request #931 (2022-10-17 17:15:13)

show [Deleted] in code notes window for non-published deleted notes


Patch:
diff --git a/src/RA_Shared.rc b/src/RA_Shared.rc
index 8eec1652..01e94ae3 100644
--- a/src/RA_Shared.rc
+++ b/src/RA_Shared.rc
@@ -91,7 +91,7 @@ BEGIN
     PUSHBUTTON      "&Save All",IDC_RA_COMMIT_ACH,367,57,68,16,BS_MULTILINE
     PUSHBUTTON      "&Reset All",IDC_RA_DOWNLOAD_ACH,367,76,68,16,BS_MULTILINE | WS_DISABLED
     PUSHBUTTON      "Re&vert All",IDC_RA_REVERTSELECTED,367,95,68,16,BS_MULTILINE
-    DEFPUSHBUTTON   "&New",IDC_RA_NEW_ASSET,367,126,68,16,BS_MULTILINE | WS_DISABLED
+    PUSHBUTTON      "&New",IDC_RA_NEW_ASSET,367,126,68,16,BS_MULTILINE | WS_DISABLED
     PUSHBUTTON      "&Clone",IDC_RA_CLONE_ASSET,367,145,68,16,BS_MULTILINE
 END
 
@@ -226,7 +226,7 @@ FONT 8, "MS Shell Dlg", 400, 0, 0x1
 BEGIN
     LTEXT           "RetroAchievements Hash:",IDC_STATIC,7,7,170,11
     LTEXT           "No game loaded.",IDC_RA_ROMCHECKSUMTEXT,7,17,170,10
-    DEFPUSHBUTTON   "Copy To Clipboard",IDC_RA_COPYCHECKSUMCLIPBOARD,7,33,82,14
+    PUSHBUTTON      "Copy To Clipboard",IDC_RA_COPYCHECKSUMCLIPBOARD,7,33,82,14
     DEFPUSHBUTTON   "OK",IDOK,127,33,50,14
 END
 
diff --git a/src/data/models/CodeNotesModel.cpp b/src/data/models/CodeNotesModel.cpp
index 4bb57c14..e3d76afa 100644
--- a/src/data/models/CodeNotesModel.cpp
+++ b/src/data/models/CodeNotesModel.cpp
@@ -628,16 +628,9 @@ void CodeNotesModel::SetCodeNote(ra::ByteAddress nAddress, const std::wstring& s
 
             m_mOriginalCodeNotes.erase(pIter2);
         }
-
-        if (sNote.empty())
-            m_mCodeNotes.erase(pIter);
     }
 
-    if (sNote.empty())
-    {
-        OnCodeNoteChanged(nAddress, sNote);
-    }
-    else if (!sOriginalAuthor.empty())
+    if (!sOriginalAuthor.empty())
     {
         AddCodeNote(nAddress, sOriginalAuthor, sNote);
     }
@@ -865,6 +858,9 @@ void CodeNotesModel::SetServerCodeNote(ra::ByteAddress nAddress, const std::wstr
     const auto pIter2 = m_mCodeNotes.find(nAddress);
     if (pIter2 != m_mCodeNotes.end() && pIter2->second.Note == sNote)
     {
+        if (sNote.empty())
+            m_mCodeNotes.erase(pIter2);
+
         if (m_mOriginalCodeNotes.empty())
             SetValue(ra::data::models::AssetModelBase::ChangesProperty, ra::etoi(ra::data::models::AssetChanges::None));
         OnCodeNoteChanged(nAddress, sNote);
diff --git a/src/data/models/CodeNotesModel.hh b/src/data/models/CodeNotesModel.hh
index caeb3055..126b6ed9 100644
--- a/src/data/models/CodeNotesModel.hh
+++ b/src/data/models/CodeNotesModel.hh
@@ -139,7 +139,7 @@ public:
     void SetCodeNote(ra::ByteAddress nAddress, const std::wstring& sNote);
 
     /// <summary>
-    /// Returns the number of known code notes
+    /// Returns the number of known code notes (not including indirect notes).
     /// </summary>
     size_t CodeNoteCount() const noexcept { return m_mCodeNotes.size(); }
 
diff --git a/src/ui/viewmodels/CodeNotesViewModel.cpp b/src/ui/viewmodels/CodeNotesViewModel.cpp
index bcef93a1..75f35804 100644
--- a/src/ui/viewmodels/CodeNotesViewModel.cpp
+++ b/src/ui/viewmodels/CodeNotesViewModel.cpp
@@ -148,7 +148,7 @@ void CodeNotesViewModel::OnCodeNoteChanged(ra::ByteAddress nAddress, const std::
         return;
 
     const auto* pCodeNotes = pGameContext.Assets().FindCodeNotes();
-    if (pCodeNotes == nullptr)
+    if (pCodeNotes == nullptr || pCodeNotes->GetIndirectSource(nAddress) != 0xFFFFFFFF)
         return;
 
     m_nUnfilteredNotesCount = pCodeNotes->CodeNoteCount();
@@ -176,6 +176,12 @@ void CodeNotesViewModel::OnCodeNoteChanged(ra::ByteAddress nAddress, const std::
                 pNote->SetNote(sNewNote);
                 pNote->SetModified(pCodeNotes->IsNoteModified(nAddress));
             }
+            else if (pCodeNotes->IsNoteModified(nAddress) && GetFilterValue().empty())
+            {
+                /* note was deleted */
+                pNote->SetNote(L"[Deleted]");
+                pNote->SetModified(true);
+            }
             else
             {
                 /* existing note was deleted or does not match filter */
diff --git a/src/ui/win32/MemoryInspectorDialog.cpp b/src/ui/win32/MemoryInspectorDialog.cpp
index 25f9eaca..f982fa58 100644
--- a/src/ui/win32/MemoryInspectorDialog.cpp
+++ b/src/ui/win32/MemoryInspectorDialog.cpp
@@ -77,31 +77,35 @@ void MemoryInspectorDialog::SearchResultsGridBinding::OnViewModelIntValueChanged
     if (args.Property == MemorySearchViewModel::ResultMemSizeProperty ||
         args.Property == MemorySearchViewModel::TotalMemorySizeProperty)
     {
-        int nWidth = 0;
-        const auto& vmMemory = GetViewModel<MemorySearchViewModel>();
-        const auto nSize = vmMemory.ResultMemSize();
-        constexpr int nCharWidth = 6;
-        constexpr int nPadding = 6;
-
-        // value column
-        auto nMaxChars = (ra::data::MemSizeBits(nSize) + 3) / 4; // 4 bits per nibble
-        if (nMaxChars == 0)
-            nMaxChars = 16;
-        nWidth = nCharWidth * (nMaxChars + 2) + nPadding * 2;
-        m_vColumns.at(1)->SetWidth(GridColumnBinding::WidthType::Pixels, nWidth);
-
-        // address column
-        nWidth = ra::ui::win32::bindings::GridAddressColumnBinding::CalculateWidth();
-        if (nSize == MemSize::Nibble_Lower)
-            nWidth += nCharWidth; // 0x1234L
-        m_vColumns.at(0)->SetWidth(GridColumnBinding::WidthType::Pixels, nWidth);
-
+        UpdateColumnWidths();
         UpdateLayout();
     }
 
     GridBinding::OnViewModelIntValueChanged(args);
 }
 
+void MemoryInspectorDialog::SearchResultsGridBinding::UpdateColumnWidths()
+{
+    int nWidth = 0;
+    const auto& vmMemory = GetViewModel<MemorySearchViewModel>();
+    const auto nSize = vmMemory.ResultMemSize();
+    constexpr int nCharWidth = 6;
+    constexpr int nPadding = 6;
+
+    // value column
+    auto nMaxChars = (ra::data::MemSizeBits(nSize) + 3) / 4; // 4 bits per nibble
+    if (nMaxChars == 0)
+        nMaxChars = 16;
+    nWidth = nCharWidth * (nMaxChars + 2) + nPadding * 2;
+    m_vColumns.at(1)->SetWidth(GridColumnBinding::WidthType::Pixels, nWidth);
+
+    // address column
+    nWidth = ra::ui::win32::bindings::GridAddressColumnBinding::CalculateWidth();
+    if (nSize == MemSize::Nibble_Lower)
+        nWidth += nCharWidth; // 0x1234L
+    m_vColumns.at(0)->SetWidth(GridColumnBinding::WidthType::Pixels, nWidth);
+}
+
 // ------------------------------------
 
 class SearchResultValueColumnBinding : public ra::ui::win32::bindings::GridTextColumnBinding
@@ -200,6 +204,8 @@ MemoryInspectorDialog::MemoryInspectorDialog(MemoryInspectorViewModel& vmMemoryI
     pDescriptionColumn->SetTextColorProperty(MemorySearchViewModel::SearchResultViewModel::DescriptionColorProperty);
     m_bindSearchResults.BindColumn(2, std::move(pDescriptionColumn));
 
+    m_bindSearchResults.UpdateColumnWidths();
+
     m_bindSearchResults.BindItems(vmMemoryInspector.Search().Results());
     m_bindSearchResults.BindRowColor(MemorySearchViewModel::SearchResultViewModel::RowColorProperty);
     m_bindSearchResults.BindIsSelected(MemorySearchViewModel::SearchResultViewModel::IsSelectedProperty);
diff --git a/src/ui/win32/MemoryInspectorDialog.hh b/src/ui/win32/MemoryInspectorDialog.hh
index 4ed772c6..15e0d9ea 100644
--- a/src/ui/win32/MemoryInspectorDialog.hh
+++ b/src/ui/win32/MemoryInspectorDialog.hh
@@ -66,6 +66,8 @@ private:
         void OnLvnItemChanged(const LPNMLISTVIEW pnmListView) override;
 
         void OnViewModelIntValueChanged(const IntModelProperty::ChangeArgs& args) override;
+
+        void UpdateColumnWidths();
     };
 
     SearchResultsGridBinding m_bindSearchResults;
diff --git a/src/ui/win32/bindings/GridColumnBinding.hh b/src/ui/win32/bindings/GridColumnBinding.hh
index cfd7743e..149ab31a 100644
--- a/src/ui/win32/bindings/GridColumnBinding.hh
+++ b/src/ui/win32/bindings/GridColumnBinding.hh
@@ -49,7 +49,10 @@ public:
     virtual std::wstring GetTooltip(_UNUSED const ra::ui::ViewModelCollectionBase& vmItems, _UNUSED gsl::index nIndex) const { return L""; }
 
     virtual bool DependsOn(const ra::ui::BoolModelProperty&) const noexcept(false) { return false; }
-    virtual bool DependsOn(const ra::ui::IntModelProperty&) const noexcept(false) { return false; }
+    virtual bool DependsOn(const ra::ui::IntModelProperty& pProperty) const noexcept(false)
+    {
+        return (m_pTextColorProperty != nullptr && pProperty == *m_pTextColorProperty);
+    }
     virtual bool DependsOn(const ra::ui::StringModelProperty&) const noexcept(false) { return false; }
 
     ra::ui::RelativePosition GetAlignment() const noexcept { return m_nAlignment; }


Test patch:
diff --git a/tests/data/models/CodeNotesModel_Tests.cpp b/tests/data/models/CodeNotesModel_Tests.cpp
index 3e156aa5..6380d4e1 100644
--- a/tests/data/models/CodeNotesModel_Tests.cpp
+++ b/tests/data/models/CodeNotesModel_Tests.cpp
@@ -452,15 +452,19 @@ public:
         });
 
         notes.SetGameId(1U);
-        notes.SetCodeNote(1234, L"Note1");
+        notes.SetCodeNote(1234U, L"Note1");
+        notes.SetServerCodeNote(1234U, L"Note1");
 
         notes.AssertNote(1234U, L"Note1");
 
-        notes.SetCodeNote(1234, L"");
-        const auto* pNote1b = notes.FindCodeNote(1234U);
-        Assert::IsNull(pNote1b);
+        // setting a note to blank does not actually delete it until it's committed
+        notes.SetCodeNote(1234U, L"");
+        notes.AssertNote(1234U, L"");
+        Assert::IsTrue(notes.IsNoteModified(1234U));
 
-        Assert::AreEqual(std::wstring(), notes.mNewNotes[1234U]);
+        // committed deleted note should no longer exist
+        notes.SetServerCodeNote(1234U, L"");
+        notes.AssertNoNote(1234U);
     }
 
     TEST_METHOD(TestDeleteCodeNoteNonExistant)
@@ -662,6 +666,104 @@ public:
         Assert::AreEqual(std::wstring(), notes.FindCodeNote(18, MemSize::SixteenBit));
     }
 
+    TEST_METHOD(TestEnumerateCodeNotes)
+    {
+        CodeNotesModelHarness notes;
+        const std::wstring sPointerNote =
+            L"Pointer\n"
+            L"+8 = Unknown\n"
+            L"+16 = Small (8-bit)";
+        notes.AddCodeNote(1234, "Author", sPointerNote);
+        notes.AddCodeNote(20, "Author", L"After [32-bit]");
+        notes.AddCodeNote(4, "Author", L"Before");
+        notes.AddCodeNote(12, "Author", L"In the middle");
+
+        int i = 0;
+        notes.EnumerateCodeNotes([&i, &sPointerNote](ra::ByteAddress nAddress, unsigned nBytes, const std::wstring& sNote) {
+            switch (i++)
+            {
+                case 0:
+                    Assert::AreEqual({4U}, nAddress);
+                    Assert::AreEqual(1U, nBytes);
+                    Assert::AreEqual(std::wstring(L"Before"), sNote);
+                    break;
+                case 1:
+                    Assert::AreEqual({12U}, nAddress);
+                    Assert::AreEqual(1U, nBytes);
+                    Assert::AreEqual(std::wstring(L"In the middle"), sNote);
+                    break;
+                case 2:
+                    Assert::AreEqual({20U}, nAddress);
+                    Assert::AreEqual(4U, nBytes);
+                    Assert::AreEqual(std::wstring(L"After [32-bit]"), sNote);
+                    break;
+                case 3:
+                    Assert::AreEqual({1234U}, nAddress);
+                    Assert::AreEqual(4U, nBytes); // unspecified size on pointer assumed to be 32-bit
+                    Assert::AreEqual(sPointerNote, sNote);
+                    break;
+                case 4:
+                    Assert::Fail(L"Too many notes");
+                    break;
+            }
+            return true;
+        }, false);
+    }
+
+    TEST_METHOD(TestEnumerateCodeNotesWithIndirect)
+    {
+        CodeNotesModelHarness notes;
+        const std::wstring sPointerNote =
+            L"Pointer\n"
+            L"+8 = Unknown\n"
+            L"+16 = Small (16-bit)";
+        notes.AddCodeNote(1234, "Author", sPointerNote);
+        notes.AddCodeNote(20, "Author", L"After [32-bit]");
+        notes.AddCodeNote(4, "Author", L"Before");
+        notes.AddCodeNote(12, "Author", L"In the middle");
+
+        int i = 0;
+        notes.EnumerateCodeNotes([&i, &sPointerNote](ra::ByteAddress nAddress, unsigned nBytes, const std::wstring& sNote) {
+            switch (i++)
+            {
+                case 0:
+                    Assert::AreEqual({4U}, nAddress);
+                    Assert::AreEqual(1U, nBytes);
+                    Assert::AreEqual(std::wstring(L"Before"), sNote);
+                    break;
+                case 1:
+                    Assert::AreEqual({8U}, nAddress);
+                    Assert::AreEqual(1U, nBytes);
+                    Assert::AreEqual(std::wstring(L"Unknown"), sNote);
+                    break;
+                case 2:
+                    Assert::AreEqual({12U}, nAddress);
+                    Assert::AreEqual(1U, nBytes);
+                    Assert::AreEqual(std::wstring(L"In the middle"), sNote);
+                    break;
+                case 3:
+                    Assert::AreEqual({16U}, nAddress);
+                    Assert::AreEqual(2U, nBytes);
+                    Assert::AreEqual(std::wstring(L"Small (16-bit)"), sNote);
+                    break;
+                case 4:
+                    Assert::AreEqual({20U}, nAddress);
+                    Assert::AreEqual(4U, nBytes);
+                    Assert::AreEqual(std::wstring(L"After [32-bit]"), sNote);
+                    break;
+                case 5:
+                    Assert::AreEqual({1234U}, nAddress);
+                    Assert::AreEqual(4U, nBytes); // unspecified size on pointer assumed to be 32-bit
+                    Assert::AreEqual(sPointerNote, sNote);
+                    break;
+                case 6:
+                    Assert::Fail(L"Too many notes");
+                    break;
+            }
+            return true;
+        }, true);
+    }
+
     TEST_METHOD(TestDoFrame)
     {
         CodeNotesModelHarness notes;
@@ -913,6 +1015,13 @@ public:
 
         Assert::IsTrue(notes.Deserialize(pTokenizer));
         Assert::AreEqual(AssetChanges::Unpublished, notes.GetChanges());
+
+        // uncommitted deleted note still exists
+        notes.AssertNote(0x1234U, L"");
+        Assert::IsTrue(notes.IsNoteModified(0x1234U));
+
+        // committed deleted note does not exist
+        notes.SetServerCodeNote(0x1234U, L"");
         notes.AssertNoNote(0x1234);
     }
 };
diff --git a/tests/ui/viewmodels/CodeNotesViewModel_Tests.cpp b/tests/ui/viewmodels/CodeNotesViewModel_Tests.cpp
index 2451c032..ba24f6b9 100644
--- a/tests/ui/viewmodels/CodeNotesViewModel_Tests.cpp
+++ b/tests/ui/viewmodels/CodeNotesViewModel_Tests.cpp
@@ -249,6 +249,40 @@ public:
         AssertRow(notes, 8, 0x0030, L"0x0030", L"Item 1 Quantity");
     }
 
+    TEST_METHOD(TestUpdateNoteIndirectUnfiltered)
+    {
+        CodeNotesViewModelHarness notes;
+        notes.PopulateNotes();
+        Assert::AreEqual({ 0U }, notes.Notes().Count());
+
+        const wchar_t* sPointerNote =
+            L"Pointer\n"
+            L"+8 = Unknown\n"        // no note at $0008
+            L"+16 = Small (16-bit)"; // note would exist at $0010
+
+        notes.SetIsVisible(true);
+        Assert::AreEqual({ 14U }, notes.Notes().Count());
+        Assert::AreEqual(std::wstring(L""), notes.GetFilterValue());
+        Assert::AreEqual(std::wstring(L"14/14"), notes.GetResultCount());
+
+        // This creates an indirect note at $0008, which would bring the total
+        // note count to 15. Make sure it's not added to the list.
+        notes.mockGameContext.SetCodeNote(0x0022, sPointerNote);
+
+        Assert::AreEqual({ 14U }, notes.Notes().Count());
+        Assert::AreEqual(std::wstring(L""), notes.GetFilterValue());
+        Assert::AreEqual(std::wstring(L"14/14"), notes.GetResultCount());
+
+        // indirect 0+8 = 8, would be first element.
+        // indirect 0+16 would be $0010.
+        // this one assert ensures the first item is not the indirect 8, or the note from the indirect 16
+        AssertRow(notes, 0, 0x0010, L"0x0010", L"Score X000");
+
+        // also validate the pointer note itself and that it wasn't duplicated
+        AssertRow(notes, 7, 0x0022, L"0x0022", sPointerNote);
+        AssertRow(notes, 8, 0x0030, L"0x0030", L"Item 1 Quantity");
+    }
+
     TEST_METHOD(TestUpdateNoteFilterStillApplies)
     {
         CodeNotesViewModelHarness notes;
@@ -369,13 +403,26 @@ public:
         Assert::AreEqual(std::wstring(L""), notes.GetFilterValue());
         Assert::AreEqual(std::wstring(L"14/14"), notes.GetResultCount());
 
-        notes.mockGameContext.DeleteCodeNote(0x0022);
+        // non-committed deleted note should still be visible in list (as [Deleted])
+        auto* pCodeNotes = notes.mockGameContext.Assets().FindCodeNotes();
+        Assert::IsNotNull(pCodeNotes);
+        Ensures(pCodeNotes != nullptr);
+        pCodeNotes->SetCodeNote(0x0022, L"");
+
+        Assert::AreEqual({ 14U }, notes.Notes().Count());
+        Assert::AreEqual(std::wstring(L""), notes.GetFilterValue());
+        Assert::AreEqual(std::wstring(L"14/14"), notes.GetResultCount());
+
+        AssertRow(notes, 7, 0x0022, L"0x0022", L"[Deleted]");
+        AssertRow(notes, 8, 0x0030, L"0x0030", L"Item 1 Quantity");
+
+        // committed deleted note should be removed from the list
+        pCodeNotes->SetServerCodeNote(0x0022, L"");
 
         Assert::AreEqual({ 13U }, notes.Notes().Count());
         Assert::AreEqual(std::wstring(L""), notes.GetFilterValue());
         Assert::AreEqual(std::wstring(L"13/13"), notes.GetResultCount());
 
-        AssertRow(notes, 6, 0x0020, L"0x0020", L"[16-bit] Max HP");
         AssertRow(notes, 7, 0x0030, L"0x0030", L"Item 1 Quantity");
     }
 
@@ -395,7 +442,20 @@ public:
         Assert::AreEqual({ 6U }, notes.Notes().Count());
         Assert::AreEqual(std::wstring(L"6/14"), notes.GetResultCount());
 
-        notes.mockGameContext.DeleteCodeNote(0x0031);
+        // non-committed deleted note will no longer matches filter (but still gets counted)
+        auto* pCodeNotes = notes.mockGameContext.Assets().FindCodeNotes();
+        Assert::IsNotNull(pCodeNotes);
+        Ensures(pCodeNotes != nullptr);
+        pCodeNotes->SetCodeNote(0x0031, L"");
+
+        Assert::AreEqual({ 5U }, notes.Notes().Count());
+        Assert::AreEqual(std::wstring(L"5/14"), notes.GetResultCount());
+        AssertRow(notes, 1, 0x0030, L"0x0030", L"Item 1 Quantity");
+        AssertRow(notes, 2, 0x0032, L"0x0032", L"Item 3 Quantity");
+
+        // committed deleted note no longer exists
+        pCodeNotes->SetServerCodeNote(0x0031, L"");
+
         Assert::AreEqual({ 5U }, notes.Notes().Count());
         Assert::AreEqual(std::wstring(L"5/13"), notes.GetResultCount());
         AssertRow(notes, 1, 0x0030, L"0x0030", L"Item 1 Quantity");
@@ -418,7 +478,19 @@ public:
         Assert::AreEqual({ 6U }, notes.Notes().Count());
         Assert::AreEqual(std::wstring(L"6/14"), notes.GetResultCount());
 
-        notes.mockGameContext.DeleteCodeNote(0x0022);
+        // non-committed deleted note not matching filter still gets counted
+        auto* pCodeNotes = notes.mockGameContext.Assets().FindCodeNotes();
+        Assert::IsNotNull(pCodeNotes);
+        Ensures(pCodeNotes != nullptr);
+        pCodeNotes->SetCodeNote(0x0022, L"");
+
+        Assert::AreEqual({ 6U }, notes.Notes().Count());
+        Assert::AreEqual(std::wstring(L"6/14"), notes.GetResultCount());
+        AssertRow(notes, 1, 0x0030, L"0x0030", L"Item 1 Quantity");
+
+        // committed deleted note no longer exists
+        pCodeNotes->SetServerCodeNote(0x0022, L"");
+
         Assert::AreEqual({ 6U }, notes.Notes().Count());
         Assert::AreEqual(std::wstring(L"6/13"), notes.GetResultCount());
         AssertRow(notes, 1, 0x0030, L"0x0030", L"Item 1 Quantity");