-
Notifications
You must be signed in to change notification settings - Fork 38
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Simplify user-facing references to the Wii's extended memory segment #10
Conversation
Sorry for the delay on this one, but after the recent release, I needed a little break :) I thought about it and I don't think it's a good idea. The reasoning behind is that it hides the fact there's 2 distinct memory regsions. Now, in the context of the watcher and scanner, this is fine, it's abstracted as one contiguous block, but where it starts to get confusing is the viewer. The way the viewer works is I have to show both regions separately and you have to browse them separately, I even named MEM1 as "Common RAM" because it's common to both consoles. Another thing to consider is because they are different regions, appart from one another, it could potentially add meaning to the memory. For example, something being in MEM2 might not be in MEM1 for a reason, maybe the data is particularly big. This is why I made the viewer this way. Ultimately, what I want is the user to be aware that MEM2 is present and separated from MEM1. These changes makes it so that the former is respected, but not the later which might be a problem considering the aforementioned 2 reasons. I am open to abstract the details that are unecessary to the users (such as endianness, Dolphin's process and their addresses, etc...), but I feel this detail is necessary to present. This is why btw I renamed the name in the UI, the name MEM2 is confusing and giving a name that tells what it is, a region only present on a WIi, is enough. |
6b8f42a
to
2456d79
Compare
2456d79
to
44966e8
Compare
As discussed over discord, I agreed to rework the paragraph without the notion of the memory type. This PR will stay open until the paragraph rework is pushed. |
Can I get a status on this PR? |
Sorry… Life happened. Closing this myself until I catch back up on…well…everything. 😔 |
…n the stack. Previously, these dialogs were constructed dynamically in the heap, and were never deleted explicitly; only when their parent QObject was destroyed at a much later time would they be deleted. This deferred destruction was in fact masking a segmentation fault caused by a double-free: the entry in the dialog was not extracted via `stealEntry()`; instead the reference was taking from the `entryCopy` variable (same pointer), resulting in the entry getting deleted twice, eventually. Reproduction steps: - Double click on an watch entry to bring up the **Edit Watch** dialog. - Make any edit and press **OK**. - Quit the application gracefully to force the destruction of the dialog. - A segmentation fault would be produced: ``` (gdb) bt #0 QArrayDataPointer<char16_t>::deref() (this=<optimised out>, this=0x558b5d1a35c0) at /usr/include/c++/11/bits/atomic_base.h:392 aldelaro5#1 QArrayDataPointer<char16_t>::~QArrayDataPointer() (this=0x558b5d1a35c0, __in_chrg=<optimised out>) at /usr/include/x86_64-linux-gnu/qt6/QtCore/qarraydatapointer.h:129 aldelaro5#2 QString::~QString() (this=0x558b5d1a35c0, __in_chrg=<optimised out>) at /usr/include/x86_64-linux-gnu/qt6/QtCore/qstring.h:1302 aldelaro5#3 MemWatchEntry::~MemWatchEntry() (this=0x558b5d1a35c0, __in_chrg=<optimised out>) at /w/dolphin-memory-engine/Source/MemoryWatch/MemWatchEntry.cpp:48 aldelaro5#4 0x0000558b5c517787 in DlgAddWatchEntry::~DlgAddWatchEntry() (this=0x558b5d1be8f0, __in_chrg=<optimised out>) at /w/dolphin-memory-engine/Source/GUI/MemWatcher/Dialogs/DlgAddWatchEntry.cpp:31 aldelaro5#5 0x0000558b5c51782d in DlgAddWatchEntry::~DlgAddWatchEntry() (this=0x558b5d1be8f0, __in_chrg=<optimised out>) at /w/dolphin-memory-engine/Source/GUI/MemWatcher/Dialogs/DlgAddWatchEntry.cpp:32 aldelaro5#6 0x00007fde5556599b in QObjectPrivate::deleteChildren() () at /lib/x86_64-linux-gnu/libQt6Core.so.6 aldelaro5#7 0x00007fde5624cab8 in QWidget::~QWidget() () at /lib/x86_64-linux-gnu/libQt6Widgets.so.6 aldelaro5#8 0x0000558b5c51d0cd in MemWatchWidget::~MemWatchWidget() (this=0x558b5cd068e0, __in_chrg=<optimised out>) at /w/dolphin-memory-engine/Source/GUI/MemWatcher/MemWatchWidget.cpp:41 aldelaro5#9 0x0000558b5c5367cd in MainWindow::~MainWindow() (this=0x7ffc70ef10f0, __in_chrg=<optimised out>) at /w/dolphin-memory-engine/Source/GUI/MainWindow.cpp:56 aldelaro5#10 0x0000558b5c4f1ad7 in main(int, char**) (argc=<optimised out>, argv=<optimised out>) at /w/dolphin-memory-engine/Source/main.cpp:56 ``` # Please enter the commit message for your changes. Lines starting # with '#' will be kept; you may remove them yourself if you want to. # An empty message aborts the commit. # # Date: Fri May 17 22:10:10 2024 +0100 # # On branch watch_entry_dialog_segfault # Changes to be committed: # modified: Source/GUI/MemViewer/MemViewer.cpp # modified: Source/GUI/MemWatcher/MemWatchWidget.cpp #
…n the stack. Previously, these dialogs were constructed dynamically in the heap, and were never deleted explicitly; only when their parent QObject was destroyed at a much later time would they be deleted. This deferred destruction was in fact masking a segmentation fault caused by a double-free: the entry in the dialog was not extracted via `stealEntry()`; instead the reference was taking from the `entryCopy` variable (same pointer), resulting in the entry getting deleted twice, eventually. Reproduction steps: - Double click on an watch entry to bring up the **Edit Watch** dialog. - Make any edit and press **OK**. - Quit the application gracefully to force the destruction of the dialog. - A segmentation fault would be produced: ``` (gdb) bt #0 QArrayDataPointer<char16_t>::deref() (this=<optimised out>, this=0x558b5d1a35c0) at /usr/include/c++/11/bits/atomic_base.h:392 aldelaro5#1 QArrayDataPointer<char16_t>::~QArrayDataPointer() (this=0x558b5d1a35c0, __in_chrg=<optimised out>) at /usr/include/x86_64-linux-gnu/qt6/QtCore/qarraydatapointer.h:129 aldelaro5#2 QString::~QString() (this=0x558b5d1a35c0, __in_chrg=<optimised out>) at /usr/include/x86_64-linux-gnu/qt6/QtCore/qstring.h:1302 aldelaro5#3 MemWatchEntry::~MemWatchEntry() (this=0x558b5d1a35c0, __in_chrg=<optimised out>) at /w/dolphin-memory-engine/Source/MemoryWatch/MemWatchEntry.cpp:48 aldelaro5#4 0x0000558b5c517787 in DlgAddWatchEntry::~DlgAddWatchEntry() (this=0x558b5d1be8f0, __in_chrg=<optimised out>) at /w/dolphin-memory-engine/Source/GUI/MemWatcher/Dialogs/DlgAddWatchEntry.cpp:31 aldelaro5#5 0x0000558b5c51782d in DlgAddWatchEntry::~DlgAddWatchEntry() (this=0x558b5d1be8f0, __in_chrg=<optimised out>) at /w/dolphin-memory-engine/Source/GUI/MemWatcher/Dialogs/DlgAddWatchEntry.cpp:32 aldelaro5#6 0x00007fde5556599b in QObjectPrivate::deleteChildren() () at /lib/x86_64-linux-gnu/libQt6Core.so.6 aldelaro5#7 0x00007fde5624cab8 in QWidget::~QWidget() () at /lib/x86_64-linux-gnu/libQt6Widgets.so.6 aldelaro5#8 0x0000558b5c51d0cd in MemWatchWidget::~MemWatchWidget() (this=0x558b5cd068e0, __in_chrg=<optimised out>) at /w/dolphin-memory-engine/Source/GUI/MemWatcher/MemWatchWidget.cpp:41 aldelaro5#9 0x0000558b5c5367cd in MainWindow::~MainWindow() (this=0x7ffc70ef10f0, __in_chrg=<optimised out>) at /w/dolphin-memory-engine/Source/GUI/MainWindow.cpp:56 aldelaro5#10 0x0000558b5c4f1ad7 in main(int, char**) (argc=<optimised out>, argv=<optimised out>) at /w/dolphin-memory-engine/Source/main.cpp:56 ```
…n the stack. Previously, these dialogs were constructed dynamically in the heap, and were never deleted explicitly; only when their parent QObject was destroyed at a much later time would they be deleted. This deferred destruction was in fact masking a segmentation fault caused by a double-free: the entry in the dialog was not extracted via `stealEntry()`; instead the reference was taken from the `entryCopy` variable (same pointer), resulting in the entry getting deleted twice, eventually. Reproduction steps: - Double-click on an watch entry to bring up the **Edit Watch** dialog. - Make any edit and press **OK**. - Quit the application gracefully to force the destruction of the dialog. - A segmentation fault would be produced: ``` (gdb) bt #0 QArrayDataPointer<char16_t>::deref() (this=<optimised out>, this=0x558b5d1a35c0) at /usr/include/c++/11/bits/atomic_base.h:392 aldelaro5#1 QArrayDataPointer<char16_t>::~QArrayDataPointer() (this=0x558b5d1a35c0, __in_chrg=<optimised out>) at /usr/include/x86_64-linux-gnu/qt6/QtCore/qarraydatapointer.h:129 aldelaro5#2 QString::~QString() (this=0x558b5d1a35c0, __in_chrg=<optimised out>) at /usr/include/x86_64-linux-gnu/qt6/QtCore/qstring.h:1302 aldelaro5#3 MemWatchEntry::~MemWatchEntry() (this=0x558b5d1a35c0, __in_chrg=<optimised out>) at /w/dolphin-memory-engine/Source/MemoryWatch/MemWatchEntry.cpp:48 aldelaro5#4 0x0000558b5c517787 in DlgAddWatchEntry::~DlgAddWatchEntry() (this=0x558b5d1be8f0, __in_chrg=<optimised out>) at /w/dolphin-memory-engine/Source/GUI/MemWatcher/Dialogs/DlgAddWatchEntry.cpp:31 aldelaro5#5 0x0000558b5c51782d in DlgAddWatchEntry::~DlgAddWatchEntry() (this=0x558b5d1be8f0, __in_chrg=<optimised out>) at /w/dolphin-memory-engine/Source/GUI/MemWatcher/Dialogs/DlgAddWatchEntry.cpp:32 aldelaro5#6 0x00007fde5556599b in QObjectPrivate::deleteChildren() () at /lib/x86_64-linux-gnu/libQt6Core.so.6 aldelaro5#7 0x00007fde5624cab8 in QWidget::~QWidget() () at /lib/x86_64-linux-gnu/libQt6Widgets.so.6 aldelaro5#8 0x0000558b5c51d0cd in MemWatchWidget::~MemWatchWidget() (this=0x558b5cd068e0, __in_chrg=<optimised out>) at /w/dolphin-memory-engine/Source/GUI/MemWatcher/MemWatchWidget.cpp:41 aldelaro5#9 0x0000558b5c5367cd in MainWindow::~MainWindow() (this=0x7ffc70ef10f0, __in_chrg=<optimised out>) at /w/dolphin-memory-engine/Source/GUI/MainWindow.cpp:56 aldelaro5#10 0x0000558b5c4f1ad7 in main(int, char**) (argc=<optimised out>, argv=<optimised out>) at /w/dolphin-memory-engine/Source/main.cpp:56 ```
…group node is right-clicked. This was a regression in aldelaro5#167. Group nodes do not have a watch entry associated with them. When a group node was right-clicked, a null pointer would be fatally dereferenced: ``` #0 MemWatchEntry::isBoundToPointer() const (this=this@entry=0x0) at /w/dolphin-memory-engine/Source/MemoryWatch/MemWatchEntry.cpp:77 aldelaro5#1 0x0000561a4ab96c94 in MemWatchWidget::onMemWatchContextMenuRequested(QPoint const&) (this=0x561a4c28c580, pos=...) at /w/dolphin-memory-engine/Source/GUI/MemWatcher/MemWatchWidget.cpp:280 aldelaro5#2 0x00007f56ac7be023 in () at /lib/x86_64-linux-gnu/libQt6Core.so.6 aldelaro5#3 0x00007f56ad489889 in QWidget::customContextMenuRequested(QPoint const&) () at /lib/x86_64-linux-gnu/libQt6Widgets.so.6 aldelaro5#4 0x00007f56ad4a6020 in QWidget::event(QEvent*) () at /lib/x86_64-linux-gnu/libQt6Widgets.so.6 aldelaro5#5 0x00007f56ad541406 in QFrame::event(QEvent*) () at /lib/x86_64-linux-gnu/libQt6Widgets.so.6 aldelaro5#6 0x00007f56ac765818 in QCoreApplicationPrivate::sendThroughObjectEventFilters(QObject*, QEvent*) () at /lib/x86_64-linux-gnu/libQt6Core.so.6 aldelaro5#7 0x00007f56ad44bd25 in QApplicationPrivate::notify_helper(QObject*, QEvent*) () at /lib/x86_64-linux-gnu/libQt6Widgets.so.6 aldelaro5#8 0x00007f56ad454c5e in QApplication::notify(QObject*, QEvent*) () at /lib/x86_64-linux-gnu/libQt6Widgets.so.6 aldelaro5#9 0x00007f56ac765a58 in QCoreApplication::notifyInternal2(QObject*, QEvent*) () at /lib/x86_64-linux-gnu/libQt6Core.so.6 aldelaro5#10 0x00007f56ad4b796c in () at /lib/x86_64-linux-gnu/libQt6Widgets.so.6 aldelaro5#11 0x00007f56ad4ba635 in () at /lib/x86_64-linux-gnu/libQt6Widgets.so.6 aldelaro5#12 0x00007f56ad44bd36 in QApplicationPrivate::notify_helper(QObject*, QEvent*) () at /lib/x86_64-linux-gnu/libQt6Widgets.so.6 aldelaro5#13 0x00007f56ac765a58 in QCoreApplication::notifyInternal2(QObject*, QEvent*) () at /lib/x86_64-linux-gnu/libQt6Core.so.6 aldelaro5#14 0x00007f56acd0a6bf in QGuiApplicationPrivate::processMouseEvent(QWindowSystemInterfacePrivate::MouseEvent*) () at /lib/x86_64-linux-gnu/libQt6Gui.so.6 aldelaro5#15 0x00007f56acd52c8c in QWindowSystemInterface::sendWindowSystemEvents(QFlags<QEventLoop::ProcessEventsFlag>) () at /lib/x86_64-linux-gnu/libQt6Gui.so.6 aldelaro5#16 0x00007f56a8c7686e in () at /usr/lib/x86_64-linux-gnu/qt6/plugins/platforms/../../../libQt6XcbQpa.so.6 aldelaro5#17 0x00007f56abe32d3b in g_main_context_dispatch () at /lib/x86_64-linux-gnu/libglib-2.0.so.0 aldelaro5#18 0x00007f56abe882b8 in () at /lib/x86_64-linux-gnu/libglib-2.0.so.0 aldelaro5#19 0x00007f56abe303e3 in g_main_context_iteration () at /lib/x86_64-linux-gnu/libglib-2.0.so.0 aldelaro5#20 0x00007f56ac98deae in QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) () at /lib/x86_64-linux-gnu/libQt6Core.so.6 aldelaro5#21 0x00007f56ac772adb in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) () at /lib/x86_64-linux-gnu/libQt6Core.so.6 aldelaro5#22 0x00007f56ac76e0f3 in QCoreApplication::exec() () at /lib/x86_64-linux-gnu/libQt6Core.so.6 aldelaro5#23 0x0000561a4ab66e8c in main(int, char**) (argc=<optimised out>, argv=<optimised out>) at /w/dolphin-memory-engine/Source/main.cpp:54 ``` Fixes aldelaro5#170.
…group node is right-clicked. This was a regression in #167. Group nodes do not have a watch entry associated with them. When a group node was right-clicked, a null pointer would be fatally dereferenced: ``` #0 MemWatchEntry::isBoundToPointer() const (this=this@entry=0x0) at /w/dolphin-memory-engine/Source/MemoryWatch/MemWatchEntry.cpp:77 #1 0x0000561a4ab96c94 in MemWatchWidget::onMemWatchContextMenuRequested(QPoint const&) (this=0x561a4c28c580, pos=...) at /w/dolphin-memory-engine/Source/GUI/MemWatcher/MemWatchWidget.cpp:280 #2 0x00007f56ac7be023 in () at /lib/x86_64-linux-gnu/libQt6Core.so.6 #3 0x00007f56ad489889 in QWidget::customContextMenuRequested(QPoint const&) () at /lib/x86_64-linux-gnu/libQt6Widgets.so.6 #4 0x00007f56ad4a6020 in QWidget::event(QEvent*) () at /lib/x86_64-linux-gnu/libQt6Widgets.so.6 #5 0x00007f56ad541406 in QFrame::event(QEvent*) () at /lib/x86_64-linux-gnu/libQt6Widgets.so.6 #6 0x00007f56ac765818 in QCoreApplicationPrivate::sendThroughObjectEventFilters(QObject*, QEvent*) () at /lib/x86_64-linux-gnu/libQt6Core.so.6 #7 0x00007f56ad44bd25 in QApplicationPrivate::notify_helper(QObject*, QEvent*) () at /lib/x86_64-linux-gnu/libQt6Widgets.so.6 #8 0x00007f56ad454c5e in QApplication::notify(QObject*, QEvent*) () at /lib/x86_64-linux-gnu/libQt6Widgets.so.6 #9 0x00007f56ac765a58 in QCoreApplication::notifyInternal2(QObject*, QEvent*) () at /lib/x86_64-linux-gnu/libQt6Core.so.6 #10 0x00007f56ad4b796c in () at /lib/x86_64-linux-gnu/libQt6Widgets.so.6 #11 0x00007f56ad4ba635 in () at /lib/x86_64-linux-gnu/libQt6Widgets.so.6 #12 0x00007f56ad44bd36 in QApplicationPrivate::notify_helper(QObject*, QEvent*) () at /lib/x86_64-linux-gnu/libQt6Widgets.so.6 #13 0x00007f56ac765a58 in QCoreApplication::notifyInternal2(QObject*, QEvent*) () at /lib/x86_64-linux-gnu/libQt6Core.so.6 #14 0x00007f56acd0a6bf in QGuiApplicationPrivate::processMouseEvent(QWindowSystemInterfacePrivate::MouseEvent*) () at /lib/x86_64-linux-gnu/libQt6Gui.so.6 #15 0x00007f56acd52c8c in QWindowSystemInterface::sendWindowSystemEvents(QFlags<QEventLoop::ProcessEventsFlag>) () at /lib/x86_64-linux-gnu/libQt6Gui.so.6 #16 0x00007f56a8c7686e in () at /usr/lib/x86_64-linux-gnu/qt6/plugins/platforms/../../../libQt6XcbQpa.so.6 #17 0x00007f56abe32d3b in g_main_context_dispatch () at /lib/x86_64-linux-gnu/libglib-2.0.so.0 #18 0x00007f56abe882b8 in () at /lib/x86_64-linux-gnu/libglib-2.0.so.0 #19 0x00007f56abe303e3 in g_main_context_iteration () at /lib/x86_64-linux-gnu/libglib-2.0.so.0 #20 0x00007f56ac98deae in QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) () at /lib/x86_64-linux-gnu/libQt6Core.so.6 #21 0x00007f56ac772adb in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) () at /lib/x86_64-linux-gnu/libQt6Core.so.6 #22 0x00007f56ac76e0f3 in QCoreApplication::exec() () at /lib/x86_64-linux-gnu/libQt6Core.so.6 #23 0x0000561a4ab66e8c in main(int, char**) (argc=<optimised out>, argv=<optimised out>) at /w/dolphin-memory-engine/Source/main.cpp:54 ``` Fixes #170.
So here's part two to my idea for a better rewrite as I mentioned to @aldelaro5 before. “Wii-only extra memory” is a mouthful and kinda awkward, so I sought to come up with a better solution on how to refer to it.
Then there's how it's represented in the GUI. The two states are currently presented with longer strings of text that are only 7.69% different; way too easy to misread in my opinion. And so I also set out to improve that situation too.
My end result is this proposal to start calling it the active “memory type”. This makes things short and sweet, but also differentiates the GUI representation much more. Take a look and compare them for yourself:
Before:
After:
This way the user's attention is drawn more towards the information we want to convey and they are much more unlikely to misread and confuse which state is active. The change also helps streamline the topic of MEM2's presence (or lack thereof) in discussion and consequently in documentation as well.
Instead of saying “make sure the Wii-only extra memory isn't there when you're doing GameCube stuff” one can just say “make sure the memory type matches what you're working with”, if it's even necessary to tell someone that. The change should also make it much more intuitive.