From 7f2043c7205a66d579fef6be03fe1eae732bb6ca Mon Sep 17 00:00:00 2001 From: John Thacker Date: Mon, 1 Apr 2024 11:30:16 -0400 Subject: [PATCH] Qt: IO Graph human readable axis ticker with SI prefixes Add a class for a QCustomPlot SI prefix axis ticker, using format_units, and supporting both linear and log scales. Use this for all the known IO Graph unit types (except don't try to interpret BASE_UNIT_STRING yet.) Use this to replace automatically scaling the data, avoiding a lot of expensive floating point calculations for busy graphs. This should make it easier to support smaller intervals eventually (see #13682). Fix #12827. Fix #14661. --- ui/io_graph_item.c | 3 + ui/logray/CMakeLists.txt | 2 + ui/qt/CMakeLists.txt | 2 + ui/qt/io_graph_dialog.cpp | 134 +++++++++++++-------------- ui/qt/io_graph_dialog.h | 10 +- ui/qt/widgets/qcp_axis_ticker_si.cpp | 74 +++++++++++++++ ui/qt/widgets/qcp_axis_ticker_si.h | 42 +++++++++ wsutil/str_util.c | 9 ++ wsutil/str_util.h | 14 +-- 9 files changed, 206 insertions(+), 84 deletions(-) create mode 100644 ui/qt/widgets/qcp_axis_ticker_si.cpp create mode 100644 ui/qt/widgets/qcp_axis_ticker_si.h diff --git a/ui/io_graph_item.c b/ui/io_graph_item.c index 95ff91fe3e..c4394abc3b 100644 --- a/ui/io_graph_item.c +++ b/ui/io_graph_item.c @@ -146,6 +146,9 @@ double get_io_graph_item(const io_graph_item_t *items_, io_graph_item_unit_t val item = &items_[idx]; // Basic units + // XXX - Should we divide these counted values by the interval + // so that they measure rates (as done with LOAD)? That might be + // more meaningful and consistent. switch (val_units_) { case IOG_ITEM_UNIT_PACKETS: return item->frames; diff --git a/ui/logray/CMakeLists.txt b/ui/logray/CMakeLists.txt index 990a5c87b1..8e00b01952 100644 --- a/ui/logray/CMakeLists.txt +++ b/ui/logray/CMakeLists.txt @@ -44,6 +44,7 @@ set(WIRESHARK_WIDGET_HEADERS ../qt/widgets/path_selection_edit.h ../qt/widgets/pref_module_view.h ../qt/widgets/profile_tree_view.h + ../qt/widgets/qcp_axis_ticker_si.h ../qt/widgets/qcp_string_legend_item.h ../qt/widgets/range_syntax_lineedit.h ../qt/widgets/resolved_addresses_view.h @@ -273,6 +274,7 @@ set(WIRESHARK_WIDGET_SRCS ../qt/widgets/path_selection_edit.cpp ../qt/widgets/pref_module_view.cpp ../qt/widgets/profile_tree_view.cpp + ../qt/widgets/qcp_axis_ticker_si.cpp ../qt/widgets/qcp_string_legend_item.cpp ../qt/widgets/range_syntax_lineedit.cpp ../qt/widgets/resolved_addresses_view.cpp diff --git a/ui/qt/CMakeLists.txt b/ui/qt/CMakeLists.txt index a311c5c75c..e07f1c3874 100644 --- a/ui/qt/CMakeLists.txt +++ b/ui/qt/CMakeLists.txt @@ -45,6 +45,7 @@ set(WIRESHARK_WIDGET_HEADERS widgets/path_selection_edit.h widgets/pref_module_view.h widgets/profile_tree_view.h + widgets/qcp_axis_ticker_si.h widgets/qcp_string_legend_item.h widgets/range_syntax_lineedit.h widgets/resolved_addresses_view.h @@ -308,6 +309,7 @@ set(WIRESHARK_WIDGET_SRCS widgets/path_selection_edit.cpp widgets/pref_module_view.cpp widgets/profile_tree_view.cpp + widgets/qcp_axis_ticker_si.cpp widgets/qcp_string_legend_item.cpp widgets/range_syntax_lineedit.cpp widgets/resolved_addresses_view.cpp diff --git a/ui/qt/io_graph_dialog.cpp b/ui/qt/io_graph_dialog.cpp index abba64884f..915e207646 100644 --- a/ui/qt/io_graph_dialog.cpp +++ b/ui/qt/io_graph_dialog.cpp @@ -27,6 +27,7 @@ #include #include #include +#include #include "progress_frame.h" #include "main_application.h" @@ -576,7 +577,7 @@ void IOGraphDialog::createIOGraph(int currentRow) ioGraphs_.append(new IOGraph(ui->ioPlot)); IOGraph* iog = ioGraphs_[currentRow]; - connect(this, SIGNAL(recalcGraphData(capture_file *, bool)), iog, SLOT(recalcGraphData(capture_file *, bool))); + connect(this, SIGNAL(recalcGraphData(capture_file *)), iog, SLOT(recalcGraphData(capture_file *))); connect(this, SIGNAL(reloadValueUnitFields()), iog, SLOT(reloadValueUnitField())); connect(&cap_file_, SIGNAL(captureEvent(CaptureEvent)), iog, SLOT(captureEvent(CaptureEvent))); @@ -960,6 +961,7 @@ void IOGraphDialog::getGraphInfo() void IOGraphDialog::updateLegend() { QCustomPlot *iop = ui->ioPlot; + QSet format_units_set; QSet vu_label_set; QString intervalText = ui->intervalComboBox->itemText(ui->intervalComboBox->currentIndex()); @@ -972,10 +974,8 @@ void IOGraphDialog::updateLegend() IOGraph *iog = ioGraphs_.value(row, Q_NULLPTR); if (graphIsEnabled(row) && iog) { QString label(iog->valueUnitLabel()); - if (!iog->scaledValueUnit().isEmpty()) { - label += " (" + iog->scaledValueUnit() + ")"; - } vu_label_set.insert(label); + format_units_set.insert(iog->formatUnits()); } } } @@ -986,6 +986,28 @@ void IOGraphDialog::updateLegend() return; } + format_size_units_e format_units = FORMAT_SIZE_UNIT_NONE; + if (format_units_set.size() == 1) { + format_units = format_units_set.values()[0]; + } + + QSharedPointer si_ticker = qSharedPointerDynamicCast(iop->yAxis->ticker()); + if (format_units != FORMAT_SIZE_UNIT_NONE) { + if (si_ticker) { + si_ticker->setUnit(format_units); + } else { + iop->yAxis->setTicker(QSharedPointer(new QCPAxisTickerSi(format_units, QString(), ui->logCheckBox->isChecked()))); + } + } else { + if (si_ticker) { + if (ui->logCheckBox->isChecked()) { + iop->yAxis->setTicker(QSharedPointer(new QCPAxisTickerLog)); + } else { + iop->yAxis->setTicker(QSharedPointer(new QCPAxisTicker)); + } + } + } + // All the same. Use the Y Axis label. if (vu_label_set.size() == 1) { iop->yAxis->setLabel(vu_label_set.values()[0] + "/" + intervalText); @@ -1217,18 +1239,8 @@ void IOGraphDialog::updateStatistics() if (need_recalc_ && !file_closed_ && prefs.gui_io_graph_automatic_update) { need_recalc_ = false; need_replot_ = true; - int enabled_graphs = 0; - if (uat_model_ != NULL) { - for (int row = 0; row < uat_model_->rowCount(); row++) { - if (graphIsEnabled(row)) { - ++enabled_graphs; - } - } - } - // With multiple visible graphs, disable Y scaling to avoid - // multiple, distinct units. - emit recalcGraphData(cap_file_.capFile(), enabled_graphs == 1); + emit recalcGraphData(cap_file_.capFile()); if (!tracer_->graph()) { if (base_graph_ && base_graph_->data()->size() > 0) { tracer_->setGraph(base_graph_); @@ -1525,13 +1537,21 @@ void IOGraphDialog::on_zoomRadioButton_toggled(bool checked) void IOGraphDialog::on_logCheckBox_toggled(bool checked) { QCustomPlot *iop = ui->ioPlot; + QSharedPointer si_ticker = qSharedPointerDynamicCast(iop->yAxis->ticker()); + if (si_ticker != nullptr) { + si_ticker->setLog(checked); + } if (checked) { iop->yAxis->setScaleType(QCPAxis::stLogarithmic); - iop->yAxis->setTicker(QSharedPointer(new QCPAxisTickerLog)); + if (si_ticker == nullptr) { + iop->yAxis->setTicker(QSharedPointer(new QCPAxisTickerLog)); + } } else { iop->yAxis->setScaleType(QCPAxis::stLinear); - iop->yAxis->setTicker(QSharedPointer(new QCPAxisTicker)); + if (si_ticker == nullptr) { + iop->yAxis->setTicker(QSharedPointer(new QCPAxisTicker)); + } } iop->replot(); } @@ -2195,7 +2215,7 @@ void IOGraph::clearAllData() start_time_ = 0.0; } -void IOGraph::recalcGraphData(capture_file *cap_file, bool enable_scaling) +void IOGraph::recalcGraphData(capture_file *cap_file) { /* Moving average variables */ unsigned int mavg_in_average_count = 0, mavg_left = 0; @@ -2278,71 +2298,41 @@ void IOGraph::recalcGraphData(capture_file *cap_file, bool enable_scaling) // qDebug() << "=rgd i" << i << ts << val; } - // attempt to rescale time values to specific units if this - // is the only enabled graph - if (enable_scaling && visible_) { - calculateScaledValueUnit(); - } else { - scaled_value_unit_.clear(); - } - emit requestReplot(); } -void IOGraph::calculateScaledValueUnit() +format_size_units_e IOGraph::formatUnits() const { - // Reset unit and recalculate if needed. - scaled_value_unit_.clear(); - - // If there is no field, scaling is not possible. - if (hf_index_ < 0) { - return; - } - switch (val_units_) { + case IOG_ITEM_UNIT_PACKETS: + case IOG_ITEM_UNIT_CALC_FRAMES: + return FORMAT_SIZE_UNIT_PACKETS; + case IOG_ITEM_UNIT_BYTES: + return FORMAT_SIZE_UNIT_BYTES; + case IOG_ITEM_UNIT_BITS: + return FORMAT_SIZE_UNIT_BITS; + case IOG_ITEM_UNIT_CALC_LOAD: + return FORMAT_SIZE_UNIT_ERLANGS; + break; + case IOG_ITEM_UNIT_CALC_FIELDS: + return FORMAT_SIZE_UNIT_FIELDS; + break; case IOG_ITEM_UNIT_CALC_SUM: case IOG_ITEM_UNIT_CALC_MAX: case IOG_ITEM_UNIT_CALC_MIN: case IOG_ITEM_UNIT_CALC_AVERAGE: // Unit is not yet known, continue detecting it. - break; + if (hf_index_ > 0) { + if (proto_registrar_get_ftype(hf_index_) == FT_RELATIVE_TIME) { + return FORMAT_SIZE_UNIT_SECONDS; + } + // Could we look if it's BASE_UNIT_STRING and use that? + // One complication is that prefixes shouldn't be combined, + // and some unit strings are already prefixed units. + } + return FORMAT_SIZE_UNIT_NONE; default: - // Unit is Packets, Bytes, Bits, etc. - return; - } - - if (proto_registrar_get_ftype(hf_index_) == FT_RELATIVE_TIME) { - // find maximum absolute value and scale accordingly - double maxValue = 0; - if (graph_) { - maxValue = maxValueFromGraphData(*graph_->data()); - } else if (bars_) { - maxValue = maxValueFromGraphData(*bars_->data()); - } - // If the maximum value is zero, then either we have no data or - // everything is zero, do not scale the unit in this case. - if (maxValue == 0) { - return; - } - - // XXX GTK+ always uses "ms" for log scale, should we do that too? - int value_multiplier; - if (maxValue >= 1.0) { - scaled_value_unit_ = "s"; - value_multiplier = 1; - } else if (maxValue >= 0.001) { - scaled_value_unit_ = "ms"; - value_multiplier = 1000; - } else { - scaled_value_unit_ = "us"; - value_multiplier = 1000000; - } - - if (graph_) { - scaleGraphData(*graph_->data(), value_multiplier); - } else if (bars_) { - scaleGraphData(*bars_->data(), value_multiplier); - } + return FORMAT_SIZE_UNIT_NONE; } } diff --git a/ui/qt/io_graph_dialog.h b/ui/qt/io_graph_dialog.h index b936257019..a76fd6b369 100644 --- a/ui/qt/io_graph_dialog.h +++ b/ui/qt/io_graph_dialog.h @@ -23,6 +23,8 @@ #include #include +#include + #include #include #include @@ -75,6 +77,7 @@ public: void setColor(const QRgb color); void setPlotStyle(int style); QString valueUnitLabel() const; + format_size_units_e formatUnits() const; void setValueUnits(int val_units); QString valueUnitField() const { return vu_field_; } void setValueUnitField(const QString &vu_field); @@ -89,7 +92,6 @@ public: bool hasItemToShow(int idx, double value) const; double getItemValue(int idx, const capture_file *cap_file) const; int maxInterval () const { return cur_idx_; } - QString scaledValueUnit() const { return scaled_value_unit_; } void clearAllData(); @@ -97,7 +99,7 @@ public: unsigned int y_axis_factor_; public slots: - void recalcGraphData(capture_file *cap_file, bool enable_scaling); + void recalcGraphData(capture_file *cap_file); void captureEvent(CaptureEvent e); void reloadValueUnitField(); @@ -112,7 +114,6 @@ private: static tap_packet_status tapPacket(void *iog_ptr, packet_info *pinfo, epan_dissect_t *edt, const void *data, tap_flags_t flags); static void tapDraw(void *iog_ptr); - void calculateScaledValueUnit(); template double maxValueFromGraphData(const DataMap &map); template void scaleGraphData(DataMap &map, int scalar); @@ -131,7 +132,6 @@ private: int hf_index_; int interval_; double start_time_; - QString scaled_value_unit_; // Cached data. We should be able to change the Y axis without retapping as // much as is feasible. @@ -172,7 +172,7 @@ protected: signals: void goToPacket(int packet_num); - void recalcGraphData(capture_file *cap_file, bool enable_scaling); + void recalcGraphData(capture_file *cap_file); void intervalChanged(int interval); void reloadValueUnitFields(); diff --git a/ui/qt/widgets/qcp_axis_ticker_si.cpp b/ui/qt/widgets/qcp_axis_ticker_si.cpp new file mode 100644 index 0000000000..be6d35a614 --- /dev/null +++ b/ui/qt/widgets/qcp_axis_ticker_si.cpp @@ -0,0 +1,74 @@ +/** @file + * + * QCustomPlot QCPAxisTicker subclass that creates human-readable + * SI unit labels, optionally supporting log scale. + * + * Copyright 2024 John Thacker + * + * Wireshark - Network traffic analyzer + * By Gerald Combs + * Copyright 1998 Gerald Combs + * + * SPDX-License-Identifier: GPL-2.0-or-later + */ + +#include + +#include +#include + +#include + +QCPAxisTickerSi::QCPAxisTickerSi(format_size_units_e unit, QString customUnit, bool log) : + mUnit(unit), mCustomUnit(customUnit), mLog(log) +{ +} + +QString QCPAxisTickerSi::getTickLabel(double tick, const QLocale& , QChar , int precision) +{ + QString label = gchar_free_to_qstring(format_units(nullptr, tick, mUnit, FORMAT_SIZE_PREFIX_SI, precision)); + + // XXX - format_units isn't consistent about whether we need to + // add a space or not + if (mUnit == FORMAT_SIZE_UNIT_NONE && !mCustomUnit.isEmpty()) { + label += mCustomUnit; + } + // XXX - "Beautiful typeset powers" for exponentials is handled by QCPAxis, + // not QCPAxisTicker and its subclasses, and its detection of exponentials + // doesn't handle having a unit or other suffix, so that won't work. + // In practical use we'll be within our prefix range, though. + return label; +} + +int QCPAxisTickerSi::getSubTickCount(double tickStep) +{ + if (mLog) { + return QCPAxisTickerLog::getSubTickCount(tickStep); + } else { + return QCPAxisTicker::getSubTickCount(tickStep); + } +} + +QVector QCPAxisTickerSi::createTickVector(double tickStep, const QCPRange &range) +{ + if (mLog) { + return QCPAxisTickerLog::createTickVector(tickStep, range); + } else { + return QCPAxisTicker::createTickVector(tickStep, range); + } +} + +void QCPAxisTickerSi::setUnit(format_size_units_e unit) +{ + mUnit = unit; +} + +void QCPAxisTickerSi::setCustomUnit(QString unit) +{ + mCustomUnit = unit; +} + +void QCPAxisTickerSi::setLog(bool log) +{ + mLog = log; +} diff --git a/ui/qt/widgets/qcp_axis_ticker_si.h b/ui/qt/widgets/qcp_axis_ticker_si.h new file mode 100644 index 0000000000..a9fa51cc37 --- /dev/null +++ b/ui/qt/widgets/qcp_axis_ticker_si.h @@ -0,0 +1,42 @@ +/** @file + * + * QCustomPlot QCPAxisTicker subclass that creates human-readable + * SI unit labels, optionally supporting log scale. + * + * Copyright 2024 John Thacker + * + * Wireshark - Network traffic analyzer + * By Gerald Combs + * Copyright 1998 Gerald Combs + * + * SPDX-License-Identifier: GPL-2.0-or-later + */ + +#ifndef QCP_AXIS_TICKER_SI_H +#define QCP_AXIS_TICKER_SI_H + +#include + +#include + +class QCPAxisTickerSi : public QCPAxisTickerLog +{ +public: + explicit QCPAxisTickerSi(format_size_units_e unit = FORMAT_SIZE_UNIT_PACKETS, QString customUnit = QString(), bool log = false); + + format_size_units_e getUnit() const { return mUnit; } + void setUnit(format_size_units_e unit); + void setCustomUnit(QString unit); + void setLog(bool log); + +protected: + virtual QString getTickLabel(double tick, const QLocale &locale, QChar formatChar, int precision) override; + virtual int getSubTickCount(double tickStep) override; + virtual QVector createTickVector(double tickStep, const QCPRange &range) override; + + format_size_units_e mUnit; + QString mCustomUnit; + bool mLog; +}; + +#endif diff --git a/wsutil/str_util.c b/wsutil/str_util.c index f1f8dad8f8..8a138ec172 100644 --- a/wsutil/str_util.c +++ b/wsutil/str_util.c @@ -550,6 +550,9 @@ format_units(wmem_allocator_t *allocator, double size, case FORMAT_SIZE_UNIT_PACKETS_S: wmem_strbuf_append(human_str, is_small ? "packets/s" : "packets/s"); break; + case FORMAT_SIZE_UNIT_FIELDS: + wmem_strbuf_append(human_str, is_small ? "fields" : "fields"); + break; case FORMAT_SIZE_UNIT_SECONDS: wmem_strbuf_append(human_str, is_small ? "seconds" : "s"); break; @@ -639,6 +642,12 @@ format_size_wmem(wmem_allocator_t *allocator, int64_t size, case FORMAT_SIZE_UNIT_PACKETS_S: wmem_strbuf_append(human_str, is_small ? " packets/s" : "packets/s"); break; + case FORMAT_SIZE_UNIT_FIELDS: + wmem_strbuf_append(human_str, is_small ? " fields" : "fields"); + break; + /* These aren't that practical to use with integers, but + * perhaps better than asserting. + */ case FORMAT_SIZE_UNIT_SECONDS: wmem_strbuf_append(human_str, is_small ? " seconds" : "s"); break; diff --git a/wsutil/str_util.h b/wsutil/str_util.h index 4b966ac4b5..859276710d 100644 --- a/wsutil/str_util.h +++ b/wsutil/str_util.h @@ -230,13 +230,11 @@ int ws_xton(char ch); typedef enum { FORMAT_SIZE_UNIT_NONE, /**< No unit will be appended. You must supply your own. */ - /* Note: This does not append a trailing space if there is no prefix. - * If you intend to append the unit (instead of listing it in a legend - * header somewhere else), you should test whether there is a prefix - * (g_ascii_isdigit() on the last character almost surely works, though not - * if the size was infinite or NaN) and append a space before the unit - * if not (and optionally use a different form for the unit, as with - * certain types below.) + /* XXX - This does not append a trailing space if there is no prefix. + * That's good if you intend to list the unit somewhere else, e.g. in a + * legend, header, or other column, but doesn't work well if intending + * to append your own unit. You can test whether there's a prefix or + * not with g_ascii_isdigit() (plus special handling for inf and NaN). */ FORMAT_SIZE_UNIT_BYTES, /**< "bytes" for un-prefixed sizes, "B" otherwise. */ FORMAT_SIZE_UNIT_BITS, /**< "bits" for un-prefixed sizes, "b" otherwise. */ @@ -244,6 +242,8 @@ typedef enum { FORMAT_SIZE_UNIT_BYTES_S, /**< "bytes/s" for un-prefixed sizes, "Bps" otherwise. */ FORMAT_SIZE_UNIT_PACKETS, /**< "packets" */ FORMAT_SIZE_UNIT_PACKETS_S, /**< "packets/s" */ + FORMAT_SIZE_UNIT_FIELDS, /**< "fields" */ + /* These next two aren't really for format_size (which takes an int) */ FORMAT_SIZE_UNIT_SECONDS, /**< "seconds" for un-prefixed sizes, "s" otherwise. */ FORMAT_SIZE_UNIT_ERLANGS, /**< "erlangs" for un-prefixed sizes, "E" otherwise. */ } format_size_units_e;