Split project settings and app settings into separate classes (#724)

* Split project settings and app settings into separate classes

Move port validation to GripServer and common CLI to the helper class
Make project settings reflect command-line port option
No longer serializes the server port. Use the command-line option or the settings dialog to set it. Fixes #708
Enable projects to be uploaded via HTTP in UI mode

* Move *BeanInfo classes to UI module. Fixes #717

* Ignore all unknown tags in save files

Author: SamCarlberg

core/src/main/java/edu/wpi/grip/core/CoreCommandLineHelper.java

@@ -1,6 +1,13 @@
 package edu.wpi.grip.core;
 
+import edu.wpi.grip.core.events.AppSettingsChangedEvent;
+import edu.wpi.grip.core.http.GripServer;
+import edu.wpi.grip.core.serialization.Project;
+import edu.wpi.grip.core.settings.AppSettings;
+import edu.wpi.grip.core.settings.SettingsProvider;
+
 import com.google.common.annotations.VisibleForTesting;
+import com.google.common.eventbus.EventBus;
 
 import org.apache.commons.cli.CommandLine;
 import org.apache.commons.cli.DefaultParser;
@@ -9,13 +16,19 @@
 import org.apache.commons.cli.Options;
 import org.apache.commons.cli.ParseException;
 
+import java.io.File;
+import java.io.IOException;
 import java.util.List;
+import java.util.logging.Level;
+import java.util.logging.Logger;
 
 /**
  * A helper class for command line options for GRIP.
  */
 public class CoreCommandLineHelper {
 
+  private static final Logger logger = Logger.getLogger("CommandLine");
+
   public static final String FILE_OPTION = "f"; // "f" for "file"
   public static final String PORT_OPTION = "p"; // "p" for "port"
   public static final String HELP_OPTION = "h"; // "h" for "help" (this is standard)
@@ -134,4 +147,52 @@ void exit() {
     System.exit(0);
   }
 
+  /**
+   * Tries to load a file from the command line arguments. Does nothing if no file was specified.
+   *
+   * @param args    the parsed command line arguments
+   * @param project the project to load the file into
+   *
+   * @throws IOException if the file couldn't be loaded
+   */
+  public void loadFile(CommandLine args, Project project) throws IOException {
+    if (args.hasOption(FILE_OPTION)) {
+      String file = args.getOptionValue(FILE_OPTION);
+      try {
+        project.open(new File(file));
+      } catch (IOException e) {
+        logger.log(Level.WARNING, "Invalid file: " + file, e);
+        throw e;
+      }
+    }
+  }
+
+  /**
+   * Tries to set the internal server port from the command line arguments. Does nothing if no port
+   * was specified.
+   *
+   * @param args             the parsed command line arguments
+   * @param settingsProvider the app's settings provider
+   * @param eventBus         the app's event bus
+   */
+  public void setServerPort(CommandLine args,
+                            SettingsProvider settingsProvider,
+                            EventBus eventBus) {
+    if (args.hasOption(PORT_OPTION)) {
+      try {
+        int port = Integer.parseInt(args.getOptionValue(PORT_OPTION));
+        if (!GripServer.isPortValid(port)) {
+          logger.warning("Not a valid port: " + port);
+        } else {
+          logger.info("Setting server port: " + port);
+          AppSettings settings = settingsProvider.getAppSettings().clone();
+          settings.setServerPort(port);
+          eventBus.post(new AppSettingsChangedEvent(settings));
+        }
+      } catch (NumberFormatException e) {
+        logger.warning("Not a valid port: " + args.getOptionValue(PORT_OPTION));
+      }
+    }
+  }
+
 }

core/src/main/java/edu/wpi/grip/core/Main.java

@@ -8,6 +8,7 @@
 import edu.wpi.grip.core.operations.Operations;
 import edu.wpi.grip.core.operations.network.GripNetworkModule;
 import edu.wpi.grip.core.serialization.Project;
+import edu.wpi.grip.core.settings.SettingsProvider;
 import edu.wpi.grip.core.sources.GripSourcesHardwareModule;
 
 import com.google.common.eventbus.EventBus;
@@ -19,7 +20,6 @@
 
 import org.apache.commons.cli.CommandLine;
 
-import java.io.File;
 import java.io.IOException;
 import java.util.logging.Level;
 import java.util.logging.Logger;
@@ -38,6 +38,8 @@ public class Main {
   @Inject
   private PipelineRunner pipelineRunner;
   @Inject
+  private SettingsProvider settingsProvider;
+  @Inject
   private EventBus eventBus;
   @Inject
   private Operations operations;
@@ -66,30 +68,8 @@ public void start(String[] args) throws IOException, InterruptedException {
     CoreCommandLineHelper commandLineHelper = new CoreCommandLineHelper();
     CommandLine parsedArgs = commandLineHelper.parse(args);
 
-    // Parse the save file option
-    if (parsedArgs.hasOption(CoreCommandLineHelper.FILE_OPTION)) {
-      // Open a project from a .grip file specified on the command line
-      String file = parsedArgs.getOptionValue(CoreCommandLineHelper.FILE_OPTION);
-      logger.log(Level.INFO, "Loading file " + file);
-      project.open(new File(file));
-    }
-
-    // Set the port AFTER loading the project to override the saved port number
-    if (parsedArgs.hasOption(CoreCommandLineHelper.PORT_OPTION)) {
-      try {
-        int port = Integer.parseInt(parsedArgs.getOptionValue(CoreCommandLineHelper.PORT_OPTION));
-        if (port < 1024 || port > 65535) {
-          logger.warning("Not a valid port: " + port);
-        } else {
-          // Valid port; set it (Note: this doesn't check to see if the port is available)
-          logger.info("Running server on port " + port);
-          gripServer.setPort(port);
-        }
-      } catch (NumberFormatException e) {
-        logger.warning(
-            "Not a valid port: " + parsedArgs.getOptionValue(CoreCommandLineHelper.PORT_OPTION));
-      }
-    }
+    commandLineHelper.loadFile(parsedArgs, project);
+    commandLineHelper.setServerPort(parsedArgs, settingsProvider, eventBus);
 
     // This will throw an exception if the port specified by the save file or command line
     // argument is already taken. Since we have to have the server running to handle remotely

core/src/main/java/edu/wpi/grip/core/Pipeline.java

@@ -1,5 +1,6 @@
 package edu.wpi.grip.core;
 
+import edu.wpi.grip.core.events.AppSettingsChangedEvent;
 import edu.wpi.grip.core.events.ConnectionAddedEvent;
 import edu.wpi.grip.core.events.ConnectionRemovedEvent;
 import edu.wpi.grip.core.events.ProjectSettingsChangedEvent;
@@ -8,6 +9,7 @@
 import edu.wpi.grip.core.events.StepAddedEvent;
 import edu.wpi.grip.core.events.StepMovedEvent;
 import edu.wpi.grip.core.events.StepRemovedEvent;
+import edu.wpi.grip.core.settings.AppSettings;
 import edu.wpi.grip.core.settings.ProjectSettings;
 import edu.wpi.grip.core.settings.SettingsProvider;
 import edu.wpi.grip.core.sockets.InputSocket;
@@ -64,6 +66,8 @@ public class Pipeline implements ConnectionValidator, SettingsProvider, StepInde
   private transient EventBus eventBus;
   private final transient ReadWriteLock stepLock = new ReentrantReadWriteLock();
   private ProjectSettings settings = new ProjectSettings();
+  @XStreamOmitField
+  private transient AppSettings appSettings = new AppSettings(); // Do not serialize this field
 
   /**
    * Locks the resource with the specified lock and performs the function. When the function is
@@ -188,6 +192,11 @@ public ProjectSettings getProjectSettings() {
     return settings;
   }
 
+  @Override
+  public AppSettings getAppSettings() {
+    return appSettings;
+  }
+
   @SuppressWarnings("unchecked")
   @Override
   public boolean canConnect(OutputSocket<?> outputSocket, InputSocket<?> inputSocket) {
@@ -391,4 +400,10 @@ public void onConnectionRemoved(ConnectionRemovedEvent event) {
   public void onProjectSettingsChanged(ProjectSettingsChangedEvent event) {
     this.settings = event.getProjectSettings();
   }
+
+  @Subscribe
+  public void onAppSettingsChanged(AppSettingsChangedEvent event) {
+    this.appSettings = event.getAppSettings();
+  }
+
 }

core/src/main/java/edu/wpi/grip/core/events/AppSettingsChangedEvent.java

@@ -0,0 +1,22 @@
+package edu.wpi.grip.core.events;
+
+import edu.wpi.grip.core.settings.AppSettings;
+
+import static com.google.common.base.Preconditions.checkNotNull;
+
+/**
+ * An event fired when the app settings are changed.
+ */
+public class AppSettingsChangedEvent {
+
+  private final AppSettings appSettings;
+
+  public AppSettingsChangedEvent(AppSettings appSettings) {
+    this.appSettings = checkNotNull(appSettings, "appSettings");
+  }
+
+  public AppSettings getAppSettings() {
+    return appSettings;
+  }
+
+}

core/src/main/java/edu/wpi/grip/core/http/GripServer.java

@@ -1,6 +1,6 @@
 package edu.wpi.grip.core.http;
 
-import edu.wpi.grip.core.events.ProjectSettingsChangedEvent;
+import edu.wpi.grip.core.events.AppSettingsChangedEvent;
 import edu.wpi.grip.core.exception.GripServerException;
 import edu.wpi.grip.core.settings.SettingsProvider;
 
@@ -48,11 +48,17 @@ public class GripServer {
    * Possible lifecycle states of the server.
    */
   public enum State {
-    /** The server has not been started yet. */
+    /**
+     * The server has not been started yet.
+     */
     PRE_RUN,
-    /** The server is currently running. */
+    /**
+     * The server is currently running.
+     */
     RUNNING,
-    /** The server was running and has been stopped. */
+    /**
+     * The server was running and has been stopped.
+     */
     STOPPED
   }
 
@@ -100,6 +106,32 @@ public enum State {
    */
   public static final String DATA_PATH = ROOT_PATH + "/data";
 
+  /**
+   * The default port the server should run on.
+   */
+  public static final int DEFAULT_PORT = 8080;
+
+  /**
+   * The lowest valid TCP port number.
+   */
+  private static final int MIN_PORT = 1024;
+
+  /**
+   * The highest valid TCP port number.
+   */
+  private static final int MAX_PORT = 65535;
+
+  /**
+   * Checks if the given TCP port is valid for a server to run on. This doesn't check availability.
+   *
+   * @param port the port to check
+   *
+   * @return true if the port is valid, false if not
+   */
+  public static boolean isPortValid(int port) {
+    return port >= MIN_PORT && port <= MAX_PORT;
+  }
+
   public interface JettyServerFactory {
     Server create(int port);
   }
@@ -116,7 +148,7 @@ public Server create(int port) {
   GripServer(ContextStore contextStore,
              JettyServerFactory serverFactory,
              SettingsProvider settingsProvider) {
-    this.port = settingsProvider.getProjectSettings().getServerPort();
+    this.port = settingsProvider.getAppSettings().getServerPort();
     this.serverFactory = serverFactory;
     this.server = serverFactory.create(port);
     this.server.setHandler(handlers);
@@ -213,6 +245,9 @@ public State getState() {
    * @param port the new port to run on.
    */
   public void setPort(int port) {
+    if (!isPortValid(port)) {
+      throw new IllegalArgumentException("Invalid port: " + port);
+    }
     stop();
     server = serverFactory.create(port);
     server.setHandler(handlers);
@@ -228,11 +263,10 @@ public int getPort() {
   }
 
   @Subscribe
-  public void settingsChanged(ProjectSettingsChangedEvent event) {
-    int port = event.getProjectSettings().getServerPort();
+  public void settingsChanged(AppSettingsChangedEvent event) {
+    int port = event.getAppSettings().getServerPort();
     if (port != getPort()) {
       setPort(port);
-      start();
     }
   }
 

core/src/main/java/edu/wpi/grip/core/http/HttpPipelineSwitcher.java

@@ -1,7 +1,6 @@
 package edu.wpi.grip.core.http;
 
 import edu.wpi.grip.core.serialization.Project;
-import edu.wpi.grip.core.util.GripMode;
 
 import com.google.inject.Inject;
 import com.google.inject.Singleton;
@@ -22,13 +21,11 @@
 public class HttpPipelineSwitcher extends PedanticHandler {
 
   private final Project project;
-  private final GripMode mode;
 
   @Inject
-  HttpPipelineSwitcher(ContextStore store, Project project, GripMode mode) {
+  HttpPipelineSwitcher(ContextStore store, Project project) {
     super(store, GripServer.PIPELINE_UPLOAD_PATH, true);
     this.project = project;
-    this.mode = mode;
   }
 
   @Override
@@ -41,24 +38,8 @@ protected void handleIfPassed(String target,
       baseRequest.setHandled(true);
       return;
     }
-    switch (mode) {
-      case HEADLESS:
-        project.open(new String(IOUtils.toByteArray(request.getInputStream()), "UTF-8"));
-        response.setStatus(HttpServletResponse.SC_CREATED);
-        baseRequest.setHandled(true);
-        break;
-      case GUI:
-        // Don't run in GUI mode, it doesn't make much sense and can easily deadlock if pipelines
-        // are rapidly posted.
-        // Intentional fall-through to default
-      default:
-        // Don't know the mode or the mode is unsupported; let the client know
-        response.setStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR);
-        sendTextContent(response,
-            String.format("GRIP is not in the correct mode: should be HEADLESS, but is %s", mode),
-            CONTENT_TYPE_PLAIN_TEXT);
-        baseRequest.setHandled(true);
-        break;
-    }
+    project.open(new String(IOUtils.toByteArray(request.getInputStream()), "UTF-8"));
+    response.setStatus(HttpServletResponse.SC_CREATED);
+    baseRequest.setHandled(true);
   }
 }

core/src/main/java/edu/wpi/grip/core/serialization/Project.java

@@ -49,6 +49,7 @@ public void initialize(StepConverter stepConverter,
                          ConnectionConverter connectionConverter,
                          ProjectSettingsConverter projectSettingsConverter) {
     xstream.setMode(XStream.NO_REFERENCES);
+    xstream.ignoreUnknownElements(); // ignores all unknown tags
     xstream.registerConverter(stepConverter);
     xstream.registerConverter(sourceConverter);
     xstream.registerConverter(socketConverter);