forntoh / LcdMenu

Display navigable menu items on your LCD display 📟 with Arduino
https://lcdmenu.forntoh.dev
MIT License
170 stars 38 forks source link

Move logic from LcdMenu to items #193

Closed ShishkinDmitriy closed 2 weeks ago

ShishkinDmitriy commented 3 weeks ago

Initial move, main changes are here. I tried to remove unneccessaty changes, had to add setBacklight method because of example.

ShishkinDmitriy commented 3 weeks ago

Had to comment test:

// unittest(no_text_off_on_for_item_input) {
//     assertNull((static_cast<ItemInput*>(mainMenu[ITEM_INPUT_INDEX]))->getTextOn());
//     assertNull((static_cast<ItemInput*>(mainMenu[ITEM_INPUT_INDEX]))->getTextOff());
// }

Because base class MenuItem has no that method, only specific ItemInput class has.

forntoh commented 3 weeks ago

⚠️ Found an issue while testing InputRotary Example, I believe CharsetInput is also affected.

With this PR

https://github.com/user-attachments/assets/eb2f6760-fc6a-4d2e-9f4f-44369249af7e

With master,

https://github.com/user-attachments/assets/0ad7573e-669b-4a72-af88-e024a2fdb4ab

Please investigate this. [Tip: Check your drawChar function it shouldn't update the blinker position]

ShishkinDmitriy commented 3 weeks ago

What a great review) Thanks. I will fix it. Regarding charset input.. I think it should be one more implementation of MenuItem let's say ItemCharsetInput. If ItemIniput listen for left, right, type, backspace. The ItemCharsetInput should listen left, right, up, down, backspace. Then method drawChar will be eliminated - it was hard for me to understand the difference between type and drawChar initially.

ShishkinDmitriy commented 3 weeks ago

Ok, I have added ItemInputCharset, so it required some changes in SimpleNavConfig

ShishkinDmitriy commented 2 weeks ago

In last commit I have added ItemInputCharsetSimple (name can be better.. tried to emphasize that only left/right dimensions are available) - implementation of charset input where need to enter to start select char and instead of up and down actions right and left are used, so it's implementation for Rotary encoder.

forntoh commented 2 weeks ago

Hey @ShishkinDmitriy, while reviewing your PR, I'd suggest the following code changes:

👉 Code Suggestion for #193

https://github.com/forntoh/LcdMenu/pull/193

You can also review and apply these suggestions locally on your machine.

Learn more about GitKraken Code Suggest >[Code Suggest](https://gitkraken.dev/redirect/aHR0cHM6Ly93d3cuZ2l0a3Jha2VuLmNvbS9zb2x1dGlvbnMvY29kZS1zdWdnZXN0P3NvdXJjZT1naXRodWIuY29tJm1lZGl1bT1wcl9jb21tZW50?source=PRSuggest&event=redirectToCodeSuggestPage&provider=github) liberates your code reviews from GitHub's restrictive, comment-only feedback style. As simple as suggesting changes in a Google-doc, provide real code suggestions from where you code, e.g. your IDE, and on anything in your project — not just on the lines of code changed in the PR. _Join your team on [GitKraken](https://gitkraken.dev/redirect/aHR0cHM6Ly93d3cuZ2l0a3Jha2VuLmNvbT9zb3VyY2U9Z2l0aHViLmNvbSZtZWRpdW09cHJfY29tbWVudA==?source=PRSuggest&event=redirectToIndexPage&provider=github) to speed up PR review._