From 26e7d96105415f3d1c4edb311aaae80ef559e156 Mon Sep 17 00:00:00 2001 From: Sam Carlberg Date: Thu, 1 Dec 2016 22:46:39 -0500 Subject: [PATCH] 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 --- .../wpi/grip/core/CoreCommandLineHelper.java | 61 +++++++++++++++++++ .../src/main/java/edu/wpi/grip/core/Main.java | 30 ++------- .../main/java/edu/wpi/grip/core/Pipeline.java | 15 +++++ .../core/events/AppSettingsChangedEvent.java | 22 +++++++ .../edu/wpi/grip/core/http/GripServer.java | 50 ++++++++++++--- .../grip/core/http/HttpPipelineSwitcher.java | 27 ++------ .../wpi/grip/core/serialization/Project.java | 1 + .../wpi/grip/core/settings/AppSettings.java | 48 +++++++++++++++ .../grip/core/settings/ProjectSettings.java | 20 +----- .../edu/wpi/grip/core/settings/Setting.java | 2 - .../edu/wpi/grip/core/settings/Settings.java | 7 +++ .../grip/core/settings/SettingsProvider.java | 10 +++ .../wpi/grip/core/http/GripServerTest.java | 5 +- .../core/http/HttpPipelineSwitcherTest.java | 32 +++------- .../network/http/HttpPublisherTest.java | 4 +- .../wpi/grip/core/sources/HttpSourceTest.java | 4 +- .../grip/core/sources/SourcesSanityTest.java | 6 +- .../core/settings/AppSettingsBeanInfo.java | 18 ++++++ .../settings/ProjectSettingsBeanInfo.java | 18 ++++++ .../core/settings/SimpleSettingsBeanInfo.java | 38 +++++++----- ui/src/main/java/edu/wpi/grip/ui/Main.java | 35 ++--------- .../edu/wpi/grip/ui/MainWindowController.java | 7 ++- .../wpi/grip/ui/ProjectSettingsEditor.java | 30 ++++++--- 23 files changed, 327 insertions(+), 163 deletions(-) create mode 100644 core/src/main/java/edu/wpi/grip/core/events/AppSettingsChangedEvent.java create mode 100644 core/src/main/java/edu/wpi/grip/core/settings/AppSettings.java create mode 100644 core/src/main/java/edu/wpi/grip/core/settings/Settings.java create mode 100644 ui/src/main/java/edu/wpi/grip/core/settings/AppSettingsBeanInfo.java create mode 100644 ui/src/main/java/edu/wpi/grip/core/settings/ProjectSettingsBeanInfo.java rename core/src/main/java/edu/wpi/grip/core/settings/ProjectSettingsBeanInfo.java => ui/src/main/java/edu/wpi/grip/core/settings/SimpleSettingsBeanInfo.java (51%) diff --git a/core/src/main/java/edu/wpi/grip/core/CoreCommandLineHelper.java b/core/src/main/java/edu/wpi/grip/core/CoreCommandLineHelper.java index 159448623b..139138b83f 100644 --- a/core/src/main/java/edu/wpi/grip/core/CoreCommandLineHelper.java +++ b/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)); + } + } + } + } diff --git a/core/src/main/java/edu/wpi/grip/core/Main.java b/core/src/main/java/edu/wpi/grip/core/Main.java index 01e75a1cda..49db1267e8 100644 --- a/core/src/main/java/edu/wpi/grip/core/Main.java +++ b/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 diff --git a/core/src/main/java/edu/wpi/grip/core/Pipeline.java b/core/src/main/java/edu/wpi/grip/core/Pipeline.java index 76c3050bae..43ef293aa2 100644 --- a/core/src/main/java/edu/wpi/grip/core/Pipeline.java +++ b/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(); + } + } diff --git a/core/src/main/java/edu/wpi/grip/core/events/AppSettingsChangedEvent.java b/core/src/main/java/edu/wpi/grip/core/events/AppSettingsChangedEvent.java new file mode 100644 index 0000000000..2281842e07 --- /dev/null +++ b/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; + } + +} diff --git a/core/src/main/java/edu/wpi/grip/core/http/GripServer.java b/core/src/main/java/edu/wpi/grip/core/http/GripServer.java index e27949d78e..b959351017 100644 --- a/core/src/main/java/edu/wpi/grip/core/http/GripServer.java +++ b/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(); } } diff --git a/core/src/main/java/edu/wpi/grip/core/http/HttpPipelineSwitcher.java b/core/src/main/java/edu/wpi/grip/core/http/HttpPipelineSwitcher.java index d349427193..37d9641175 100644 --- a/core/src/main/java/edu/wpi/grip/core/http/HttpPipelineSwitcher.java +++ b/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); } } diff --git a/core/src/main/java/edu/wpi/grip/core/serialization/Project.java b/core/src/main/java/edu/wpi/grip/core/serialization/Project.java index 64812c491e..b75083a3fe 100644 --- a/core/src/main/java/edu/wpi/grip/core/serialization/Project.java +++ b/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); diff --git a/core/src/main/java/edu/wpi/grip/core/settings/AppSettings.java b/core/src/main/java/edu/wpi/grip/core/settings/AppSettings.java new file mode 100644 index 0000000000..884158f62f --- /dev/null +++ b/core/src/main/java/edu/wpi/grip/core/settings/AppSettings.java @@ -0,0 +1,48 @@ +package edu.wpi.grip.core.settings; + +import edu.wpi.grip.core.http.GripServer; + +import com.google.common.base.MoreObjects; + +import javax.annotation.Nonnegative; + +import static com.google.common.base.Preconditions.checkArgument; + +/** + * Holds settings for the GRIP app. These settings are either global settings or set via the + * command line. + */ +@SuppressWarnings("JavadocMethod") +public class AppSettings implements Settings, Cloneable { + + @Setting(label = "Internal server port", + description = "The port that the internal server should run on.") + private int serverPort = GripServer.DEFAULT_PORT; + + public int getServerPort() { + return serverPort; + } + + public void setServerPort(@Nonnegative int serverPort) { + checkArgument(GripServer.isPortValid(serverPort), + "Server port must be in the range 1024..65535"); + this.serverPort = serverPort; + } + + @Override + public String toString() { + return MoreObjects.toStringHelper(this) + .add("serverPort", serverPort) + .toString(); + } + + @Override + public AppSettings clone() { + try { + return (AppSettings) super.clone(); + } catch (CloneNotSupportedException impossible) { + throw new AssertionError(impossible); + } + } + +} diff --git a/core/src/main/java/edu/wpi/grip/core/settings/ProjectSettings.java b/core/src/main/java/edu/wpi/grip/core/settings/ProjectSettings.java index 289eb2d8da..cf78f57bfb 100644 --- a/core/src/main/java/edu/wpi/grip/core/settings/ProjectSettings.java +++ b/core/src/main/java/edu/wpi/grip/core/settings/ProjectSettings.java @@ -13,7 +13,7 @@ * numbers, which need to be preserved when deploying the project. */ @SuppressWarnings("JavadocMethod") -public class ProjectSettings implements Cloneable { +public class ProjectSettings implements Settings, Cloneable { @Setting(label = "FRC team number", description = "The team number, if used for FRC") private int teamNumber = 0; @@ -43,10 +43,6 @@ public class ProjectSettings implements Cloneable { private String deployJvmOptions = "-Xmx50m -XX:-OmitStackTraceInFastThrow " + "-XX:+HeapDumpOnOutOfMemoryError -XX:MaxNewSize=16m"; - @Setting(label = "Internal server port", - description = "The port that the internal server should run on.") - private int serverPort = 8080; - public int getTeamNumber() { return teamNumber; } @@ -64,8 +60,7 @@ public void setTeamNumber(@Nonnegative int teamNumber) { this.teamNumber = teamNumber; // If the deploy address and/or NetworkTables server address was previously the default for - // the old team - // number (ie: it was roborio-xxx-frc.local), update it with the new team number + // the old team number (ie: it was roborio-xxx-frc.local), update it with the new team number if (oldFrcAddress.equals(getDeployAddress())) { setDeployAddress(newFrcAddress); } @@ -139,16 +134,6 @@ private String computeFRCAddress(int teamNumber) { return "roboRIO-" + teamNumber + "-FRC.local"; } - public int getServerPort() { - return serverPort; - } - - public void setServerPort(@Nonnegative int serverPort) { - checkArgument(serverPort >= 1024 && serverPort <= 65535, - "Server port must be in the range 1024..65535"); - this.serverPort = serverPort; - } - @Override public String toString() { return MoreObjects.toStringHelper(this) @@ -159,7 +144,6 @@ public String toString() { .add("deployJvmOptions", deployJvmOptions) .add("publishAddress", publishAddress) .add("teamNumber", teamNumber) - .add("serverPort", serverPort) .toString(); } diff --git a/core/src/main/java/edu/wpi/grip/core/settings/Setting.java b/core/src/main/java/edu/wpi/grip/core/settings/Setting.java index e18af2aed5..acd157f745 100644 --- a/core/src/main/java/edu/wpi/grip/core/settings/Setting.java +++ b/core/src/main/java/edu/wpi/grip/core/settings/Setting.java @@ -10,8 +10,6 @@ * provide user-presentable strings for the project settings editor, which is autogenerated by * ControlsFX using the JavaBeans API. This is similar to the more general-purpose annotations in * JEP 256: http://openjdk.java.net/jeps/256 - * - * @see ProjectSettingsBeanInfo */ @Retention(RetentionPolicy.RUNTIME) @Target(ElementType.FIELD) diff --git a/core/src/main/java/edu/wpi/grip/core/settings/Settings.java b/core/src/main/java/edu/wpi/grip/core/settings/Settings.java new file mode 100644 index 0000000000..5ef99e84e5 --- /dev/null +++ b/core/src/main/java/edu/wpi/grip/core/settings/Settings.java @@ -0,0 +1,7 @@ +package edu.wpi.grip.core.settings; + +/** + * Marker interface for a settings class. + */ +public interface Settings { +} diff --git a/core/src/main/java/edu/wpi/grip/core/settings/SettingsProvider.java b/core/src/main/java/edu/wpi/grip/core/settings/SettingsProvider.java index 2874ccdabc..67b42bd8e7 100644 --- a/core/src/main/java/edu/wpi/grip/core/settings/SettingsProvider.java +++ b/core/src/main/java/edu/wpi/grip/core/settings/SettingsProvider.java @@ -14,4 +14,14 @@ public interface SettingsProvider { * @return The current per-project settings. */ ProjectSettings getProjectSettings(); + + /** + * Gets the current global app settings. This object may become out of date if the settings are + * edited by the user, so objects requiring a preference value should also subscribe to + * {@link edu.wpi.grip.core.events.AppSettingsChangedEvent} to get updates. + * + * @return the current global app settings + */ + AppSettings getAppSettings(); + } diff --git a/core/src/test/java/edu/wpi/grip/core/http/GripServerTest.java b/core/src/test/java/edu/wpi/grip/core/http/GripServerTest.java index a3101c2af3..30d3914d7b 100644 --- a/core/src/test/java/edu/wpi/grip/core/http/GripServerTest.java +++ b/core/src/test/java/edu/wpi/grip/core/http/GripServerTest.java @@ -1,7 +1,7 @@ package edu.wpi.grip.core.http; +import edu.wpi.grip.core.Pipeline; import edu.wpi.grip.core.exception.GripServerException; -import edu.wpi.grip.core.settings.ProjectSettings; import edu.wpi.grip.core.settings.SettingsProvider; import org.apache.http.HttpResponse; @@ -20,6 +20,7 @@ import java.io.ByteArrayInputStream; import java.io.IOException; import java.nio.charset.StandardCharsets; + import javax.servlet.ServletException; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; @@ -56,7 +57,7 @@ public static GripServer makeServer(ContextStore store, } public GripServerTest() { - instance = new GripServer(new ContextStore(), new TestServerFactory(), ProjectSettings::new); + instance = new GripServer(new ContextStore(), new TestServerFactory(), new Pipeline()); instance.start(); client = new DefaultHttpClient(); diff --git a/core/src/test/java/edu/wpi/grip/core/http/HttpPipelineSwitcherTest.java b/core/src/test/java/edu/wpi/grip/core/http/HttpPipelineSwitcherTest.java index fe6a00b893..e5090dd11e 100644 --- a/core/src/test/java/edu/wpi/grip/core/http/HttpPipelineSwitcherTest.java +++ b/core/src/test/java/edu/wpi/grip/core/http/HttpPipelineSwitcherTest.java @@ -1,8 +1,7 @@ package edu.wpi.grip.core.http; +import edu.wpi.grip.core.Pipeline; import edu.wpi.grip.core.serialization.Project; -import edu.wpi.grip.core.settings.ProjectSettings; -import edu.wpi.grip.core.util.GripMode; import org.apache.http.HttpResponse; import org.apache.http.client.HttpClient; @@ -33,7 +32,7 @@ public class HttpPipelineSwitcherTest { @Before public void setUp() { server = new GripServer(new ContextStore(), new GripServerTest.TestServerFactory(), - ProjectSettings::new); + new Pipeline()); client = HttpClients.createDefault(); server.start(); } @@ -46,7 +45,7 @@ public void open(String projectXml) { fail("This should not have been called"); } }; - pipelineSwitcher = new HttpPipelineSwitcher(new ContextStore(), project, GripMode.HEADLESS); + pipelineSwitcher = makePipelineSwitcher(); server.addHandler(pipelineSwitcher); HttpResponse response = doGet(GripServer.PIPELINE_UPLOAD_PATH); EntityUtils.consume(response.getEntity()); @@ -63,7 +62,7 @@ public void open(String projectXml) { assertEquals("Payload was not converted correctly", payload, projectXml); } }; - pipelineSwitcher = new HttpPipelineSwitcher(new ContextStore(), project, GripMode.HEADLESS); + pipelineSwitcher = makePipelineSwitcher(); server.addHandler(pipelineSwitcher); HttpResponse response = doPost(GripServer.PIPELINE_UPLOAD_PATH, payload); EntityUtils.consume(response.getEntity()); @@ -79,7 +78,7 @@ public void open(String projectXml) { didRun[0] = true; } }; - pipelineSwitcher = new HttpPipelineSwitcher(new ContextStore(), project, GripMode.HEADLESS); + pipelineSwitcher = makePipelineSwitcher(); server.addHandler(pipelineSwitcher); HttpResponse response = doPost(GripServer.PIPELINE_UPLOAD_PATH, "dummy data"); assertEquals("HTTP status should be 201 Created", @@ -89,23 +88,6 @@ public void open(String projectXml) { assertTrue("Project was not opened", didRun[0]); } - @Test - public void testGui() throws IOException { - Project project = new Project() { - @Override - public void open(String projectXml) { - fail("Project should not be opened"); - } - }; - pipelineSwitcher = new HttpPipelineSwitcher(new ContextStore(), project, GripMode.GUI); - server.addHandler(pipelineSwitcher); - HttpResponse response = doPost(GripServer.PIPELINE_UPLOAD_PATH, "dummy data"); - assertEquals("HTTP status should be 500 Internal Error", - 500, - response.getStatusLine().getStatusCode()); - EntityUtils.consume(response.getEntity()); - } - @After public void tearDown() { server.stop(); @@ -113,6 +95,10 @@ public void tearDown() { pipelineSwitcher.releaseContext(); } + private HttpPipelineSwitcher makePipelineSwitcher() { + return new HttpPipelineSwitcher(new ContextStore(), project); + } + private HttpResponse doGet(String path) throws IOException { String uri = "http://localhost:" + server.getPort() + path; HttpGet get = new HttpGet(uri); diff --git a/core/src/test/java/edu/wpi/grip/core/operations/network/http/HttpPublisherTest.java b/core/src/test/java/edu/wpi/grip/core/operations/network/http/HttpPublisherTest.java index af2040ec98..aa21fe60e0 100644 --- a/core/src/test/java/edu/wpi/grip/core/operations/network/http/HttpPublisherTest.java +++ b/core/src/test/java/edu/wpi/grip/core/operations/network/http/HttpPublisherTest.java @@ -1,12 +1,12 @@ package edu.wpi.grip.core.operations.network.http; +import edu.wpi.grip.core.Pipeline; import edu.wpi.grip.core.events.RunStartedEvent; import edu.wpi.grip.core.events.RunStoppedEvent; import edu.wpi.grip.core.http.ContextStore; import edu.wpi.grip.core.http.GripServer; import edu.wpi.grip.core.http.GripServerTest; import edu.wpi.grip.core.operations.network.NumberPublishable; -import edu.wpi.grip.core.settings.ProjectSettings; import edu.wpi.grip.core.sockets.InputSocket; import edu.wpi.grip.core.sockets.MockInputSocketFactory; @@ -61,7 +61,7 @@ public void setUp() { ContextStore contextStore = new ContextStore(); InputSocket.Factory isf = new MockInputSocketFactory(eventBus); server = GripServerTest.makeServer( - contextStore, new GripServerTest.TestServerFactory(), ProjectSettings::new); + contextStore, new GripServerTest.TestServerFactory(), new Pipeline()); dataHandler = new DataHandler(contextStore); eventBus.register(dataHandler); diff --git a/core/src/test/java/edu/wpi/grip/core/sources/HttpSourceTest.java b/core/src/test/java/edu/wpi/grip/core/sources/HttpSourceTest.java index e4bfbac59f..3f5553d187 100644 --- a/core/src/test/java/edu/wpi/grip/core/sources/HttpSourceTest.java +++ b/core/src/test/java/edu/wpi/grip/core/sources/HttpSourceTest.java @@ -1,9 +1,9 @@ package edu.wpi.grip.core.sources; +import edu.wpi.grip.core.Pipeline; import edu.wpi.grip.core.http.ContextStore; import edu.wpi.grip.core.http.GripServer; import edu.wpi.grip.core.http.GripServerTest; -import edu.wpi.grip.core.settings.ProjectSettings; import edu.wpi.grip.core.sockets.MockOutputSocketFactory; import edu.wpi.grip.core.sockets.OutputSocket; import edu.wpi.grip.core.util.MockExceptionWitness; @@ -45,7 +45,7 @@ public class HttpSourceTest { public void setUp() throws URIException, URISyntaxException { GripServer.JettyServerFactory f = new GripServerTest.TestServerFactory(); ContextStore contextStore = new ContextStore(); - server = GripServerTest.makeServer(contextStore, f, ProjectSettings::new); + server = GripServerTest.makeServer(contextStore, f, new Pipeline()); server.start(); EventBus eventBus = new EventBus(); OutputSocket.Factory osf = new MockOutputSocketFactory(eventBus); diff --git a/core/src/test/java/edu/wpi/grip/core/sources/SourcesSanityTest.java b/core/src/test/java/edu/wpi/grip/core/sources/SourcesSanityTest.java index 4b2ee61ca2..e4381c1542 100644 --- a/core/src/test/java/edu/wpi/grip/core/sources/SourcesSanityTest.java +++ b/core/src/test/java/edu/wpi/grip/core/sources/SourcesSanityTest.java @@ -1,10 +1,10 @@ package edu.wpi.grip.core.sources; +import edu.wpi.grip.core.Pipeline; import edu.wpi.grip.core.http.ContextStore; import edu.wpi.grip.core.http.GripServer; import edu.wpi.grip.core.http.GripServerTest; -import edu.wpi.grip.core.settings.ProjectSettings; import edu.wpi.grip.core.util.ExceptionWitness; import edu.wpi.grip.core.util.MockExceptionWitness; import edu.wpi.grip.core.util.service.SingleActionListener; @@ -26,10 +26,8 @@ public SourcesSanityTest() { setDefault(ExceptionWitness.Factory.class, MockExceptionWitness.MOCK_FACTORY); GripServer.JettyServerFactory serverFactory = new GripServerTest.TestServerFactory(); - ProjectSettings projectSettings = new ProjectSettings(); - projectSettings.setServerPort(8080); GripServer server = - GripServerTest.makeServer(new ContextStore(), serverFactory, () -> projectSettings); + GripServerTest.makeServer(new ContextStore(), serverFactory, new Pipeline()); setDefault(GripServer.class, server); } } diff --git a/ui/src/main/java/edu/wpi/grip/core/settings/AppSettingsBeanInfo.java b/ui/src/main/java/edu/wpi/grip/core/settings/AppSettingsBeanInfo.java new file mode 100644 index 0000000000..316c769dcd --- /dev/null +++ b/ui/src/main/java/edu/wpi/grip/core/settings/AppSettingsBeanInfo.java @@ -0,0 +1,18 @@ +package edu.wpi.grip.core.settings; + +/** + * BeanInfo class for {@link AppSettings}. This inspects annotations on the properties in + * AppSettings to produce PropertyDescriptors with proper display names and descriptions. + * ControlsFX's PropertySheet control uses JavaBean properties to generate the settings editor, so + * we need this class in order to make the properties have user-presentable names and descriptions. + * Another way to do this without annotations would be to hardcode a bunch of PropertyDescriptors + * here, but that would be error-prone (we would get no warning if we add a new setting and forget + * to add a descriptor here). + */ +public class AppSettingsBeanInfo extends SimpleSettingsBeanInfo { + + public AppSettingsBeanInfo() { + super(AppSettings.class); + } + +} diff --git a/ui/src/main/java/edu/wpi/grip/core/settings/ProjectSettingsBeanInfo.java b/ui/src/main/java/edu/wpi/grip/core/settings/ProjectSettingsBeanInfo.java new file mode 100644 index 0000000000..9c2d6db29c --- /dev/null +++ b/ui/src/main/java/edu/wpi/grip/core/settings/ProjectSettingsBeanInfo.java @@ -0,0 +1,18 @@ +package edu.wpi.grip.core.settings; + +/** + * BeanInfo class for {@link ProjectSettings}. This inspects annotations on the properties in + * ProjectSettings to produce PropertyDescriptors with proper display names and descriptions. + * ControlsFX's PropertySheet control uses JavaBean properties to generate the settings editor, so + * we need this class in order to make the properties have user-presentable names and descriptions. + * Another way to do this without annotations would be to hardcode a bunch of PropertyDescriptors + * here, but that would be error-prone (we would get no warning if we add a new setting and forget + * to add a descriptor here). + */ +public class ProjectSettingsBeanInfo extends SimpleSettingsBeanInfo { + + public ProjectSettingsBeanInfo() { + super(ProjectSettings.class); + } + +} diff --git a/core/src/main/java/edu/wpi/grip/core/settings/ProjectSettingsBeanInfo.java b/ui/src/main/java/edu/wpi/grip/core/settings/SimpleSettingsBeanInfo.java similarity index 51% rename from core/src/main/java/edu/wpi/grip/core/settings/ProjectSettingsBeanInfo.java rename to ui/src/main/java/edu/wpi/grip/core/settings/SimpleSettingsBeanInfo.java index af4f5b1008..bc3a8ccd70 100644 --- a/core/src/main/java/edu/wpi/grip/core/settings/ProjectSettingsBeanInfo.java +++ b/ui/src/main/java/edu/wpi/grip/core/settings/SimpleSettingsBeanInfo.java @@ -11,37 +11,42 @@ import java.util.ArrayList; import java.util.List; +import static com.google.common.base.Preconditions.checkNotNull; + /** - * BeanInfo class for {@link ProjectSettings}. This inspects annotations on the properties in - * ProjectSettings to produce PropertyDescriptors with proper display names and descriptions. - * ControlsFX's PropertySheet control uses JavaBean properties to generate the settings editor, so - * we need this class in order to make the properties have user-presentable names and descriptions. - * Another way to do this without annotations would be to hardcode a bunch of PropertyDescriptors - * here, but that would be error-prone (we would get no warning if we add a new setting and forget - * to add a descriptor here). This class is never run in headless mode, so the nonexistance of the - * JavaBeans API and the slowness of reflection on the roboRIO is not an issue. + * Simple bean info for a settings class. The settings class should subscribe to the javabean + * standard. */ -public class ProjectSettingsBeanInfo extends SimpleBeanInfo { +public class SimpleSettingsBeanInfo extends SimpleBeanInfo { - private final Converter caseConverter = + private static final Converter caseConverter = CaseFormat.LOWER_CAMEL.converterTo(CaseFormat.UPPER_CAMEL); + private final Class beanClass; + + public SimpleSettingsBeanInfo(Class beanClass) { + this.beanClass = checkNotNull(beanClass, "beanClass"); + } + @Override public PropertyDescriptor[] getPropertyDescriptors() { - Field[] fields = ProjectSettings.class.getDeclaredFields(); + Field[] fields = beanClass.getDeclaredFields(); try { - // For each property in ProjectSettings with a @Setting annotation, create a descriptor - // with a display name - // and description included. + // For each property in the bean with a @Setting annotation, create a descriptor + // with a display name and description included. List propertyDescriptors = new ArrayList<>(); for (Field field : fields) { String property = field.getName(); Setting setting = field.getAnnotation(Setting.class); if (setting != null) { - PropertyDescriptor descriptor = new PropertyDescriptor(property, ProjectSettings.class, - "get" + caseConverter.convert(property), "set" + caseConverter.convert(property)); + PropertyDescriptor descriptor = new PropertyDescriptor( + property, + beanClass, + "get" + caseConverter.convert(property), + "set" + caseConverter.convert(property) + ); descriptor.setDisplayName(setting.label()); descriptor.setShortDescription(setting.description()); propertyDescriptors.add(descriptor); @@ -54,5 +59,6 @@ public PropertyDescriptor[] getPropertyDescriptors() { // constructor throw Throwables.propagate(ex); } + } } diff --git a/ui/src/main/java/edu/wpi/grip/ui/Main.java b/ui/src/main/java/edu/wpi/grip/ui/Main.java index df42ae3a49..a37cc390f6 100644 --- a/ui/src/main/java/edu/wpi/grip/ui/Main.java +++ b/ui/src/main/java/edu/wpi/grip/ui/Main.java @@ -1,6 +1,5 @@ package edu.wpi.grip.ui; -import edu.wpi.grip.core.CoreCommandLineHelper; import edu.wpi.grip.core.GripCoreModule; import edu.wpi.grip.core.GripFileModule; import edu.wpi.grip.core.PipelineRunner; @@ -11,6 +10,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 edu.wpi.grip.core.util.SafeShutdown; import edu.wpi.grip.ui.util.DPIUtility; @@ -25,7 +25,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; @@ -58,12 +57,14 @@ public class Main extends Application { @Inject private EventBus eventBus; @Inject private PipelineRunner pipelineRunner; @Inject private Project project; + @Inject private SettingsProvider settingsProvider; @Inject private Operations operations; @Inject private CVOperations cvOperations; @Inject private GripServer server; @Inject private HttpPipelineSwitcher pipelineSwitcher; private Parent root; private boolean headless; + private final UICommandLineHelper commandLineHelper = new UICommandLineHelper(); private CommandLine parsedArgs; public static void main(String[] args) { @@ -72,7 +73,6 @@ public static void main(String[] args) { @Override public void init() throws IOException { - UICommandLineHelper commandLineHelper = new UICommandLineHelper(); parsedArgs = commandLineHelper.parse(getParameters().getRaw()); if (parsedArgs.hasOption(UICommandLineHelper.HEADLESS_OPTION)) { @@ -138,33 +138,8 @@ public void start(Stage stage) throws IOException { operations.addOperations(); cvOperations.addOperations(); - // If there was a file specified on the command line, open it immediately - if (parsedArgs.hasOption(CoreCommandLineHelper.FILE_OPTION)) { - String file = parsedArgs.getOptionValue(CoreCommandLineHelper.FILE_OPTION); - try { - project.open(new File(file)); - } catch (IOException e) { - logger.log(Level.SEVERE, "Error loading file: " + file); - throw e; - } - } - - // Set the port AFTER loading the project to override the setting in the file - 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); - server.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 diff --git a/ui/src/main/java/edu/wpi/grip/ui/MainWindowController.java b/ui/src/main/java/edu/wpi/grip/ui/MainWindowController.java index 3b00668f33..5b7b77f393 100644 --- a/ui/src/main/java/edu/wpi/grip/ui/MainWindowController.java +++ b/ui/src/main/java/edu/wpi/grip/ui/MainWindowController.java @@ -3,11 +3,13 @@ import edu.wpi.grip.core.Palette; import edu.wpi.grip.core.Pipeline; import edu.wpi.grip.core.PipelineRunner; +import edu.wpi.grip.core.events.AppSettingsChangedEvent; import edu.wpi.grip.core.events.BenchmarkEvent; import edu.wpi.grip.core.events.ProjectSettingsChangedEvent; import edu.wpi.grip.core.events.TimerEvent; import edu.wpi.grip.core.events.UnexpectedThrowableEvent; import edu.wpi.grip.core.serialization.Project; +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.util.SafeShutdown; @@ -235,11 +237,14 @@ public boolean saveProjectAs() throws IOException { @FXML protected void showProjectSettingsEditor() { final ProjectSettings projectSettings = settingsProvider.getProjectSettings().clone(); + final AppSettings appSettings = settingsProvider.getAppSettings().clone(); - ProjectSettingsEditor projectSettingsEditor = new ProjectSettingsEditor(root, projectSettings); + ProjectSettingsEditor projectSettingsEditor + = new ProjectSettingsEditor(root, projectSettings, appSettings); projectSettingsEditor.showAndWait().ifPresent(buttonType -> { if (buttonType == ButtonType.OK) { eventBus.post(new ProjectSettingsChangedEvent(projectSettings)); + eventBus.post(new AppSettingsChangedEvent(appSettings)); } }); } diff --git a/ui/src/main/java/edu/wpi/grip/ui/ProjectSettingsEditor.java b/ui/src/main/java/edu/wpi/grip/ui/ProjectSettingsEditor.java index 303e476597..9e099079db 100644 --- a/ui/src/main/java/edu/wpi/grip/ui/ProjectSettingsEditor.java +++ b/ui/src/main/java/edu/wpi/grip/ui/ProjectSettingsEditor.java @@ -1,36 +1,52 @@ package edu.wpi.grip.ui; +import edu.wpi.grip.core.settings.AppSettings; import edu.wpi.grip.core.settings.ProjectSettings; import edu.wpi.grip.ui.util.DPIUtility; import org.controlsfx.control.PropertySheet; import org.controlsfx.property.BeanPropertyUtils; +import javafx.collections.ObservableList; import javafx.scene.Parent; import javafx.scene.control.ButtonType; import javafx.scene.control.Dialog; import javafx.scene.control.DialogPane; +import javafx.scene.control.Separator; import javafx.scene.image.Image; import javafx.scene.image.ImageView; +import javafx.scene.layout.VBox; /** * A JavaFX dialog that lets the user edit the project {@link ProjectSettings}. */ public class ProjectSettingsEditor extends Dialog { + private static class CustomPropertySheet extends PropertySheet { + public CustomPropertySheet(ObservableList items) { + super(items); + setMode(Mode.NAME); + setModeSwitcherVisible(false); + setSearchBoxVisible(false); + } + } + @SuppressWarnings("JavadocMethod") - public ProjectSettingsEditor(Parent root, ProjectSettings projectSettings) { + public ProjectSettingsEditor(Parent root, + ProjectSettings projectSettings, + AppSettings appSettings) { super(); - PropertySheet propertySheet = new PropertySheet(BeanPropertyUtils - .getProperties(projectSettings)); - propertySheet.setMode(PropertySheet.Mode.NAME); - propertySheet.setModeSwitcherVisible(false); - propertySheet.setSearchBoxVisible(false); + VBox content = new VBox( + new CustomPropertySheet(BeanPropertyUtils.getProperties(projectSettings)), + new Separator(), + new CustomPropertySheet(BeanPropertyUtils.getProperties(appSettings)) + ); + content.setSpacing(5.0); DialogPane pane = getDialogPane(); pane.getButtonTypes().setAll(ButtonType.OK, ButtonType.CANCEL); - pane.setContent(propertySheet); + pane.setContent(content); pane.styleProperty().bind(root.styleProperty()); pane.getStylesheets().addAll(root.getStylesheets()); pane.setPrefSize(DPIUtility.SETTINGS_DIALOG_SIZE, DPIUtility.SETTINGS_DIALOG_SIZE);