Improve UX for MemViewer and better code flexibility

[Broxzier]

I originalle started with the idea to add a "Right-click Hex -> Add To Watch" option, but found a lot of other things that could be improved first 😄

Copied from the commit:

1. Use smaller font
2. The Home and End keys jumps to and selects the first and last byte respectively
3. Fix calculation error when setting scrollbar upon pressing End
4. Always set m_carretBetweenHex to false when navigating using the keyboard
5. Fix selection jumping when pressing Left or Right when at the edges of the row
6. Refactor to use defines for number of rows, columns and bytes shown
7. Add support for clicking the ASCII table and typing in it
8. Refactored writeHexCharacterToSelectedMemory to be usable for ASCII too
9. Ignore non-left click events (you could middleclick a value to select it)
10. Play system beep when a non-hex character is edited
11. Update selection coords when scrolling

Some notes to clarify:

1. The smaller font is a personal preference, but since the rest of the UI also doesn't use large text anywhere, I found it unnecessary.
2. Previously they only scrolled to the beginning and end, now they select the bytes too
3. When calculating the range of the scroll area, 1 was subtracted to remove the last row, but it should have subtracted the number of visible rows.
4. no comments
5. Easy to reproduce: Select the very last visible byte in the area, and press Right (or the very first and press left, same efffect). The window nicely scrolled down 1 row, but the selection jumped to the top row (or bottom row when going the opposite direction). Now it selects the correct byte.
6. Can be used to easily change the number of rows for example, perhaps useful to save in a variable later, and should it easier to make the window resizable.
7. The only point that really added new code
8. (in my opinion) a easier to read function too.
9. no comments
10. Only tested on Windows, I assume Qt handles it well for all platforms.
11. Srolling down 3 lines will move the selection up 3 lines, easy as that. Has some calculations for page up/down when near the beginning or end.

Before:

After (with extra rows just because):

[aldelaro5]

I will do a more extensive review later when I have time, but for now, I am just going to give 2 comments.

About 1, I do not like the change. The reason is because a common theme I noticed with different hex viewers and more specifically Cheat Engine (which this program aims to replace when using with Dolphin) is that their hex viewer tends to be really difficult for me to look at for hours in a row simply because it's a lot of condensed data, in a rather small font (which really is hindered by the fact that I have a myopia).

Putting a bigger font was done for that reason, to ease the fact that it's a lot of info to read in a little space. Ideally, the best would be to make the size configurable, but considering how little settings this program may need (the only I can think of are the updates and freeze timers) and how less needed it is than other features, I prefer to not do it now and just make eveyrone happy by putting the font slightly larger.

It does make the number of possible rows to display slightly lower because of the added space, but in practice, what I noticed is you really don't need to consult that much memory in one sight often. I noticed this after using Cheat Engine for years on multiple games so I am not really concerned about the added space.

Which leads me to my second comment, change number 6, despite what I jsut said, I do approve, I agreed the code needed that refactor which means I would have done it eventually. However, what I do not approve is the use of define, I would rather have them be a class wide constant simply because it provides better type checking and I don't feel a preprocessor is a necessity here. Also, in my opinion, writting const makes it litterally obvious you are just going to use this as compile time and only there to have a nice label at read time. Finally, I am already doing this practice in the code anyway, check Source/Common/MemoryCommon.h which contains some constants about the GameCube/Wii RAM which are used when dealing with console addresses.

The other changes doesn't sound something I would have a problem with, but like I said, I still need to do an extensive review on them. Overall, this PR sounds really nice :)

[Broxzier]

Agreed with both comments. I'll make both the font size and the defines and make them member variables. For the font size, what do you think about having a Ctrl+Scroll feature to increase/decrease the font size? Many editors work that way, but maybe it's not that obvious for this memory viewer. Otherwise maybe a A- and A+ button in the corner?

[aldelaro5]

I am fine, as long as the window size adapts itself and there is a limit because if you zoom too much, this might be bad, same for too much out.

I actually never thought of that, the code should even account for this because it's binded all to the EM size so you would just need to recalculate that once and redraw.

[Broxzier]

The window adapts itself nicely already, I only had to move the calculations from initialize.
Right now there's only a minimum font size of 5 (hard coded), but no maximum. I don't think a maximum limit is really necessary, increasing the font to ridiculous numbers is not what this tool is intended for.

[aldelaro5]

Other than these changes, make the #define class wide const and everything is LGTM.

[aldelaro5]

Put the return inside braces here. I am okay to omit the braces, but only if the entire seqeunce of if, else if and else don't have them. This is not the case here and even if return is trivial, it's still a bit jarring to read.

[Broxzier]

With the new changes this can be removed entirely. Clicking the empty space is checked before this block now.

[aldelaro5]

Removing this actually doesn't work, you can select the first of the row if you click on the last hex character of the row header and it really does not ignore clicks on the header (clickedPosY is calculated using integer division while the left member, if you click on the column header, will always be equal or less then the right member which means, it's always 0 here). After testing a bit, this is the correct way to do this:

if (!(x >= m_rowHeaderWidth &&
        x <= m_rowHeaderWidth + m_hexAreaWidth + spacing + (m_charWidthEm * VISIBLE_COLS) &&
        y >= m_columnHeaderHeight && y <= m_columnHeaderHeight + m_charHeight * VISIBLE_ROWS))
    return;

It assumes spacing is declared a line above and obviously, replace the define constant by whatever name you give to the class wide const. Note also the addition of && y <= m_columnHeaderHeight + m_charHeight * VISIBLE_ROWS, this is because while testing this, I found a bug that is affecting master where if you click JUST RIGHT you can select an invisible row at the bottom, I found this PR fitting for the fix so I put it in there. Essentially, this condition ensure that whatever you clicked in is in the hex area or the ascii area, the entire rectangle.

You are free to put a comment there if it's confusing, but if you do go with this, remove the test you do on clickedPosY.

[Broxzier]

Well spotted. Clicking the row, column headers or area between the two tables was due to the devision. I didn't take into account that any value between 0 and -width would return 0. I think a nicer way than having those concatinated conditions is to check if the relative click position is < 0 rather than checking the index.

[aldelaro5]

The scroll should be BEFORE assigning m_byteSelectedPosY because if you set it THEN scroll, it's going to be on the last column of the same row instead of being the last column of the previous row. Since you do it first for the right arrow key, do so here to remain consistent.

[Broxzier]

Ah yeah, the change that makes the selection move when scrolling changed this behaviour. Actually this entire part should be refactored, it's not taking the variable number of rows and columns into account.

[aldelaro5]

Here, you have to do m_byteSelectedPosY = 0; as you no longer stay the selection to the same place everytime. Do so after the scroll, this will make the selection jump one row above,

[aldelaro5]

same here, but you assign 15 this time, fixes the same thing, but down this time.

[aldelaro5]

The last commit says the defines were replaced, but I still see them here.

[Broxzier]

Forgot to push the latest changes, should be good now.

[aldelaro5]

I don't like this way of doing this for 2 reasons: 1: it doesn't consider the horizontal space after the ascii area and second, it's honestly more confusing to read to make sure this work because essentially, what you want to do is test that the click was inside the grid and having all these conditions separated doesn't make that part obvious. I would rather keep the 4 conditions I mentioned because they all set each boundaries of the grid.

[Broxzier]

The area to the right doesn't really need to be checked, since it touches the scroll bar, even when zoomed in very far, but (Actually scrap that, that's not the case anymore.) I do agree it would be nicer to do.
I don't agree on the readibility. I'd rather see conditions simply comparing two variables than having a bunch of additions within the condition. That's something I'm working on now. The code that you suggested earlier didn't match the areas exactly (pink box in the screenshot).

I'll combine the two, by storing the left/right and top/bottom in variables, then checking if the clicked locaiton is in either of those areas.

[aldelaro5]

Ok, sounds fine with better variable labels.

[Broxzier]

I've got the correct area calculations for the hex table now:

  int spacing = m_charWidthEm / 2;
  int hexAreaLeft = m_rowHeaderWidth - spacing / 2;
  int hexAreaRight = m_rowHeaderWidth + m_hexAreaWidth - spacing / 2;
  int asciiAreaLeft = (m_hexAsciiSeparatorPosX + spacing);
  int asciiAreaRight = asciiAreaLeft + m_charWidthEm * m_numColumns;
  int AreaTop = m_columnHeaderHeight + m_charHeight - fontMetrics().overlinePos();
  int AreaBottom = AreaTop + m_charHeight * m_numRows;
  int cellWidth = m_charWidthEm * 2 + spacing;
  int cellHeight = m_charHeight;

The click area includes half of the spacing between each byte. Now to get the values for the ascii table and simplyfy it a bit, then I'll get this PR nice and clean! :P