Debugger MemoryWidget: More display types, use combo box for options

[TryTwo]

Added Int8/16/32 and double..

• Do we want signed displays?
• 1.234e+09 syntax looks like it could be better, but what would I replace it with? Remove the +?
• Int8 looks terrible, but will look fine if we add a dual view - two different column types with 4 bytes per row
• I left display type as a group, as it will likely get more items soon.
• Do we need to rename U8/16/32?
• Thoughts on sidebar rearrangement?

Most importantly, what else do I need to add? Do we need more customization in this PR like bytes per row? A dual-view would require a lot of rearranging in the Update function to pass two view types through it. I think I would spin off column value updates and breakpoint tagging updates to two different functions, but will take suggestions on a better method to do it.

[TryTwo]

Added row length combo for 4 bytes, 8 bytes, or 16 bytes. Helps a lot with integer displays.

[AdmiralCurtiss]

The Search buttons should probably be closer to the top, probably right below the Set Value button, since they work off the same input field. Also get rid of the box they're in.

The display type might also look better with just a label next to it instead of an entire box for a single button.

[AdmiralCurtiss]

Rename U8/16/32 to Hex, probably. Might as well have Signed N and Unsigned N as separate options too.

Integers should be right-aligned within the column.

The Int32 view needs to be wide enough to fit the full integer, please don't use exponential for that, it's completely useless.

[TryTwo]

Forgot to remove the search box. Made int32 full width. Renamed things.

Happy to accommodate any further suggestions.

[AdmiralCurtiss]

...you should rebase this on master before you introduce even more conflicts.

[TryTwo]

Oh I didn't know it'd pick up rebase changes in Compare. That's a bit annoying. I'm still working on some stuff, so nobody needs to bother reviewing the code atm.

[TryTwo]

I don't know how far this PR should go. I can break things up into two PRs, which is why I'm making multiple commits. Should I add this as a third commit to this PR?

[AdmiralCurtiss]

Less changes per PR is better as long as they make sense individually -- easier to review and all. So yes, I'd split that up.

It's okay to have multiple PRs that depend on eachother, too, so you don't have to deal with merge conflicts. That is, one PR that has commit A only, and then another PR that has commits A and B with a note that the first PR should be merged before this one.

[TryTwo]

Ahh ok, that makes sense.

[TryTwo]

Made clicking preserve byte offset (like 80000001), made row breakpoint conform to row settings.

Breakpointing odd bytes (0x1) has weird results, I'm not sure if I need to enforce some sort of even byte check. Also, a 4-byte row breakpoint on a 0x2 offset is weird.

/edit
MemChecks::OverlapsMemcheck code is only fine if everything is aligned normally. Setting breakpoint aligned at 0x2 - 0x5 will cause a check at 0x6 or 0x7 with length = 4 to return true.
mc.end_address | page_end_suffix) == page_end_address)
0x5 | 0x3 == 0x7 true

MemChecks::GetMemCheck is more accurate for unexpected breakpoint ranges. Could I just call GetMemCheck and look for a nullptr?

Alternatively we can forcibly align things to normal offsets when odd addresses are input. u32 to 4 bytes, etc. Do games ever load a u32 from like 0x2 offsets?

[TryTwo]

@AdmiralCurtiss I need your thoughts on how the rows should be aligned if you input 80000001 or 80000002. Keep in mind integer values change if the alignment isn't a multiple of the underlying byte length. The easy thing to do is forcibly round the row addresses to 0 at all times and just highlight the target address so you know where to look, but I'm thinking you need more functionality than that.

4 byte and 8 byte row lengths would of course if you rows aligned like that: Like 80000000, 80000004, 80000008 for byte.

[sepalani]

@AdmiralCurtiss I need your thoughts on how the rows should be aligned if you input 80000001 or 80000002. Keep in mind integer values change if the alignment isn't a multiple of the underlying byte length. The easy thing to do is forcibly round the row addresses to 0 at all times and just highlight the target address so you know where to look, but I'm thinking you need more functionality than that.

4 byte and 8 byte row lengths would of course if you rows aligned like that: Like 80000000, 80000004, 80000008 for byte.

What about adding a setting for that and let the user set the alignment such as: (feel free to come with better names/wording)

• auto: alignment based on type and the number of bytes per row
• none: will go at the address+offset without taking the alignment into account
• fixed: based on the number of bytes per row (the behaviour of hex editor)
• custom: based on the user input

I'm fine with moving the memory dump buttons down below as long as they are in a dedicated group box like the other options.

[TryTwo]

What about adding a setting for that and let the user set the alignment such as: (feel free to come with better names/wording)

That might work. Would it be better to put that in the table's context menu?

[AdmiralCurtiss]

I think by default just aligning starting at the current address is fine, if you jump to ...02 then you'll just get 02, 06, 0a, 0e when 4-byte aligned. If you want to be fancy you can make options, but I dunno if it's really all that necessary -- you can just go back 2 bytes after all.

[TryTwo]

Alright, how is that for alignment options?

Too many things were being called address / addr, so I renamed some things, and then I renamed the associated things as well.

[TryTwo]

The only thing left out was a custom alignment, as I didn't want to make an input box for it. If needed, I could do a "lock alignment" option, and then the address box would be used to set the alignment, then after it is locked, it wouldn't change even if the address does.

[TryTwo]

Should be pretty much done with the PR, unless @sepalani wants more for alignment, or if anyone has other thoughts.

For the UI buttons, search has a label that pops up giving you feedback, so I left the box alone. Then since everything else has a box, dump got one for consistency.

[Dentomologist]

We've recently merged changes to MemoryWidget, so you'll need to rebase this PR off the latest master.

[AdmiralCurtiss]

I feel like the alignment option should be another combobox or something by the Display Type options, rather than hidden inside the right-click menu. The names also aren't very self-explanatory, but to be fair I don't have a good suggestion either and it's easy enough to figure out with some experimentation. Otherwise this seems mostly fine.

[TryTwo]

Made the alignment combobox. Also squashed the two commits to one, it was too hard to keep them split. Not sure if "No Alignment" should have a warning or be disabled for floats, doubles, and maybe ints, as the value changes.

[AdmiralCurtiss]

Alright, lemme test this again but this seems fine to me now.

[AdmiralCurtiss]

You didn't fix the random lowercase unsigned! But yeah seems good otherwise.

[TryTwo]

I got it this time.