Elastic #113
Elastic #113
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds the Elastic library to enable integration with the Elastic dashboard as a replacement for the deprecated Shuffleboard. The code is taken directly from the official Elastic documentation and provides notification and tab selection functionality.
Key Changes
- Added complete Elastic.java utility class with notification support
- Implemented NetworkTables integration for dashboard communication
- Provided fluent builder API for notification configuration
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| /** | ||
| * Represents an notification object to be sent to the Elastic dashboard. This object holds |
There was a problem hiding this comment.
The phrase "Represents an notification object" contains a grammatical error. The article "an" should be "a" since "notification" starts with a consonant sound.
| public void setDescription(String description) { | ||
| this.description = description; | ||
| } | ||
|
|
There was a problem hiding this comment.
The method getDescription() is missing a Javadoc comment, while all other getter methods in the class have documentation. This creates inconsistency in the API documentation.
| /** | |
| * Gets the description of this notification | |
| * | |
| * @return the description of this notification | |
| */ |
| * @return the current notification | ||
| */ | ||
| public Notification withLevel(NotificationLevel level) { | ||
| this.level = level; | ||
| return this; | ||
| } | ||
|
|
||
| /** | ||
| * Modifies the notification's title and returns itself to allow for method chaining | ||
| * | ||
| * @param title the title to set the notification to | ||
| * @return the current notification | ||
| */ | ||
| public Notification withTitle(String title) { | ||
| setTitle(title); | ||
| return this; | ||
| } | ||
|
|
||
| /** | ||
| * Modifies the notification's description and returns itself to allow for method chaining | ||
| * | ||
| * @param description the description to set the notification to | ||
| * @return the current notification | ||
| */ | ||
| public Notification withDescription(String description) { | ||
| setDescription(description); | ||
| return this; | ||
| } | ||
|
|
||
| /** | ||
| * Modifies the notification's display time and returns itself to allow for method chaining | ||
| * | ||
| * @param seconds the number of seconds to display the notification for | ||
| * @return the current notification | ||
| */ | ||
| public Notification withDisplaySeconds(double seconds) { | ||
| return withDisplayMilliseconds((int) Math.round(seconds * 1000)); | ||
| } | ||
|
|
||
| /** | ||
| * Modifies the notification's display time and returns itself to allow for method chaining | ||
| * | ||
| * @param displayTimeMillis the number of milliseconds to display the notification for | ||
| * @return the current notification | ||
| */ | ||
| public Notification withDisplayMilliseconds(int displayTimeMillis) { | ||
| setDisplayTimeMillis(displayTimeMillis); | ||
| return this; | ||
| } | ||
|
|
||
| /** | ||
| * Modifies the notification's width and returns itself to allow for method chaining | ||
| * | ||
| * @param width the width to set the notification to | ||
| * @return the current notification | ||
| */ | ||
| public Notification withWidth(double width) { | ||
| setWidth(width); | ||
| return this; | ||
| } | ||
|
|
||
| /** | ||
| * Modifies the notification's height and returns itself to allow for method chaining | ||
| * | ||
| * @param height the height to set the notification to | ||
| * @return the current notification | ||
| */ | ||
| public Notification withHeight(double height) { | ||
| setHeight(height); | ||
| return this; | ||
| } | ||
|
|
||
| /** | ||
| * Modifies the notification's height and returns itself to allow for method chaining | ||
| * | ||
| * <p>This will set the height to -1 to have it automatically determined by the dashboard | ||
| * | ||
| * @return the current notification | ||
| */ | ||
| public Notification withAutomaticHeight() { | ||
| setHeight(-1); | ||
| return this; | ||
| } | ||
|
|
||
| /** | ||
| * Modifies the notification to disable the auto dismiss behavior | ||
| * | ||
| * <p>This sets the display time to 0 milliseconds | ||
| * | ||
| * <p>The auto dismiss behavior can be re-enabled by setting the display time to a number | ||
| * greater than 0 | ||
| * | ||
| * @return the current notification | ||
| */ | ||
| public Notification withNoAutoDismiss() { | ||
| setDisplayTimeMillis(0); | ||
| return this; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The Elastic class lacks test coverage. Similar utility classes in this project (e.g., Limelight, LinearInterpolation) have comprehensive unit tests. Consider adding tests to verify notification serialization, NetworkTable publishing behavior, and the various Notification builder methods work correctly.
| } | ||
|
|
||
| /** | ||
| * Sends an notification to the Elastic dashboard. The notification is serialized as a JSON string |
There was a problem hiding this comment.
The phrase "Sends an notification" contains a grammatical error. The article "an" should be "a" since "notification" starts with a consonant sound.
| * Sends an notification to the Elastic dashboard. The notification is serialized as a JSON string | |
| * Sends a notification to the Elastic dashboard. The notification is serialized as a JSON string |
|
tf, I didn't know copilot can review pull requests, got really confused at first how someone responded to my pull request in like less than 5 min 💀 |
CoolSpy3
left a comment
CoolSpy3
left a comment
There was a problem hiding this comment.
I'd recommend moving this to a directory that indicates that this is not our code. (e.x. org.carlmontrobotics.elasticlib, org.carlmontrobotics.vendorlibs, com.github.Gold872.elasticlib, etc.) As a side note, iirc, Limelight provides a similar helper class that is currently just included in the necessary robot projects. We should think about unifying our handling of them. (I agree it would be nice to have them here so we could use them both in lib199 code.)
For targeting, you can move this to target master and then merge into 2025 once this merges, so that this file won't be listed in the 2025 changes. (Assuming this file doesn't rely on any 2025-only features.) For anything using this, I'd PR against master if the changes can be made independently of the 2025 changes. Otherwise, PR against 2025 to avoid the need to merge/rewrite stuff later. That being said, I'd hold-off on actually merging anything non-critical into the 2025 code until 2025 merges (again, to avoid re-approving changes).
|
Actually, ya know what, I've thought about it some more, and you can just ignore my previous comment about re-basing onto So, just move the file to a new package, and I'll approve. (For clarification, the |
Alr, I just moved ElasticLib to vendorLibs. Just to make sure if I make a limelight PR, should it also target 2025? |
CoolSpy3
left a comment
CoolSpy3
left a comment
There was a problem hiding this comment.
Great! Yeah, for Limelight, just target 2025 as well. I think it makes more sense to keep all the 2025 code together than to try and do nicer merging strategies.
@CoolSpy3 could you review this pls. This is just elasticlib, I took it directly from the docs: https://frc-elastic.gitbook.io/docs/additional-features-and-references/robot-notifications-with-elasticlib. I added this because we will need to switch to elastic as shuffleboard is getting deprecated. I am not sure if I am supposed to merge this into master or 2025, I put 2025 as the base branch because it is the branch that is actively getting worked on and has the code that actually works.