Skip to content
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

Debugger MemoryWidget: Add dual views #10578

Merged
merged 2 commits into from Apr 23, 2022

Conversation

TryTwo
Copy link
Contributor

@TryTwo TryTwo commented Apr 11, 2022

Relies on #10563. Restores the dual views ability that WX had, and improves on it.

PR_dual_view

Looking for input on how customizable it should be. Currently it's always 4 bytes per row and the left side is always hex. It auto chooses the size/columns based on what is on the right. The second display type box, for bytes per row, doesn't do anything in dual view, because it's always 4 bytes or 8 bytes for a double.

I think the left side always being hex is fine.

Currently Double has a small breakpoint display bug. I also just added the Hex8/Hex16 ability to the left side, so it got a little more complicated than just having Hex32. I'll maybe need to clean it up more.

@TryTwo TryTwo force-pushed the PR_MemoryWidget_Dual_Views branch 2 times, most recently from 1d7474a to e7d3291 Compare April 12, 2022 05:06
@TryTwo
Copy link
Contributor Author

TryTwo commented Apr 12, 2022

If I added a hex64 view type the mismatch for Double vs two hex32 columns could be corrected by displaying one hex64 column instead. Hex64 wouldn't need a UI option. If it's easier to read two hex32's then I'll just leave it with a few special rules to deal with the mismatch.

@AdmiralCurtiss
Copy link
Contributor

AdmiralCurtiss commented Apr 16, 2022

You need to rebase this now.

Also, I'm not really happy with this approach. I think it would be way more useful if the bytes-per-row still worked as set, and possibly even to have separate dropdowns for the left and right half's types actually the more i play with this the less sure if that's really necessary, left half hex feels very correct.

Some extra spacing between the two halves couldn't hurt either.

@TryTwo TryTwo force-pushed the PR_MemoryWidget_Dual_Views branch from e7d3291 to cc22f1a Compare April 17, 2022 07:16
@TryTwo
Copy link
Contributor Author

TryTwo commented Apr 17, 2022

First was rebase. Second is commit for larger size dual views. I'm not sure if you'll like it, so I left it as a second commit. It gets buggy if I have to stretch the table too far, I forget how to fix that atm.

Extra spacing might require an empty column between them.

Copy link
Contributor

@AdmiralCurtiss AdmiralCurtiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few nitpicks but otherwise this is fine. I'd still like to see a bit of a spacing between the two halves, and the ability to have the same hex view on both sides is kinda weird, though with the UI as-is it makes sense I suppose.

Source/Core/DolphinQt/Debugger/MemoryViewWidget.cpp Outdated Show resolved Hide resolved
Source/Core/DolphinQt/Debugger/MemoryViewWidget.cpp Outdated Show resolved Hide resolved
Source/Core/DolphinQt/Debugger/MemoryViewWidget.cpp Outdated Show resolved Hide resolved
Source/Core/DolphinQt/Debugger/MemoryViewWidget.cpp Outdated Show resolved Hide resolved
Source/Core/DolphinQt/Debugger/MemoryWidget.cpp Outdated Show resolved Hide resolved
@TryTwo TryTwo force-pushed the PR_MemoryWidget_Dual_Views branch from f605f06 to 83909c6 Compare April 19, 2022 05:50
@TryTwo
Copy link
Contributor Author

TryTwo commented Apr 19, 2022

Trying to just use Hex64 when dual view + double is selected.
Removed Hex + Hex dual view.
Added some spacing for dual view.

Should I have it save all the display type settings between sessions? Also haven't worked on breakpoints yet.

@@ -91,31 +92,34 @@ static int GetTypeSize(MemoryViewWidget::Type type)

static int GetCharacterCount(MemoryViewWidget::Type type)
Copy link
Contributor

@sepalani sepalani Apr 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you plan to include the space, what about GetCellSize as a function name? Can't this function be marked as constexpr. I also highly suggest that for each case, you write an example.

For instance:

case MemoryViewWidget::Type::Unsigned32: // UINT_MAX = 4294967295
  return 11;
case MemoryViewWidget::Type::Float32: // FLT_MAX = 3.40282e+38
case MemoryViewWidget::Type::Signed32: // INT_MIN = -2147483648
  return 12;

EDIT: Some constants that might be interesting to look at https://en.cppreference.com/w/cpp/types/climits

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I don't think it will be used for anything else. I can add examples too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually the cell size needs to be calculated from the font, so I think CharacterCount is most accurate.

@TryTwo TryTwo force-pushed the PR_MemoryWidget_Dual_Views branch from 83909c6 to 1c8979b Compare April 20, 2022 13:16
@TryTwo
Copy link
Contributor Author

TryTwo commented Apr 20, 2022

Tried a solution to row breakpoints.

@AdmiralCurtiss
Copy link
Contributor

I can still get the row breakpoints to break with this. But again, I wouldn't worry about it, it can be fixed or removed in a separate PR. Your commits are also now a combined thing of multiple features in one commit, which is far worse than row breakpoints being screwy.

@AdmiralCurtiss
Copy link
Contributor

I would also argue that the Dual View checkbox not doing anything in Hex view is worse than it displaying two Hex views, from a discoveryability perspective.

@TryTwo TryTwo force-pushed the PR_MemoryWidget_Dual_Views branch from 1c8979b to a7111e3 Compare April 23, 2022 09:58
@TryTwo
Copy link
Contributor Author

TryTwo commented Apr 23, 2022

My goal isn't to change the features of row breakpoints, but to keep them from breaking. I think some of the breaking happened in the previous PR though, so I can move the changes to a new PR if you want. Either way I think I got them fixed.

Copy link
Contributor

@AdmiralCurtiss AdmiralCurtiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I'm okay with this now.

@AdmiralCurtiss AdmiralCurtiss merged commit 4c080b8 into dolphin-emu:master Apr 23, 2022
@TryTwo TryTwo deleted the PR_MemoryWidget_Dual_Views branch February 6, 2024 04:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants