Debugger: MemoryWidget: add float and integer input types. Add input preview.

[TryTwo]

Data preview has automatic spacing per byte.

Float is only for singles.
Integer can accept both unsigned and signed numbers up to 4 bytes.
Hex is right aligned and based on the view type. Leading zeroes are always filled automatically.

View-based input:

• "-2" integer in u8 view is FE, and in u16 view it's FF FE
• "1" hex in u32 view is 00 00 00 01, and in u16 view it's 00 01
• "123" hex in u8 view is 01 23 and 12345 -> 01 23 45. (u8 x3)
• "12345" hex in u16 view is 00 01 23 45 (u16 x2)

Fixed a bug where an empty address offset would throw an error.

Loses the ability for infinite length hex string input, but still has it for ascii. Hex max length is 8 bytes. I could go back and redo it if people actually slam massive hex strings in there.

Not sure if anyone is against right-aligned. For me, when inputting 4 byte numbers, I'm either filling all 4 bytes or just operating on the right hand side, like inputting "1e" -> 00 00 00 1E. Also, inputting just one digit when needed is more comfortable "1" -> 01. Plus the input preview clearly shows what will be entered.

/edit Given that more and more types may be added, converted to a combo list. Made types more explicit.

[AdmiralCurtiss]

Something is broken here in the hex view if you have leading zeroes and go over 16 characters. 1234567812345678 parses as 8 bytes, 001234567812345678 parses as 9 bytes, but 011234567812345678 parses as error. This also really breaks the GUI if you go too long.

Spaces in the input box also seem to parse as leading zeroes.

[AdmiralCurtiss]

Seems like an improvement other than that though!

[TryTwo]

Something is broken here in the hex view if you have leading zeroes and go over 16 characters. 1234567812345678 parses as 8 bytes, 001234567812345678 parses as 9 bytes, but 011234567812345678 parses as error. This also really breaks the GUI if you go too long.

Spaces in the input box also seem to parse as leading zeroes.

Thanks for finding these edge cases! Spaces are now removed unless using ascii.
Hex input is now capped at 16 digits input, and other types don't care about leading zeroes. It shouldn't break the UI now.

[AdmiralCurtiss]

Squash the fixup and I'm okay with this.

[AdmiralCurtiss]

Apologies for the conflicts, hope that's not too bad to fix.

[TryTwo]

np, I should probably look at this one more time later to make sure I didn't break anything.

[AdmiralCurtiss]

When using Input Type == Hex and Data Type == ASCII, it pads the entered hex value to multiples of 4 bytes. While it's not exactly clear what the padding should be in that case, I think no padding (same as Data Type == U8) makes more sense for that.

(For Data Type == Float, the 4 byte padding makes sense to keep, IMO, since floats are 4 bytes long.)

[TryTwo]

Ohh, you're right. I didn't consider that case.

[sepalani]

I really think the logic is overkill. What about using a dropdown menu with the proper input type (u8, s8, u16, s16, u32, s32, u64, s64, float, double, hex, ascii, ...)?

Moreover, I think the m_type_* members are misused. IIRC, they were only used to specify the representation of the memory view, not the input type. This use would be counterintuitive.

[TryTwo]

6 different options for something that can be done with 1 option and some logic seems like overkill to me. Is it really unreasonable to expect someone to choose u8 view if they want to input a u8? The view doesn't determine type, only padding/length ( I guess you could call that type tho).

[AdmiralCurtiss]

I find this actually pretty intuitive for integers and floats, though ymmv of course. For a hex string, less so, maybe that one should never be padded?

[sepalani]

6 different options for something that can be done with 1 option and some logic seems like overkill to me.

"Can be done with 1 option" and tons of branching is IMHO clearly overkill. On top of that, checking the first character each time to see if its signed or not is redundant and way harder to read.

The same goes for the padding heuristic which is everywhere. Whereas knowing the exact type gives you the appropriate padding without having to compute it.

Is it really unreasonable to expect someone to choose u8 view if they want to input a u8? The view doesn't determine type, only padding/length ( I guess you could call that type tho).

Yes it is, if it was not for the hex string preview, I don't think many people would have understood it. Especially, since there is an ASCII type and a float type in both group. So that's extremely confusing.

[TryTwo]

How would you turn -20 from a QString into an 8 bit hexadecimal and a 16 bit hexadecimal QString? I'm more than happy to improve upon it, with a bit of guidance.

My basic goal is adding integer + float. Allowing the option to input 1 -> 00000001. Add a spaced out preview to easily see what you're inputting and how many bytes it is.

Should input and view be combined into one thing? Is there any time that'd be annoying?
I could also make it so if input size > intended size it defaults to string of bytes instead of continued padding.

[sepalani]

How would you turn -20 from a QString into an 8 bit hexadecimal and a 16 bit hexadecimal QString? I'm more than happy to improve upon it, with a bit of guidance.

A QComboBox can be used with the proper input type (u8, s8, u16, s16, u32, s32, u64, s64, float, double, hex, ascii, ...). Sticking to radio button sounds like a bad design choice to me. With the QComboBox index, the validation/preview can be simplified and will boil down to a switch and an enum class like this and picking the right QString method (toShort, toInt, ...).

For instance:

switch (combo_id)
  {
  case InputComboId::S16:
  {
    short value = input_text.toShort(&good, 10);
    if (good)
      hex_string = QStringLiteral("%1").arg(value, 4, 16, QLatin1Char('0')).toUpper();
    break;
  }
  case InputComboId::S32:
  {
    int value = input_text.toInt(&good, 10);
    if (good)
      hex_string = QStringLiteral("%1").arg(value, 8, 16, QLatin1Char('0')).toUpper();
    break;
  }
  // Etc. (...)
  }

Should input and view be combined into one thing? Is there any time that'd be annoying?
I could also make it so if input size > intended size it defaults to string of bytes instead of continued padding.

I think changing the view behaviour is out of scope of the PR.

[TryTwo]

That code doesn't give the correct output and the fix is the same padding convolution that's trying to be avoided.

[sepalani]

I thought that it would have behaved like libfmt's format or printf but I was wrong.

Here is an alternative using libfmt sprintf:

  case InputComboId::S16:
  {
    short value = input_text.toShort(&good, 10);
    if (good)
      hex_string = QString::fromStdString(fmt::sprintf("%04hX", value));
    break;
  }

Using fmt::format follows python's format and doesn't print the intended number:

// Code testing
short value = -42;
qDebug() << QStringLiteral("%1").arg(value, 4, 16, QLatin1Char('0')).toUpper();
qDebug() << QString::fromStdString(fmt::format("{:04X}", value));
// You'll need to use printf specifiers and length modifiers here
qDebug() << QString::fromStdString(fmt::sprintf("%04hX", value));

[TryTwo]

Thank you! I knew fmt was incorrect, but didn't know about fmt sprintf.

[TryTwo]

I didn't find a simpler method for float than BitCast, but everything else seems improved.

One problem I have with making 12 different input types, is all the red you will see if you go out of range. With just the integer type, at least you see an output and can adjust it rather than just getting hit with an invalid s8/s16/u8/whatever input that's out of range.

[TryTwo]

Reimplemented original behavior for long hex strings. You can use any length hex, but hex must have whole bytes when using a long hex string.

[TryTwo]

I improved the logic thanks to fmt::sprintf being pointed out. The remaining convoluted line is for a string of u16's being padded with zeroes. Do we want to allow a string of u16 or to just let it fall back to u8's?

I could add 3 extra radio buttons for input length, to divorce it from the view type.

[sepalani]

As explained in my previous comment, I really think the input type's radio buttons are a very bad design choice and it should be switched to a QComboBox instead. It will make the code easier to read, avoid all these redundant checks for the sign and the padding, on top of enforcing the type checking.

Using the view's data type doesn't sound fine to me since it's inconsistent with the input type's radio buttons and incoherent like U8, U16, ... (where "U" stands for unsigned) allow to input negative integers whereas ASCII and float data view don't do anything.

[TryTwo]

The only implementations I can think up involving a QComboBox with s8, s16, s32, u8, u16, u32, hex string, float, double, ascii would feel clunky to actually use.

Also, if you input a "-2" s8 while looking at "12345678" in a U32 data view, do you expect to get 000000FE or 123456FE? Connecting the data view to input length avoids the question.

QString doesn't have a direct s8/signed char conversion that I know of, so I think you still need to check it as an int for size/sign.

Wouldn't it be more accurate to rename view data type to byte lengths rather than U's, regardless?

[sepalani]

The only implementations I can think up involving a QComboBox with s8, s16, s32, u8, u16, u32, hex string, float, double, ascii would feel clunky to actually use.

What do you feel clunky about it? Using radio buttons is as much clunky if not clunkier especially if it's tight to other radio buttons of the view format (no hex editor I've used so far have this behaviour). Using a combo box is straightforward and explicit about what it will be doing (i.e. set a value of a specific data type at the specified address+offset).

BTW, I think there is a misconception with your approach between padding/alignment and endianness. Using big-endian is fine as it's what the Wii/GC are using but magically deducing a "padding" for the radio button integer is just confusing. Same for the hex radio button where grouping bytes using padding doesn't make much sense for an array of bytes.

Also, if you input a "-2" s8 while looking at "12345678" in a U32 data view, do you expect to get 000000FE or 123456FE? Connecting the data view to input length avoids the question.

I'm strongly opposed to connect it to the data view. I don't think people expect implicit stuff to happen like padding based on type and alignment. What would make the most sense is when you input "-2" (no matter what the view type is, even ASCII or Float), it writes "-2" at the specified address+offset, that's all. The answer would (in big endian):

• FE345678 for s8,
• 00FE5678 for s16 and
• 000000FE for s32 integers

QString doesn't have a direct s8/signed char conversion that I know of, so I think you still need to check it as an int for size/sign.

I think using toUshort/toShort for unsigned char/signed char and checking the size should be fine:

// For s8
short value = input_text.toShort(&good, 10);
good &= std::numeric_limits<signed char>::min() <= value && value <= std::numeric_limits<signed char>::max()

[AdmiralCurtiss]

To be clear, I'm not strongly for or against any of this; heck I was fine with the initial implementation of this PR. I think the memory window has too many big usability issues to start caring about (imo) small stuff like this.

[TryTwo]

Sorry, I use the old WX style inputs in my personal builds, so I guess it was confusing for me to mention. Integers were an afterthought for this PR, because we can't even view things as integers. So heavily focusing on integers is weird for me.

Your checkbox idea is good, since we already use it and it's consistent with other programs.

[TryTwo]

Ok, the QString base 0 was pretty good. Is this second combobox ok? I probably need to blank it for anything that's not Unsigned?

[AdmiralCurtiss]

I can't say I've ever seen any UI element like that, and I question how often one actually needs octal (I never do), but yeah, I suppose this works. @sepalani thoughts on this?

[TryTwo]

Yeah looking at it again, just using the checkbox would be better It should only be active for unsigned right?

With the setup you can still prepend 0 for octal.

[AdmiralCurtiss]

Technically there's nothing wrong with writing a signed -1 as 0xffffffff or whatever. It doesn't encode differently if it's signed, but I see no reason to not allow it.

[TryTwo]

Hmm toShort(&good, 0) sees 0xffff as outside the limits of a signed short. It judges it as an unsigned. Not sure of a simple fix.

[AdmiralCurtiss]

Alright, played around with this a bit. There's a few minor weird quirks (eg. octal with prefixed 0 works on unsigned but not on signed, if you're in hex mode you can't prefix a 0x manually, hex byte string doesn't allow spaces but int/float does) but I'd be fine with just merging this as-is and tweaking the details later, because this is still a massive improvement over the previous state. Depends on how much you still want to work on this here.

[AdmiralCurtiss]

Oh, one thing that you should probably fix though: If you enter a long Hex Byte String (eg. 000000000000000000000000000000000000000000000000000000000000) the UI stretches horizontally. Not sure why.

[TryTwo]

I fixed everything except octal for signed, because it needs to work with hexadecimal too, and doesn't. My solutions to getting signed to work with hex would backslide into messy code. Like changing the signed enum to unsigned when validating or using a different switch set just for hex.

I still need to merge the commits. I will wait awhile longer to see if anyone else has anything more for me.

[sepalani]

I can't say I've ever seen any UI element like that, and I question how often one actually needs octal (I never do), but yeah, I suppose this works. @sepalani thoughts on this?

I'm fine with the combo box, or the checkbox for hex. The checkbox for hex feels less awkward and I haven't needed the use for octal yet.

Hmm toShort(&good, 0) sees 0xffff as outside the limits of a signed short. It judges it as an unsigned. Not sure of a simple fix.

That's the intended behaviour, 0xFFFF (aka 65535) is an invalid signed short (s16) but a valid unsigned short (u16, see toUShort). If you want you can input it as -1/-0x01 (s16) or 0xFFFF (u16).

[sepalani]

Beside these nitpicks I'm fine with the current state of this PR.

[sepalani]

Parse as Hex to be consistent with the Cheat Search feature.

[TryTwo]

I wanted to allow as much space for the input box as possible. I could put it on the same line as the input combobox?

[TryTwo]

Thanks for the great suggestions. Should it be InputID or InputId for the enum?

[AdmiralCurtiss]

InputID as per the coding guidelines for abbreviations.

[TryTwo]

std::isprint(c, std::locale::classic()); Do I need to check this when processing ascii? I just noticed it in memoryviewwidget. Otherwise, ascii auto-trusts any input.

[AdmiralCurtiss]

I don't think that's necessary, if the user enters a non-printable character in the textbox they can probably deal with the aftermath.

[AdmiralCurtiss]

Can you squish the commits now? I think we're finally done here.