Convenience functions to set axis properties in visualization tool.

webrtc/tools/event_log_visualizer/plot_base.h

Index: webrtc/tools/event_log_visualizer/plot_base.h
diff --git a/webrtc/tools/event_log_visualizer/plot_base.h b/webrtc/tools/event_log_visualizer/plot_base.h
index 925dcecf3db1546cc684ff5d5b0500f101670713..f665c677898604a7d4cd68cc2074c04e401a2d33 100644
--- a/webrtc/tools/event_log_visualizer/plot_base.h
+++ b/webrtc/tools/event_log_visualizer/plot_base.h
@@ -10,11 +10,14 @@
 #ifndef WEBRTC_TOOLS_EVENT_LOG_VISUALIZER_PLOT_BASE_H_
 #define WEBRTC_TOOLS_EVENT_LOG_VISUALIZER_PLOT_BASE_H_
 
+#include <algorithm>
 #include <memory>
 #include <string>
 #include <utility>
 #include <vector>
 
+#include "webrtc/base/checks.h"
+
 namespace webrtc {
 namespace plotting {
 
@@ -44,32 +47,90 @@ struct TimeSeries {
   std::vector<TimeSeriesPoint> points;
 };
 
-// This is basically a struct that represents of a general graph, with axes,
+// This is basically a struct that represents a general graph, with axes,
 // title and one or more data series. We make it a class only to document that

[danilchap, 2016/07/26 10:52:46]

Comment probably need an update too: This struct become almost normal class, not
just a struct & interface.

[terelius, 2016/07/26 17:37:38]

Done.

 // it also specifies an interface for the draw()ing objects.
 class Plot {

[danilchap, 2016/07/26 10:52:46]

may be add plot_base.cc and move implementation there to make this more proper
class.

[terelius, 2016/07/26 17:37:38]

Done.

  public:
   virtual ~Plot() {}
-  virtual void draw() = 0;
-
-  float xaxis_min;
-  float xaxis_max;
-  std::string xaxis_label;
-  float yaxis_min;
-  float yaxis_max;
-  std::string yaxis_label;
-  std::vector<TimeSeries> series;
-  std::string title;
+  virtual void Draw() = 0;
+
+  void SetXAxis(float min,
+                float max,
+                std::string label,
+                float left_margin = 0,

[danilchap, 2016/07/26 10:52:46]

it doesn't look like you use default arguments (i.e. didn't notice calls to
these functions with just 3 parameters). May be remove them.
or move Margin constants into this class and use them as default parameters
(since it will be uncommon for margins to be different)

[terelius, 2016/07/26 17:37:38]

They are not used right now, but just setting the axis limits (without adding
any extra margins) seems like a very natural case. 

+                float right_margin = 0) {

[danilchap, 2016/07/26 10:52:46]

margin doesn't use same units as min/max. It might help to hint that.
E.g. by renaming parameters to right_margin_percent (and then divide it by
100.0)

[terelius, 2016/07/26 17:37:38]

I added comments indicating that the margins are measured as fractions of the
interval.

+    xaxis_min_ = min - left_margin * (max - min);
+    xaxis_max_ = max + right_margin * (max - min);
+    xaxis_label_ = label;
+  }
+
+  void SetSuggestedXAxis(float min,
+                         float max,
+                         std::string label,
+                         float left_margin = 0,
+                         float right_margin = 0) {
+    for (auto& series : series_list_) {

[danilchap, 2016/07/26 10:52:46]

const auto& here and next line

[terelius, 2016/07/26 17:37:38]

Done.

+      for (auto& point : series.points) {
+        min = std::min(min, point.x);

[danilchap, 2016/07/26 10:52:46]

std::min as function and min as variable in same line hints that may be
better to rename min to e.g. min_value.
It would also hint min/max are not pixels.

[terelius, 2016/07/26 17:37:38]

Done.

+        max = std::max(max, point.x);
+      }
+    }
+    RTC_DCHECK(max >= min);

[danilchap, 2016/07/26 10:52:46]

RTC_DCHECK_LE(min, max);
and have this check in SetXAxis

[terelius, 2016/07/26 17:37:38]

Done.

+    xaxis_min_ = min - left_margin * (max - min);

[danilchap, 2016/07/26 10:52:46]

may be call SetXAxis instead of duplicating the code.

[terelius, 2016/07/26 17:37:38]

Done.

+    xaxis_max_ = max + right_margin * (max - min);
+    xaxis_label_ = label;
+  }
+
+  void SetYAxis(float min,
+                float max,
+                std::string label,
+                float bottom_margin = 0,
+                float top_margin = 0) {
+    yaxis_min_ = min - bottom_margin * (max - min);
+    yaxis_max_ = max + top_margin * (max - min);
+    yaxis_label_ = label;
+  }
+
+  void SetSuggestedYAxis(float min,
+                         float max,
+                         std::string label,
+                         float bottom_margin = 0,
+                         float top_margin = 0) {
+    for (auto& series : series_list_) {
+      for (auto& point : series.points) {
+        min = std::min(min, point.y);
+        max = std::max(max, point.y);
+      }
+    }
+    RTC_DCHECK(max >= min);
+    yaxis_min_ = min - bottom_margin * (max - min);
+    yaxis_max_ = max + top_margin * (max - min);
+    yaxis_label_ = label;
+  }
+
+  void SetTitle(std::string title) { title_ = title; }
+
+  std::vector<TimeSeries> series_list_;

[danilchap, 2016/07/26 10:52:46]

Why rename this variable to _list when it is not even a list?
generic name 'series' looks better.

Probably better to hide this member into protected section too and add a
function similar to one in PlotCollection:
TimeSeries* AddNewSeries();
or
AddNewSeries(TimeSeries value); // Move constructor can be used then.

[terelius, 2016/07/26 17:37:38]

This variable is a collection of TimeSeries objects. The problem with the name
series is that there is no difference between the singular and plural form, so
we would get strange-looking code like
for (auto& series : series_)

I will consider adding an AddNewSeries()-function in a follow-up CL.

+
+ protected:

[danilchap, 2016/07/26 10:52:46]

either make it private, or update CL description that you didn't

[terelius, 2016/07/26 17:37:38]

Can't be private. Updated CL description.

+  float xaxis_min_;
+  float xaxis_max_;
+  std::string xaxis_label_;
+  float yaxis_min_;
+  float yaxis_max_;
+  std::string yaxis_label_;
+  std::string title_;
 };
 
 class PlotCollection {
  public:
   virtual ~PlotCollection() {}
-  virtual void draw() = 0;
-  virtual Plot* append_new_plot() = 0;
+  virtual void Draw() = 0;
+  virtual Plot* AppendNewPlot() = 0;
 
  protected:
-  std::vector<std::unique_ptr<Plot> > plots;
+  std::vector<std::unique_ptr<Plot> > plots_;
 };
 
 }  // namespace plotting