Stackable log viewer, BlueDriver support, and v2.7.0 updates#58
Stackable log viewer, BlueDriver support, and v2.7.0 updates#58SomethingNew71 merged 7 commits intomainfrom
Conversation
Signed-off-by: Cole Gentry <peapod2007@gmail.com>
Signed-off-by: Cole Gentry <peapod2007@gmail.com>
… tracking on - Bumps version to 2.7.0 - Adds AfrLambdaUnit to the unit preference system allowing users to toggle between AFR and Lambda display for mixture data regardless of what the source ECU outputs (gasoline stoich 14.7 conversion) - Defaults cursor tracking to enabled - Adds i18n keys for AFR/Lambda setting across all 15 languages
- Updates Rust stable from 1.92.0 to 1.94.1 - Updates egui ecosystem: eframe 0.34.1, egui_plot 0.35.0, egui_extras 0.34.1 - Migrates deprecated serde_yaml to serde_yml - Updates strum 0.28, dirs 6.0, ureq 3.3, flate2 1.1, chrono 0.4.44, arboard 3.6, objc2 0.6.4 - Adapts to egui 0.34 API: splits App::update into logic/ui, migrates panels to Panel::top/left/bottom with show_inside, fixes TextEdit frame
- Updates zip crate from 2.2 to 8.5 (read-only API unchanged) - Updates printpdf from 0.7 to 0.9 (complete API rewrite) - Rewrites export.rs to use printpdf 0.9 Op-based API replacing the old layer-based imperative model with Vec<Op> operations - Adds helper functions (push_text, push_filled_rect, push_closed_line, push_open_line) to reduce repetition in PDF generation
- Updates API client to handle new response format (plain array instead
of { data: [...] } wrapper) fixing spec refresh failures on startup
- Adds Diagnostics variant to ChannelCategory enum to support the
MegaSquirt adapter spec
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates UltraLog to version 2.7.0, introducing a stacked plot mode for organizing channels into multiple independent areas and adding support for the BlueDriver OBD-II log format. Key improvements include automatic encoding detection for UTF-16 files, a migration from serde_yaml to serde_yml, and a refactor of the PDF export logic following a dependency update. The UI has been refined for histograms and scatter plots, and new AFR/Lambda unit conversion preferences have been added. Review feedback identifies a critical compilation error caused by an incorrect trait method rename, performance issues from excessive cloning in the render loop, and suggestions to simplify channel management and increase the robustness of plot resizing and parsing logic.
|
|
||
| impl eframe::App for UltraLogApp { | ||
| fn update(&mut self, ctx: &egui::Context, _frame: &mut eframe::Frame) { | ||
| fn logic(&mut self, ctx: &egui::Context, _frame: &mut eframe::Frame) { |
There was a problem hiding this comment.
The eframe::App trait requires an update method to be implemented. Renaming it to logic will cause a compilation error as the required trait method will be missing. If you intended to split logic and UI rendering, you should keep the update method and have it call your helper methods.
| fn logic(&mut self, ctx: &egui::Context, _frame: &mut eframe::Frame) { | |
| fn update(&mut self, ctx: &egui::Context, _frame: &mut eframe::Frame) { |
| let plot_areas = self.tabs[tab_idx].plot_areas.clone(); | ||
| let selected_channels = self.tabs[tab_idx].selected_channels.clone(); |
There was a problem hiding this comment.
Cloning plot_areas and selected_channels every frame inside the render loop is inefficient and will cause performance degradation, especially with large logs or many channels. Consider using references or passing slices to sub-render functions instead of cloning the entire collections.
| // Check if stacked mode is enabled | ||
| if self.tabs[tab_idx].stacked_mode { | ||
| // Use the stacked mode removal method | ||
| self.remove_channel_from_plot(index); | ||
| } else { | ||
| // Single-plot mode: remove from selected_channels and plot_areas | ||
| if index < self.tabs[tab_idx].selected_channels.len() { | ||
| self.tabs[tab_idx].selected_channels.remove(index); | ||
|
|
||
| // Also remove from plot area indices | ||
| if !self.tabs[tab_idx].plot_areas.is_empty() { | ||
| self.tabs[tab_idx].plot_areas[0] | ||
| .channel_indices | ||
| .retain(|&idx| idx != index); | ||
| // Shift down indices | ||
| for idx in &mut self.tabs[tab_idx].plot_areas[0].channel_indices { | ||
| if *idx > index { | ||
| *idx -= 1; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic in the else block for single-plot mode is redundant. The remove_channel_from_plot method is already designed to handle multiple plot areas and will work correctly for single-plot mode (where plot_areas contains only one element). You can simplify this by calling remove_channel_from_plot regardless of the mode.
// Remove the channel from its plot and selected channels
self.remove_channel_from_plot(index);| } | ||
|
|
||
| /// Adjust plot heights after resize drag | ||
| pub fn adjust_plot_heights(&mut self, plot_idx: usize, delta_pixels: f32) { |
There was a problem hiding this comment.
Using a usize index to identify plot areas for resizing is fragile in a dynamic UI where plots can be deleted. If a plot is deleted while a resize drag is active, the index may point to the wrong plot or be out of bounds. It is safer to use the unique plot_area_id to identify the target plot.
| let (name, unit) = if let Some(paren_start) = header.rfind('(') { | ||
| if let Some(paren_end) = header.rfind(')') { | ||
| if paren_end > paren_start { | ||
| let unit = header[paren_start + 1..paren_end].trim().to_string(); | ||
| let name = header[..paren_start].trim().to_string(); | ||
| (name, unit) |
There was a problem hiding this comment.
The header parsing logic using rfind for parentheses is somewhat fragile. While it works for standard BlueDriver formats, it may fail or produce unexpected results if channel names contain nested parentheses or other special characters. Consider using a more robust regex or a dedicated CSV parser if the format complexity increases.
- Simplify remove_channel to always use remove_channel_from_plot which handles both stacked and single-plot modes identically - Remove full selected_channels clone from stacked render loop, only clone individual channels when building per-plot channel lists
1 participant
Summary
Diagnosticschannel categoryTest plan