Debugger MemoryViewWidget: Allow direct editing of memory cells.

[TryTwo]

It seems to work fine, but not really happy with how I coded it. It seems like MemoryView and Memory widget should share the same update functions, but I wasn't sure how to responsibly put that together.

Was anyone else working on this? Thought I'd PR it to open a discussion.

[AdmiralCurtiss]

Oh, nice. But yes I agree, the text field and the explicit cell editing probably want to share the parsing code, just so that you don't have weird edge cases where typing a value works in the text box but not in the cell or something.

[TryTwo]

@AdmiralCurtiss Any thoughts on where the parsing code should go and what should call it? MemoryViewWidget doesn't currently send any data to MemoryWidget. Should it be in MemoryView? Should it return a QString or a QByteArray? MemoryWidget uses it as a QString right now, but MemoryViewWidget would only need the QByteArray.

[AdmiralCurtiss]

I'd put it in MemoryViewWidget just for convenience, and have it return a byte array.

[TryTwo]

@AdmiralCurtiss I don't know how to form a correct length QByteArray without first making a padded QString ( u16 1 -> 0001). Is it okay to use the MemoryWidget logic then just tack on a QByteArray conversion at the end?

[AdmiralCurtiss]

Do you need a QByteArray? You can just do something like u16 value; auto std_array = Common::BitCastToArray<u8>(value); to get an std::array, which should be functionally equivalent, though take care of endianness. (There's some endian helper stuff in Common/Swap.h)

[TryTwo]

@AdmiralCurtiss Ok that works, but I'm not experienced with std::array. How do I properly return the array from the function when the size is variable?

[AdmiralCurtiss]

std::array is a fixed size, so you can't. You'd have to convert it to an std::vector or similar.

return std::vector<u8>(array.begin(), array.end())

[TryTwo]

Thanks, it worked great!

Not sure if I'm sharing the enum correctly.
Vector to QString conversion in MemoryWidget works correctly, but could have a better way to do it.
Not sure how much I wanted to do in the Table class vs the View class for cell editing.
There's a stack overflow if I don't use the m_updating check, because of the accessors. Not sure if that's the correct fix.

[AdmiralCurtiss]

Instead of having this flag, it might be better to use the either QSignalBlocker or the signal blocking helper from DolphinQt/QtUtils/SignalBlocking.h?

[TryTwo]

This seems to work fine. I was getting a stack overflow when having no updating check or signal block. Is one QSignalBlocker placed in this spot good?

[AdmiralCurtiss]

You need to re-verify the address for each byte, you could write past the end of memory.

[TryTwo]

Interesting. I accidentally carried the check over from loading memory values. In the current memory widget coding, there's no check at all. I'm not sure how it handles input that starts in range then goes out of range.

I don't think I need the check at all. You can only edit a cell if it is a valid address, and you can only go past the cell size if you input ascii. but I'm not sure if that should be allowed?

[TryTwo]

Ok so I stepped through it in the VS debugger. It does kind of seem to freak out. I added an end address check, plus added address checks to MemoryWidget. I don't think we want to write anything if the end address is invalid.

[AdmiralCurtiss]

What is this comment trying to say here?

[TryTwo]

I was just noting this caused a stack overflow with Update().

[AdmiralCurtiss]

Yeah, but what is it referring to? The OnItemChanged() function? Then it should be on top of that, not in the middle of it.

[TryTwo]

I think I can just remove the note now. The accessor itself was crashing with the accessor in Update() I think? When I deleted the accessor stuff, it was fine, so I was marking that specifically.

[AdmiralCurtiss]

You broke something with the padding in the preview box, eg:

e: here: #10794 (comment)

[TryTwo]

I don't 100% understand the accessor stuff. Does checking for a valid address work for all address spaces? Does setting values even work for other address spaces besides effective?

[AdmiralCurtiss]

It should. Can you find a situation where it doesn't?

[TryTwo]

No, it looks good. I just never use the other address space options, so wasn't sure if I'd break something I don't use. I was able to give it a quick test and it worked.