Conversation
equivalence rules for all classes neccecary made comments in hpp files
sample still needs finished
and user input is within classes now
callback execution broken
and need to tidy up sections
| * @param cb a void pointer to this items callback method | ||
| * @param ctx a void pointer to any context information for this menu(if none provided, is NULL) | ||
| */ | ||
| MenuItem(void* parent, void* term, char* option, char* text, callback_t cb, void* ctx); |
There was a problem hiding this comment.
Is there a reason these are void* instead of MenuItem pointers? Isn't it always the case that the parent to a MenuItem will be another MenuItem?
| * @param cb a void pointer to this items callback method | ||
| * @param ctx a void pointer to any context information for this menu(if none provided, is NULL) | ||
| */ | ||
| MenuItem(void* parent, void* term, char* option, char* text, callback_t cb, void* ctx); |
There was a problem hiding this comment.
If many of these are going to be passed a null value commonly, they could be given default values. Default values only work for objects at the end of the initialization list, so perhaps they should be re-ordered so that things that are almost never null are at the front of the list.
| void* term; | ||
| }; | ||
|
|
||
| class SubMenu : public MenuItem { |
There was a problem hiding this comment.
I don't particularly like that these are in the same hpp but have different cpps. Is there any reason for them to be in the same hpp? if there isn't, please move them to separate hpps.
| * takes a uart object and sets up the command line terminal | ||
| * @param uart an instance of a UART object | ||
| * @param baud 9600 or 115200 | ||
| */ |
There was a problem hiding this comment.
incorrect doc comment
| * @param term void pointer to the terminal this is being done inside of(optional to leave as nullptr, just here to | ||
| * be used with callbacks) | ||
| */ | ||
| void enterSub(io::UART& uart, char** args, void* term); |
There was a problem hiding this comment.
Is there a reason to pass in another uart instance? won't it always be the same one?
| callback_t cb2 = it2->getcb(); | ||
| void* ctx2 = it2->getctx(); | ||
| // if any corresponding arent equal, return false | ||
| if (option != option2 || text != text2 || cb != cb2 || ctx != ctx2) { |
There was a problem hiding this comment.
bear in mind that this will literally compare memory addresses, not contents. If that is intended, that's alright, but otherwise this will not work the way you think it does. Additionally, do we necessarily need an equals() method? Is there any point at which we are using it?
| * @param ctx enterence behavior callback void*(leave null for nothing) | ||
| * @param items list of items in submenu | ||
| */ | ||
| SubMenu(void* parent, void* term, char* option, char* text, callback_t cb, void* ctx, MenuItem** items); |
There was a problem hiding this comment.
I'm struggling to understand what make a subMenu different from a MenuItem. If they aren't significantly different we should consider if we need to care about the distinction between them.
There was a problem hiding this comment.
additionally, if a submenu is also a menu, maybe it should inherit from menu as well?
| /** | ||
| * pointer to callback method for this item | ||
| */ | ||
| callback_t cb; |
There was a problem hiding this comment.
rename this to callback
| /** | ||
| * context for this item, void* because it is of an abstract type | ||
| */ | ||
| void* ctx; |
There was a problem hiding this comment.
rename this to context
| // a list of input strings(your input from the terminal), and a void* | ||
| // mostly a placeholder to ease handling void* to a function, when you fill with a function make sure the parameters are | ||
| // (UART,char**,void*) | ||
| using callback_t = void (*)(core::io::UART&, char** inputList, void*); |
There was a problem hiding this comment.
Big fan of this, we should possibly consider requiring this instead of typedefs in our code, because this is far more readable. However, it should probably be in the namespace.
include/core/utils/terminal/menu.hpp
Outdated
| /** | ||
| * list of all items contained in the menu | ||
| */ | ||
| MenuItem** items; |
There was a problem hiding this comment.
I'm confused about what we're doing here tbh. Since we are capping the size of the list to such a small number, we could just create the array statically, inside this class, instead of having the user pass the array into the object. This will both protect access and also clean up the main code.
We may want to look into using templates to set the size of the menu, but I'm not married to that.
Also see my notes on having this be a linked list instead. I think for our use case it makes far more sense.
2 participants
generic UART terminal to load and execute functions