Nodelist: choice of long or short name#7926
Conversation
|
|
|
|
|
This is code for #7926 |
|
Thanks @l0g-lab . Might want to update the PR text at the top with a description (can probably copy bits from the Github issue). Just means people can review without going to another link. |
There was a problem hiding this comment.
Pull Request Overview
This PR implements configurable node name display length by adding a menu option to toggle between long and short node names in the node list display.
Key changes include:
- Added a new menu option to switch between long and short node names
- Modified the node name retrieval logic to respect the configuration setting
- Added menu infrastructure to support the new functionality
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/graphics/draw/NodeListRenderer.cpp | Updates getSafeNodeName() function to use long or short names based on config setting |
| src/graphics/draw/MenuHandler.h | Adds node_name_length_menu enum and nodeNameLengthMenu() declaration |
| src/graphics/draw/MenuHandler.cpp | Implements the node name length menu functionality and integrates it into screen options |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| int options = 1; | ||
|
|
||
| #if defined(T_LORA_PAGER) | ||
| optionsArray[options] = "Node Name Length"; |
There was a problem hiding this comment.
I think we should call this something more aligned to the product "Show Long/Short Name"
Xaositek
left a comment
Xaositek
left a comment
There was a problem hiding this comment.
Looks good with the changes.
|
@l0g-lab You will need to run trunk to get this through CI. |
|
Protobuf changes have been added to their master branch. My latest commit removes what should be auto-generated. |
|
For this to get merged, do we wait for additional review? I see there are a large number of pending PRs, so not expecting anything right away but I'm interested in what the rest of this looks like. |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Xaositek
left a comment
Xaositek
left a comment
There was a problem hiding this comment.
Made a couple comments along the way - that extra log line can likely be removed when you've completed development of this feature.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Platform: ESP32
Description
I am looking at implementing the ability to use node long names for certain screen sizes in the NodeListRenderer.cpp file. Many nodes in my area are owned by the same person, causing duplicates on the node list page. Using long names for certain screen sizes removes these "duplicates".
At first, minimal changes were required, but I thought it may be a good idea to add a menu option instead of just making it a default setting.
Based on existing code, it seems like an #if #else #endif based on device or screen type/size would follow the standards of what you guys have so far.
I have done some testing, but would welcome additional extended testing. Also, suggestions on device or screen types to build this functionality for is needed as right now it only supports the LilyGo T-Pager.
This is my first PR but looking forward to contributing.
🤝 Attestations