-
-
Notifications
You must be signed in to change notification settings - Fork 744
Add automatic detection and merging of separate date/time column (#1220) #1259
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Related to #1220 |
facontidavide
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
first pass of suggested changes. also please remember to apply formatting with pre-commit before submitting a PR
| else if (index >= curve->dataSize()) | ||
| { | ||
| // Target is at or beyond the last point - return the last point | ||
| return curve->sample(curve->dataSize() - 1); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the purpose? also, do not include in the PR changes that are not related
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've included two small fixes as reported in the PR description (Fix tooltip display at end of time series data)
| for (const char* fmt : date_formats) | ||
| { | ||
| std::istringstream in{ base_str }; | ||
| in.imbue(std::locale::classic()); | ||
| date::year_month_day ymd; | ||
| in >> date::parse(fmt, ymd); | ||
| if (!in.fail()) | ||
| { | ||
| info.type = ColumnType::DATE_ONLY; | ||
| info.format = fmt; | ||
| return info; | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you please use a lambda or a small helper function to avoid code repetition?
| } | ||
|
|
||
| // Find DATE_ONLY and TIME_ONLY column pairs | ||
| for (size_t date_idx = 0; date_idx < column_types.size(); date_idx++) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my suggestion is to limit to consecutive columns only, therefore change this to
| for (size_t date_idx = 0; date_idx < column_types.size(); date_idx++) | |
| using PJ::CSV::ColumnType; | |
| for (size_t index = 0; (index + 1) < column_types.size(); index++) | |
| { | |
| const auto date_idx = index; | |
| const auto tie_idx = index + 1; | |
| if((column_types[date_idx].type!= ColumnType::DATE_ONLY) || | |
| (column_types[time_idx].type!= ColumnType::TIME_ONLY)) | |
| { | |
| continue; // early return is easier to read | |
| } | |
| // is_adjacent is implicitly true |
| combined.virtual_name = virtual_name; | ||
| _combined_columns.push_back(combined); | ||
|
|
||
| // Add to the UI list widget ONLY (not to column_names, which is used for CSV parsing) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure that I want to add a column in the UI. At most a not in a QLabel?
| if (lines.size() > 0) | ||
| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I usually prefer early returns
| if (lines.size() > 0) | |
| { | |
| if (lines.empty()) | |
| { | |
| return; | |
| } |
| "%H:%M:%S", | ||
| "%H:%M", | ||
| "%I:%M:%S %p", // 12-hour with AM/PM | ||
| "%I:%M %p" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should accept times that don't have seconds
| min_time = std::min(min_time, t0); | ||
| max_time = std::max(max_time, t1); | ||
| max_steps = std::max(max_steps, (int)data.size()); | ||
| max_steps = std::max(max_steps, (int)data.size() - 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here the other fix (Timeline slider fix)


Summary
Added automatic detection of CSV files with date and time stored in separate columns, with automatic combination into a single timestamp for plotting.
Motivation
Some data logging equipment stores date and time information in separated CSV columns:
PlotJuggler previously required timestamps in a single column.
Solution
Implemented automatic detection and merging:
Timeline slider fix
The timeline slider was diving the time range by the number of data points instead of the number of intervals between them, causing navigation steps to display incorrect time values. This fix ensures the slider accurately represents the actual time spacing in the data.
Fix tooltip display at end of time series data
When the cursor is positioned at or beyond the last data point in a time series, the tooltip now correctly shows the final data point value instead of disappearing. This ensures consistent tooltip behavior across the entire data range, including at the boundaries.