Skip to content
This repository has been archived by the owner on Jan 8, 2024. It is now read-only.

Remove menu magic numbers #47

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

wutje
Copy link
Contributor

@wutje wutje commented Sep 19, 2023

Add documentation on the code in ui/menu.c
Made all relevant code depend on a single define for the split.

for (i = 0; i < 3; i++) {
if (gMenuCursor || i) {
if ((gMenuListCount - 1) != gMenuCursor || (i != 2)) {
UI_PrintString(MenuList[gMenuCursor + i - 1], 0, 127, i * 2, 8, false);
UI_PrintString(MenuList[gMenuCursor + i - 1], 0, SPLIT - 2, i * 2, 8, false);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is actually a bug fix 😄

Copy link
Owner

Choose a reason for hiding this comment

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

What do you mean with "bug fix" ? The original uses 127. From Ghidra:

            if (((cursor | i) != 0) && ((gMenuListCount - 1 != cursor || (i != 2)))) {
                    GUI_PrintString("SQL" + ((cursor + i) - 1 & 0xff) * 7,0,0x7f,(i << 0x19) >> 0x18,8,0);

Copy link
Owner

Choose a reason for hiding this comment

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

If you mean a bug by QS, then I have the "fixes" branch for that. Fixes is rebased on v2.01.31 which is rebased on main.
I have fixed a few bugs in "fixes" that QS firmware (e.g. the 8.33kHz step for airband was not working correctly).

I'll test the "48" here and commit to fixes after the test.

@OneOfEleven
Copy link

The 127 / " SPLIT - 2" can be anything you like, it's not used, that param is only used when centering the text, which it's not, it's left aligned.

@wutje
Copy link
Contributor Author

wutje commented Sep 19, 2023

Yes I just double checked and saw the same code. Still seems wrong to put in 127.
If you would want your menu item to centered, they are suddenly centered to full screen width.

@DualTachyon
Copy link
Owner

It would probably make more sense to combine End and bCentered to just End, and it only centers if End > 0 or something.

@OneOfEleven
Copy link

OneOfEleven commented Sep 19, 2023

Thats what I do. Means 1 less param to pass.

@wutje
Copy link
Contributor Author

wutje commented Sep 20, 2023

Do you still plan to pickup the rest of the magic number reduction? The bugfix was more of a consistency thing.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants