-
-
Notifications
You must be signed in to change notification settings - Fork 494
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add generic list selection menu and use it in an editor menu #1895
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heya, thanks for the PR!
I gave it a quick look and I have a few questions on choices regarding code design. I'm not sure what's intended and what's a mistake, so if there's a reason you made certain things this way, let me know!
I think you should also try to add a dropdown menu for the owl, too! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if you finished all the edits you planned to do, but in case you did, I have two extra comments:
More changes and fix signedness compiler warning Fix inconsistent indentation
Files menu_list.cpp and menu_list.hpp are missing license info, check how it looks like in other files. |
Woops I forgot about that |
Fix trailing underscore
Please address the build issues. (Most are due to code quality problems, like a missing |
Pro tip: before pushing, you can enable code quality checks by passing |
src/gui/menu_list.cpp
Outdated
for(uint i = 0; i < items.size(); i++) { | ||
add_entry(i, items[i]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MSVC doesn't recognize uint
, please use size_t
instead
Ah I was wondering how to get the errors to appear, thanks |
That last build error is not part of the code, it's a known issue with the environment. |
Add a generic list selection menu and use it.
Closes #1894