Reorganize Settings screen and register up/down direction icons#348
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the user experience of the Settings screen by restructuring its visual elements and introducing clearer navigation cues. The changes aim to make the interface more intuitive and visually consistent without altering any underlying functionality or input handling. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request reorganizes the Settings screen for better layout and adds up/down directional icons. The changes in images.lua to register the new icons are correct. In ui.lua, the layout logic is updated as described. I've identified a potential crash if no POPStarter profiles are configured and suggested a fix. I also have a couple of suggestions to improve the maintainability of the layout code by replacing magic numbers with named constants and addressing a potential visual imbalance due to asymmetric icon positioning.
| Font.ftPrint(BFONT, UI.SCR.X_MID, profile_title_y + 78, 8, UI.SCR.X, 16, PLDR.PROFILES[UI.ProfileQuery.curopt].DESC, UI.CCOL.GREY) | ||
| Font.ftPrint(BFONT, UI.SCR.X_MID, profile_title_y + 128, 8, UI.SCR.X, 16, PLDR.PROFILES[UI.ProfileQuery.curopt].ELF, Color.new(128,128,128, 110)) |
There was a problem hiding this comment.
If PLDR.PROFILES is empty, profcnt will be 0. UI.ProfileQuery.curopt could still be 1, which would cause a crash on these lines when trying to access fields on a nil value. The entire block for drawing the profile selection (lines 1255-1267) should be wrapped in an if profcnt > 0 then ... end block to prevent this.
| local profile_title_y = layout.TITLE_Y + 92 | ||
| local up_icon = IMG.up | ||
| local down_icon = IMG.down | ||
| Font.ftPrint(BFONT, UI.SCR.X_MID, profile_title_y, 8, UI.SCR.X, 16, "Choose POPStarter Profile", UI.CCOL.GREY) | ||
| if up_icon ~= nil then | ||
| Graphics.drawImage(up_icon, UI.SCR.X_MID - 150, profile_title_y - 2, UI.CCOL.GREY) | ||
| end | ||
| if down_icon ~= nil then | ||
| Graphics.drawImage(down_icon, UI.SCR.X_MID + 128, profile_title_y - 2, UI.CCOL.GREY) | ||
| end | ||
| Font.ftPrint(BFONT, UI.SCR.X_MID, profile_title_y + 30, 8, UI.SCR.X, 16, "Profile "..UI.ProfileQuery.curopt, UI.CCOL.GREY) | ||
| Font.ftPrint(BFONT, UI.SCR.X_MID, profile_title_y + 78, 8, UI.SCR.X, 16, PLDR.PROFILES[UI.ProfileQuery.curopt].DESC, UI.CCOL.GREY) | ||
| Font.ftPrint(BFONT, UI.SCR.X_MID, profile_title_y + 128, 8, UI.SCR.X, 16, PLDR.PROFILES[UI.ProfileQuery.curopt].ELF, Color.new(128,128,128, 110)) |
There was a problem hiding this comment.
This function uses several magic numbers for vertical positioning of UI elements (e.g., 92, 30, 78, 128). This also applies to the 30 used for mode_y on line 1243. To improve readability and maintainability, consider defining these as named constants within the UI.LAYOUT table. This would make the layout logic clearer and easier to adjust in the future.
| Graphics.drawImage(up_icon, UI.SCR.X_MID - 150, profile_title_y - 2, UI.CCOL.GREY) | ||
| end | ||
| if down_icon ~= nil then | ||
| Graphics.drawImage(down_icon, UI.SCR.X_MID + 128, profile_title_y - 2, UI.CCOL.GREY) |
There was a problem hiding this comment.
The horizontal offsets for the up and down icons (-150 and +128 from screen center) are asymmetric. Since the text 'Choose POPStarter Profile' is constant and centered, one would expect symmetric offsets for the icons on either side for visual balance. If this asymmetry is not intentional (e.g., to account for different icon widths), consider using symmetric values like -150 and +150.
Motivation
Description
upanddownassets inbin/POPSLDR/images.luasoIMG.upandIMG.downresolve at runtime.UI.ProfileQuery.Playinbin/POPSLDR/ui.luato change the top title to"Settings"and move theBDMA Moderow directly under the title while keeping the existing left/right icon rendering around the BDMA value."Choose POPStarter Profile"(withIMG.upandIMG.downshown on the left/right), then renderingProfile X, the profile description, and thePOPSTARTER.ELFpath together below it.Testing
bin/POPSLDR/images.luacontains{"up","up.png"}and{"down","down.png"}andbin/POPSLDR/ui.luacontains the updatedUI.ProfileQuery.Playlayout, with automated text search confirming the expected lines were inserted.2 files changed, 18 insertions(+), 5 deletions(-).IMG.up/IMG.downreferences exist and the BDMA/profile drawing logic was repositioned as required, and those checks passed.Codex Task