From 4614685fac1fe666a0c77028843f9c6674d1f824 Mon Sep 17 00:00:00 2001 From: Marc Van de Wiele Date: Thu, 25 Apr 2024 22:49:30 +0200 Subject: [PATCH 1/2] Add History to Memory Viewer Enhanced user experience by providing an history on the memory viewer with the last 50 explored addresses (can move backward or forward) --- .../viewmodels/MemoryInspectorViewModel.cpp | 1 + src/ui/viewmodels/MemoryViewerViewModel.cpp | 30 +++++++++++++++++++ src/ui/viewmodels/MemoryViewerViewModel.hh | 6 ++++ .../bindings/MemoryViewerControlBinding.cpp | 7 +++++ 4 files changed, 44 insertions(+) diff --git a/src/ui/viewmodels/MemoryInspectorViewModel.cpp b/src/ui/viewmodels/MemoryInspectorViewModel.cpp index de7061d0a..8812a86a3 100644 --- a/src/ui/viewmodels/MemoryInspectorViewModel.cpp +++ b/src/ui/viewmodels/MemoryInspectorViewModel.cpp @@ -287,6 +287,7 @@ void MemoryInspectorViewModel::OnCurrentAddressChanged(ra::ByteAddress nNewAddre SetValue(CurrentAddressValueProperty, nValue); m_pViewer.SetAddress(nNewAddress); + m_pViewer.SaveToMemViewHistory(); } void MemoryInspectorViewModel::BookmarkCurrentAddress() const diff --git a/src/ui/viewmodels/MemoryViewerViewModel.cpp b/src/ui/viewmodels/MemoryViewerViewModel.cpp index b44ca9e8e..c70289415 100644 --- a/src/ui/viewmodels/MemoryViewerViewModel.cpp +++ b/src/ui/viewmodels/MemoryViewerViewModel.cpp @@ -23,11 +23,14 @@ constexpr uint8_t HIGHLIGHTED_COLOR = STALE_COLOR | gsl::narrow_cast(ra constexpr int ADDRESS_COLUMN_WIDTH = 10; constexpr int BASE_MEMORY_VIEWER_WIDTH_IN_CHARACTERS = (ADDRESS_COLUMN_WIDTH + 16 * 3 - 1); // address column + 16 bytes "XX " - last space +constexpr int MEMVIEW_HISTORY_MAX_SIZE = 50; std::unique_ptr MemoryViewerViewModel::s_pFontSurface; std::unique_ptr MemoryViewerViewModel::s_pFontASCIISurface; ra::ui::Size MemoryViewerViewModel::s_szChar; int MemoryViewerViewModel::s_nFont = 0; +std::list MemoryViewerViewModel::s_listMemViewHistory = {0}; +std::list::iterator MemoryViewerViewModel::s_iterMemViewHistoryIndex = s_listMemViewHistory.begin(); constexpr char FIRST_ASCII_CHAR = 32; constexpr char LAST_ASCII_CHAR = 127; @@ -1314,6 +1317,33 @@ void MemoryViewerViewModel::RenderHeader() } } +void MemoryViewerViewModel::SaveToMemViewHistory() +{ + if (*s_iterMemViewHistoryIndex != GetAddress()) + { + s_listMemViewHistory.erase(s_listMemViewHistory.begin(), s_iterMemViewHistoryIndex); + if (s_listMemViewHistory.size() > MEMVIEW_HISTORY_MAX_SIZE) + s_listMemViewHistory.pop_back(); + s_listMemViewHistory.push_front(GetAddress()); + s_iterMemViewHistoryIndex = s_listMemViewHistory.begin(); + } +} + +void MemoryViewerViewModel::MoveMemViewHistoryForward() +{ + if (s_listMemViewHistory.size() >= 1 and s_iterMemViewHistoryIndex != s_listMemViewHistory.begin()) + --s_iterMemViewHistoryIndex; + SetAddress(*s_iterMemViewHistoryIndex); +} + +void MemoryViewerViewModel::MoveMemViewHistoryBackward() +{ + int iIndex = std::distance(s_listMemViewHistory.begin(), s_iterMemViewHistoryIndex); + if (s_listMemViewHistory.size() > (iIndex + 1)) + ++s_iterMemViewHistoryIndex; + SetAddress(*s_iterMemViewHistoryIndex); +} + } // namespace viewmodels } // namespace ui } // namespace ra diff --git a/src/ui/viewmodels/MemoryViewerViewModel.hh b/src/ui/viewmodels/MemoryViewerViewModel.hh index f295118cc..cef620a54 100644 --- a/src/ui/viewmodels/MemoryViewerViewModel.hh +++ b/src/ui/viewmodels/MemoryViewerViewModel.hh @@ -175,6 +175,10 @@ public: void AdvanceCursorPage(); void RetreatCursorPage(); + void SaveToMemViewHistory(); + void MoveMemViewHistoryBackward(); + void MoveMemViewHistoryForward(); + protected: void OnValueChanged(const IntModelProperty::ChangeArgs& args) override; @@ -235,6 +239,8 @@ private: static std::unique_ptr s_pFontSurface; static std::unique_ptr s_pFontASCIISurface; static int s_nFont; + static std::list s_listMemViewHistory; + static std::list::iterator s_iterMemViewHistoryIndex; class MemoryBookmarkMonitor; friend class MemoryBookmarkMonitor; diff --git a/src/ui/win32/bindings/MemoryViewerControlBinding.cpp b/src/ui/win32/bindings/MemoryViewerControlBinding.cpp index 9cf8128d0..677fc8f55 100644 --- a/src/ui/win32/bindings/MemoryViewerControlBinding.cpp +++ b/src/ui/win32/bindings/MemoryViewerControlBinding.cpp @@ -57,6 +57,13 @@ INT_PTR CALLBACK MemoryViewerControlBinding::WndProc(HWND hControl, UINT uMsg, W case WM_USER_INVALIDATE: Invalidate(); return FALSE; + + case WM_XBUTTONUP: + if (GET_XBUTTON_WPARAM(wParam) == XBUTTON1) + m_pViewModel.MoveMemViewHistoryBackward(); + else + m_pViewModel.MoveMemViewHistoryForward(); + return FALSE; } return ControlBinding::WndProc(hControl, uMsg, wParam, lParam); From b7488ab0e0ffef7914c04db10d9d18fa431a0452 Mon Sep 17 00:00:00 2001 From: Marc Van de Wiele Date: Mon, 29 Apr 2024 22:24:19 +0200 Subject: [PATCH 2/2] Refactor history feature following review - Update naming - Move the history list to vector - Add ClearHistory function - Refactor of the AddToHistory logic - Add unit tests --- .../viewmodels/MemoryInspectorViewModel.cpp | 1 - src/ui/viewmodels/MemoryViewerViewModel.cpp | 50 ++++++++++++------- src/ui/viewmodels/MemoryViewerViewModel.hh | 14 ++++-- .../bindings/MemoryViewerControlBinding.cpp | 6 +-- .../MemoryViewerViewModel_Tests.cpp | 48 ++++++++++++++++++ 5 files changed, 92 insertions(+), 27 deletions(-) diff --git a/src/ui/viewmodels/MemoryInspectorViewModel.cpp b/src/ui/viewmodels/MemoryInspectorViewModel.cpp index 8812a86a3..de7061d0a 100644 --- a/src/ui/viewmodels/MemoryInspectorViewModel.cpp +++ b/src/ui/viewmodels/MemoryInspectorViewModel.cpp @@ -287,7 +287,6 @@ void MemoryInspectorViewModel::OnCurrentAddressChanged(ra::ByteAddress nNewAddre SetValue(CurrentAddressValueProperty, nValue); m_pViewer.SetAddress(nNewAddress); - m_pViewer.SaveToMemViewHistory(); } void MemoryInspectorViewModel::BookmarkCurrentAddress() const diff --git a/src/ui/viewmodels/MemoryViewerViewModel.cpp b/src/ui/viewmodels/MemoryViewerViewModel.cpp index c70289415..4191e6265 100644 --- a/src/ui/viewmodels/MemoryViewerViewModel.cpp +++ b/src/ui/viewmodels/MemoryViewerViewModel.cpp @@ -29,8 +29,6 @@ std::unique_ptr MemoryViewerViewModel::s_pFontSurface std::unique_ptr MemoryViewerViewModel::s_pFontASCIISurface; ra::ui::Size MemoryViewerViewModel::s_szChar; int MemoryViewerViewModel::s_nFont = 0; -std::list MemoryViewerViewModel::s_listMemViewHistory = {0}; -std::list::iterator MemoryViewerViewModel::s_iterMemViewHistoryIndex = s_listMemViewHistory.begin(); constexpr char FIRST_ASCII_CHAR = 32; constexpr char LAST_ASCII_CHAR = 127; @@ -97,6 +95,8 @@ class MemoryViewerViewModel::MemoryBookmarkMonitor : protected ViewModelCollecti private: ViewModelCollection& m_vBookmarks; MemoryViewerViewModel& m_pOwner; + std::vector vHistory; + std::vector::iterator iterHistoryIndex; }; MemoryViewerViewModel::MemoryViewerViewModel() @@ -113,6 +113,9 @@ MemoryViewerViewModel::MemoryViewerViewModel() m_pInvalid = m_pMemory + MaxLines * 16 * 2; memset(m_pInvalid, 0, MaxLines * 16); + + vHistory = {0}; + iterHistoryIndex = vHistory.begin(); } void MemoryViewerViewModel::InitializeNotifyTargets() @@ -369,6 +372,8 @@ void MemoryViewerViewModel::SetAddress(ra::ByteAddress nValue) { SetValue(PendingAddressProperty, gsl::narrow_cast(nValue)); } + + AddToHistory(nValue); } void MemoryViewerViewModel::SetFirstAddress(ra::ByteAddress value) @@ -669,6 +674,8 @@ void MemoryViewerViewModel::OnActiveGameChanged() const auto& pGameContext = ra::services::ServiceLocator::Get(); m_bReadOnly = (pGameContext.GameId() == 0); + + ClearHistory(); } void MemoryViewerViewModel::OnCodeNoteChanged(ra::ByteAddress nAddress, const std::wstring& sNote) @@ -1317,31 +1324,38 @@ void MemoryViewerViewModel::RenderHeader() } } -void MemoryViewerViewModel::SaveToMemViewHistory() +void MemoryViewerViewModel::AddToHistory(ra::ByteAddress nAddress) { - if (*s_iterMemViewHistoryIndex != GetAddress()) + if (*iterHistoryIndex != nAddress) { - s_listMemViewHistory.erase(s_listMemViewHistory.begin(), s_iterMemViewHistoryIndex); - if (s_listMemViewHistory.size() > MEMVIEW_HISTORY_MAX_SIZE) - s_listMemViewHistory.pop_back(); - s_listMemViewHistory.push_front(GetAddress()); - s_iterMemViewHistoryIndex = s_listMemViewHistory.begin(); + vHistory.erase(iterHistoryIndex + 1, vHistory.end()); + if (vHistory.size() == MEMVIEW_HISTORY_MAX_SIZE) + vHistory.erase(vHistory.begin()); + vHistory.push_back(nAddress); + iterHistoryIndex = vHistory.end() - 1; } } -void MemoryViewerViewModel::MoveMemViewHistoryForward() +void MemoryViewerViewModel::ClearHistory() +{ + vHistory.erase(vHistory.begin(), vHistory.end()); + vHistory.push_back(0); + iterHistoryIndex = vHistory.begin(); +} + +void MemoryViewerViewModel::MoveHistoryForward() { - if (s_listMemViewHistory.size() >= 1 and s_iterMemViewHistoryIndex != s_listMemViewHistory.begin()) - --s_iterMemViewHistoryIndex; - SetAddress(*s_iterMemViewHistoryIndex); + int nIndex = std::distance(vHistory.begin(), iterHistoryIndex); + if (vHistory.size() > (nIndex + 1)) + ++iterHistoryIndex; + SetAddress(*iterHistoryIndex); } -void MemoryViewerViewModel::MoveMemViewHistoryBackward() +void MemoryViewerViewModel::MoveHistoryBackward() { - int iIndex = std::distance(s_listMemViewHistory.begin(), s_iterMemViewHistoryIndex); - if (s_listMemViewHistory.size() > (iIndex + 1)) - ++s_iterMemViewHistoryIndex; - SetAddress(*s_iterMemViewHistoryIndex); + if (vHistory.size() >= 1 and iterHistoryIndex != vHistory.begin()) + --iterHistoryIndex; + SetAddress(*iterHistoryIndex); } } // namespace viewmodels diff --git a/src/ui/viewmodels/MemoryViewerViewModel.hh b/src/ui/viewmodels/MemoryViewerViewModel.hh index cef620a54..8b2d9d70e 100644 --- a/src/ui/viewmodels/MemoryViewerViewModel.hh +++ b/src/ui/viewmodels/MemoryViewerViewModel.hh @@ -175,9 +175,10 @@ public: void AdvanceCursorPage(); void RetreatCursorPage(); - void SaveToMemViewHistory(); - void MoveMemViewHistoryBackward(); - void MoveMemViewHistoryForward(); + void AddToHistory(ra::ByteAddress nAddress); + void ClearHistory(); + void MoveHistoryBackward(); + void MoveHistoryForward(); protected: void OnValueChanged(const IntModelProperty::ChangeArgs& args) override; @@ -210,6 +211,8 @@ protected: static constexpr int REDRAW_ALL = REDRAW_MEMORY | REDRAW_ADDRESSES | REDRAW_HEADERS; int m_nNeedsRedraw = REDRAW_ALL; + + static constexpr int MaxLines = 128; private: @@ -239,12 +242,13 @@ private: static std::unique_ptr s_pFontSurface; static std::unique_ptr s_pFontASCIISurface; static int s_nFont; - static std::list s_listMemViewHistory; - static std::list::iterator s_iterMemViewHistoryIndex; class MemoryBookmarkMonitor; friend class MemoryBookmarkMonitor; std::unique_ptr m_pBookmarkMonitor; + + std::vector vHistory; + std::vector::iterator iterHistoryIndex; }; } // namespace viewmodels diff --git a/src/ui/win32/bindings/MemoryViewerControlBinding.cpp b/src/ui/win32/bindings/MemoryViewerControlBinding.cpp index 677fc8f55..930974309 100644 --- a/src/ui/win32/bindings/MemoryViewerControlBinding.cpp +++ b/src/ui/win32/bindings/MemoryViewerControlBinding.cpp @@ -60,9 +60,9 @@ INT_PTR CALLBACK MemoryViewerControlBinding::WndProc(HWND hControl, UINT uMsg, W case WM_XBUTTONUP: if (GET_XBUTTON_WPARAM(wParam) == XBUTTON1) - m_pViewModel.MoveMemViewHistoryBackward(); - else - m_pViewModel.MoveMemViewHistoryForward(); + m_pViewModel.MoveHistoryBackward(); + else if (GET_XBUTTON_WPARAM(wParam) == XBUTTON2) + m_pViewModel.MoveHistoryForward(); return FALSE; } diff --git a/tests/ui/viewmodels/MemoryViewerViewModel_Tests.cpp b/tests/ui/viewmodels/MemoryViewerViewModel_Tests.cpp index b900ee33b..81d16f2ba 100644 --- a/tests/ui/viewmodels/MemoryViewerViewModel_Tests.cpp +++ b/tests/ui/viewmodels/MemoryViewerViewModel_Tests.cpp @@ -1918,6 +1918,54 @@ TEST_CLASS(MemoryViewerViewModel_Tests) Assert::IsTrue(viewer.NeedsRedraw()); viewer.MockRender(); } + TEST_METHOD(TestOnMoveHistory) + { + MemoryViewerViewModelHarness viewer; + viewer.InitializeMemory(256); + + viewer.SetAddress(0U); + viewer.SetReadOnly(false); + + // Step backward one time should do nothing as it's the first move + viewer.MoveHistoryBackward(); + Assert::AreEqual({0U}, viewer.GetAddress()); + + // Move to 0x1 and step backward should lead to address 0x0 + viewer.SetAddress(1U); + viewer.MoveHistoryBackward(); + Assert::AreEqual({0U}, viewer.GetAddress()); + + // Move one step forward should lead to address 0x1 + viewer.MoveHistoryForward(); + Assert::AreEqual({1U}, viewer.GetAddress()); + + // Move to 0x1 then to 0x2 and finally to 0x3 and step backward one time should lead to address 0x2 + viewer.SetAddress(1U); + viewer.SetAddress(2U); + viewer.SetAddress(3U); + viewer.MoveHistoryBackward(); + Assert::AreEqual({2U}, viewer.GetAddress()); + + // Move two steps forward should lead to address 0x3 as it's the most recent history entry + viewer.MoveHistoryForward(); + viewer.MoveHistoryForward(); + Assert::AreEqual({3U}, viewer.GetAddress()); + + // Three last steps backward and we are back to square one at address 0x0 + viewer.MoveHistoryBackward(); + viewer.MoveHistoryBackward(); + viewer.MoveHistoryBackward(); + Assert::AreEqual({0U}, viewer.GetAddress()); + + // Change game to clear history + viewer.mockGameContext.NotifyActiveGameChanged(); + + // Move three steps forward to get back to 0x3 should do nothing as the game changed and history got cleared + viewer.MoveHistoryForward(); + viewer.MoveHistoryForward(); + viewer.MoveHistoryForward(); + Assert::AreEqual({0U}, viewer.GetAddress()); + } }; } // namespace tests