Comms menu controls #6736
Comms menu controls #6736
Conversation
…rnate communication menu control scheme
wookieejedi
added
enhancement
There was a problem hiding this comment.
Explicit values assigned to control enums are no longer required for new control entries (old entries must still have them) and are recommended to be defined by name only. Reason: maintenance QOL.
...or at least they should be. I'm sure I added that feature in Controls6...
| @@ -108,6 +108,7 @@ mmode_item MsgItems[MAX_MENU_ITEMS]; | |||
| int Num_menu_items = -1; // number of items for a message menu | |||
|
|
|||
| int First_menu_item= -1; // index of first item in the menu | |||
| int Selected_menu_item = First_menu_item; // index of selected item in the menu | |||
There was a problem hiding this comment.
What is the valid range of values for this index? Our codebase uses negative values (especially -1) in many places to signal an error code or uninitialized state. Please add this info to the line's comment doxy, and use the //!< sequence to have doxygen pick up on it.
| //Check if comms menu is up | ||
| if (Player->flags & PLAYER_FLAGS_MSG_MODE) | ||
| { | ||
| //move down |
There was a problem hiding this comment.
Typo. This is the move up method.
| @@ -108,6 +108,7 @@ mmode_item MsgItems[MAX_MENU_ITEMS]; | |||
| int Num_menu_items = -1; // number of items for a message menu | |||
|
|
|||
| int First_menu_item= -1; // index of first item in the menu | |||
There was a problem hiding this comment.
This index seems odd. Probably outside the scope of this review, but why would we need to track the first menu item? Why can we not assume the first menu item index is 0? Should this be a constant expression intead?
There was a problem hiding this comment.
My memory is a bit fuzzy because it's been a few months but IIRC it's valid for submenus to have 0 items.
| //Select the last menu item if we went outside items range, so we can loop around | ||
| else if (Selected_menu_item < 0) | ||
| { | ||
| First_menu_item = ((Num_menu_items - 1) / 10) * 10; |
There was a problem hiding this comment.
Caution: integer arithmetic. Possible abuse of hardcoded magic numbers. What are the assumptions made here? Please document either here or at the declaration of First_menu_item.
There was a problem hiding this comment.
10 probably needs to be MAX_MENU_DISPLAY define is my guess.
| } | ||
| } | ||
|
|
||
| //function that tricks hud_squadmsg_get_key() into thinking player selected a menu item with a num key press |
There was a problem hiding this comment.
Thank you for the candid message and reasoning here.
This feature adds new controls for comms menu, allowing the user to select given commands using simple up/down/select keybinds.
I'm not familiar with C++, so I hope I'm not doing any obvious blunders here.