Delegate Rendering of Separator and Value to Renderer #240
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
## Description This PR refactors the rendering logic by delegating the responsibility of rendering separators and values from individual items to the MenuRenderer. The changes include: - Moving the render logic from each item to the `MenuRenderer`. Allowing the `MenuRenderer` to decide how to render the value, enabling customization of the separator. - This refactor enhances the flexibility and maintainability of the rendering process by centralizing the rendering logic within the `MenuRenderer`. ### Changes - Updated `MenuRenderer` to handle the rendering of separators and values. - Removed rendering logic from individual items.
|
Memory usage change @ d1c16ab
Click for full report table
Click for full report CSV |
Adjust how effective columns are calculated based on presence of arrows and icons. Simplified logic for determining available display space.
|
Memory usage change @ 7b0d2b8
Click for full report table
Click for full report CSV |
- Added calculation of available columns based on arrow presence - Updated text padding and effective column calculations to use available columns instead of recalculating each time.
Think it's a bad idea.. |
|
Memory usage change @ e016a47
Click for full report table
Click for full report CSV |
Centralizing the rendering logic in the
The new menu controls that you're add would still work, all you would need to do is to pass the This will get rendered appropriately like |
|
Memory usage change @ 9524270
Click for full report table
Click for full report CSV |
|
What if I have small screen and I want to render Just don't separate concerns, single item knows about it's concepts, knows how to change them and how to render them on piece of display. |
I am planning to open a new PR which will handle horizontal scrolling of ALL long items so, it's still a WIP, so the issue of no small display will be effectively gone (Hopefully 😃) And I I guess your example here is already the case for |
|
Check this out https://github.com/ShishkinDmitriy/LcdMenu/blob/feature/add-multidimentional-items/src/BaseItemManyWidgets.h#L50 |
|
Here's a video of the WIP Screen.Recording.2024-10-08.at.11.46.36.movBasically, what happens here is that the MenuScreen shifts the view when I can also make it possible for items to shift the view themselves if they desire to |
|
Looks great! How it works with Rotary? It has no LEFT and Right commands. |
I you have only a RotaryEncoder, you're out of luck 😃, you can't shift it by command, I am thinking of adding like a timed shift (but not in the same WIP), that someone can enable to automatically h-scroll items when they are in focus like an animation. If items have the possibility to shift the view programatically (for display purposes) then I believe all the cases I see in your WIP can be handled Possibility: when using only a rotary encoder and I type a char, I (the item) can decide to shift the whole view instead of just the view the value occupies. |
|
I did this way: Recording.2024-10-08.121622.mp4Right now keep text maybe not a good idea, maybe show value would be better. Item has 3 slots, every time you ENTER control move to next, and draw method tries to draw it up to blinker. After ENTER on last widget, it release. So item controls what to show, also we can combine and allow LEFT and RIGHT, but if no such commands, blinker should control view. |
Looks really nice! Can't wait for that PR to be ready, will be a huge improvement. I see it integrating well with my changes as the renderer provides the possibility to move the cursor and start/stop blinker, this would be implemented like you did in input item, you'll just need to play with the cursor position, calculate the available view space and then adjust the value to display before sending to the renderer, just like how iteminput currently does to shift texts when typing. I see you wrote a lot of code (well commented 💪), and you unfortunately need to refactor a lot 😬 sorry! |
|
Memory usage change @ 0a9ddbe
Click for full report table
Click for full report CSV |
|
Memory usage change @ e7d850d
Click for full report table
Click for full report CSV |
forntoh
force-pushed
the
enhancement/rendering-separator
branch
from
2c107d1 to
a805fc2
Compare
forntoh
force-pushed
the
enhancement/rendering-separator
branch
from
a805fc2 to
125ce0b
Compare
|
Memory usage change @ 125ce0b
Click for full report table
Click for full report CSV |
ShishkinDmitriy
approved these changes
Oct 8, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR refactors the rendering logic by delegating the responsibility of rendering separators and values from individual items to the MenuRenderer. The changes include:
MenuRenderer. Allowing theMenuRendererto decide how to render the value, enabling customization of the separator.MenuRenderer.Changes
MenuRendererto handle the rendering of separators and values.viewworks inItemInput, now when backspaces is pressed while the view is scrolled, it moves the characters to the right immediately until view is zero, this fixes the situation in previous releases where when I start deleting the characters and I reach the colon, I can't see the next character I am deleting.Screenshots/Video (if applicable)
Screen.Recording.2024-10-08.at.15.44.09.mov
Checklist
General Requirements
breaking-changeif it introduces a breaking change.Refactor/Enhancement
enhancement.