Repository: jtablesaw/tablesaw
Pull request #691 (2019-10-24 12:52:05)

Adds plotly config argument. Only config.displayModeBar
Thanks for contributing.

- [x] Tick to sign-off your agreement to the [Developer Certificate of Origin (DCO) 1.1](https://developercertificate.org)

## Description

Plot.ly has a `config` argument ( https://plot.ly/javascript/configuration-options/ ) as the last argument to `Plotly.plot`. This has many options to customize the UI.

This PR adds the initial way of supporting this `config` object and adds the `displayModeBar` option.

Note that I've made `config` part of the `Layout` object. This isn't how Plot.ly does it but it seems convenient to have a single 'configuration object'. The `config` field is not saved to the context nor serialized to JS by `Layout`. It is the job of `Figure` to do that. If you don't like it I can remove this part.

## Testing

Yes, a basic one.


emilianbold: The Travis build shows as pending but seems to have finished successfully.

Would appreciate some feedback on this.
lwhite1: Hi @emilianbold 

Sorry about the lack of feedback. We only have three active maintainers and I'm the only one who does visualization stuff. Right now I'm on vacation and when I get back on Tuesday I'm buried with work at my real job. (i work on cancer diagnostics so I don't feel right sneaking time out for my open source when we're behind.) Tablesaw needs more collaborators. 

If you'd like to consider becoming a maintainer, I will find time to review your submission and comments today. Otherwise, I will get to it when I can.

Again. sorry it's not quicker. Just a very busy time right now.  



emilianbold: I can become a maintainer: I have the time and a project I work on depends on Tablesaw.

I mostly need to add many things to the visualization part these days.
lwhite1: > I can become a maintainer: I have the time and a project I work on depends on Tablesaw.
> I mostly need to add many things to the visualization part these days.

That sounds great. I have a bit of time now and will try to at least answer your questions. In general, visualization is the place with the greatest need for work, so this could work out well.

> Plot.ly has a config argument ( https://plot.ly/javascript/configuration-options/ ) as the last argument to Plotly.plot. This has many options to customize the UI. 

I didn't realize this existed. It would be awesome to have this in Tablesaw.  

> Note that I've made config part of the Layout object. This isn't how Plot.ly does it but it seems convenient to have a single 'configuration object'. The config field is not saved to the context nor serialized to JS by Layout. It is the job of Figure to do that. If you don't like it I can remove this part.

I think i prefer to mirror the Plot.ly code for a few reasons: 
- even though they're both configs, they're intended to deal with different plotting needs: layout vs interaction (of course there is some overlap, but the idea makes sense, I think.)
- keeping a design that is close to plot.ly may pay off if the Plot.ly code is restructured. It will be clear where to look for any of our code that needs to change. 
- having the Layout serialize some of its elements but not all of them is likely to confuse people. "I created my figure and am serializing the layout, but..."
- having object A hold B and still having to pass both into the Figure builder

So i would have the figure builder take three objects: layout, config, and trace(s) as you are,  but leave the config and layout objects separate.

Does this sound ok? 

(I'll come back to your other PR a little later tonight)

emilianbold: Yes, it makes sense to keep layout and config separate. I've pushed a commit that fixes this.

Now only `Figure` and the builder deals with `Config`.

Patch:
diff --git a/jsplot/src/main/java/tech/tablesaw/plotly/components/Config.java b/jsplot/src/main/java/tech/tablesaw/plotly/components/Config.java
new file mode 100644
index 00000000..e1bbcb32
--- /dev/null
+++ b/jsplot/src/main/java/tech/tablesaw/plotly/components/Config.java
@@ -0,0 +1,58 @@
+package tech.tablesaw.plotly.components;
+
+import com.mitchellbosecke.pebble.error.PebbleException;
+import com.mitchellbosecke.pebble.template.PebbleTemplate;
+import java.io.IOException;
+import java.io.StringWriter;
+import java.io.Writer;
+import java.util.HashMap;
+import java.util.Map;
+
+public class Config extends Component {
+
+  private final Boolean displayModeBar;
+
+  private Config(Builder builder) {
+    this.displayModeBar = builder.displayModeBar;
+  }
+
+  public static Builder builder() {
+    return new Builder();
+  }
+
+  @Override
+  public String asJavascript() {
+    Writer writer = new StringWriter();
+    PebbleTemplate compiledTemplate;
+
+    try {
+      compiledTemplate = engine.getTemplate("config_template.html");
+      compiledTemplate.evaluate(writer, getContext());
+    } catch (PebbleException | IOException e) {
+      e.printStackTrace();
+    }
+    return writer.toString();
+  }
+
+  private Map<String, Object> getContext() {
+    Map<String, Object> context = new HashMap<>();
+    context.put("displayModeBar", displayModeBar);
+    return context;
+  }
+
+  public static class Builder {
+
+    Boolean displayModeBar;
+
+    private Builder() {}
+
+    public Builder displayModeBar(boolean displayModeBar) {
+      this.displayModeBar = displayModeBar;
+      return this;
+    }
+
+    public Config build() {
+      return new Config(this);
+    }
+  }
+}
diff --git a/jsplot/src/main/java/tech/tablesaw/plotly/components/Figure.java b/jsplot/src/main/java/tech/tablesaw/plotly/components/Figure.java
index a93caae8..6ba04627 100644
--- a/jsplot/src/main/java/tech/tablesaw/plotly/components/Figure.java
+++ b/jsplot/src/main/java/tech/tablesaw/plotly/components/Figure.java
@@ -27,6 +27,7 @@ public class Figure {
 
   private final Trace[] data;
   private final Layout layout;
+  private final Config config;
   private final EventHandler[] eventHandlers;
 
   private final Map<String, Object> context = new HashMap<>();
@@ -36,27 +37,29 @@ public class Figure {
   public Figure(FigureBuilder builder) {
     this.data = builder.traces();
     this.layout = builder.layout;
+    this.config = builder.config;
     this.eventHandlers = builder.eventHandlers();
   }
 
   public Figure(Trace... traces) {
-    this.data = traces;
-    this.layout = null;
-    this.eventHandlers = null;
+    this((Layout) null, traces);
   }
 
   public Figure(Layout layout, Trace... traces) {
+    this(layout, (Config) null, traces);
+  }
+
+  public Figure(Layout layout, Config config, Trace... traces) {
     this.data = traces;
     this.layout = layout;
+    this.config = config;
     this.eventHandlers = null;
   }
 
   /** @deprecated Use the FigureBuilder instead */
   @Deprecated
   public Figure(Layout layout, EventHandler eventHandler, Trace... traces) {
-    this.data = traces;
-    this.layout = layout;
-    this.eventHandlers = new EventHandler[] {eventHandler};
+    this(layout, new EventHandler[] {eventHandler}, traces);
   }
 
   /** @deprecated Use the FigureBuilder instead */
@@ -64,6 +67,7 @@ public class Figure {
   public Figure(Layout layout, EventHandler[] eventHandlers, Trace... traces) {
     this.data = traces;
     this.layout = layout;
+    this.config = null;
     this.eventHandlers = eventHandlers;
   }
 
@@ -101,6 +105,9 @@ public class Figure {
     if (layout != null) {
       builder.append(layout.asJavascript());
     }
+    if (config != null) {
+      builder.append(config.asJavascript());
+    }
     builder.append(System.lineSeparator());
     for (int i = 0; i < data.length; i++) {
       Trace trace = data[i];
@@ -132,6 +139,10 @@ public class Figure {
       builder.append(", ");
       builder.append("layout");
     }
+    if (config != null) {
+      builder.append(", ");
+      builder.append("config");
+    }
 
     builder.append(");");
 
@@ -163,6 +174,7 @@ public class Figure {
   public static class FigureBuilder {
 
     private Layout layout;
+    private Config config;
     private List<Trace> traces = new ArrayList<>();
     private List<EventHandler> eventHandlers = new ArrayList<>();
 
@@ -171,6 +183,11 @@ public class Figure {
       return this;
     }
 
+    public FigureBuilder config(Config config) {
+      this.config = config;
+      return this;
+    }
+
     public FigureBuilder addTraces(Trace... traces) {
       this.traces.addAll(Arrays.asList(traces));
       return this;
diff --git a/jsplot/src/main/resources/config_template.html b/jsplot/src/main/resources/config_template.html
new file mode 100644
index 00000000..52564f2a
--- /dev/null
+++ b/jsplot/src/main/resources/config_template.html
@@ -0,0 +1,5 @@
+var config = {
+{% if displayModeBar is not null %}
+    displayModeBar: {{displayModeBar}}
+{% endif %}
+}
\ No newline at end of file


Test patch:
diff --git a/jsplot/src/test/java/tech/tablesaw/plotly/components/ConfigTest.java b/jsplot/src/test/java/tech/tablesaw/plotly/components/ConfigTest.java
new file mode 100644
index 00000000..535db5fd
--- /dev/null
+++ b/jsplot/src/test/java/tech/tablesaw/plotly/components/ConfigTest.java
@@ -0,0 +1,25 @@
+package tech.tablesaw.plotly.components;
+
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import org.junit.jupiter.api.Test;
+
+public class ConfigTest {
+
+  @Test
+  public void testJavascript() {
+    {
+      Config config = Config.builder().displayModeBar(true).build();
+      assertTrue(config.asJavascript().contains("displayModeBar: true"));
+    }
+    {
+      Config config = Config.builder().displayModeBar(false).build();
+      assertTrue(config.asJavascript().contains("displayModeBar: false"));
+    }
+    {
+      Config config = Config.builder().build();
+      assertFalse(config.asJavascript().contains("displayModeBar"));
+    }
+  }
+}