Added better port validation (#58)

* Remade PreferencesDialog to look better

* Remade PreferencesDialog to look better

* Revert "Remade PreferencesDialog to look better"

This reverts commit ae384ae

* Redid PreferencesDialog to look better and added a default port switch.
#46

* Working on port validation
#50

* Added test error msg
#50

* Using ValidationSupport instead
#50

* Disable buttons when port is invalid
#50

* Happy with how the text field looks
#50

* Added test for disabled buttons
#50

* Added test for general invalid ports
#50

* Removed old comments
#50

* Added more rigorous port validation testing
#50

* Made Austin happy
#50

* Fixed merge

* Increase codecov

Author: Octogonapus

src/main/java/edu/wpi/first/outlineviewer/controller/PreferencesController.java

@@ -1,17 +1,24 @@
 package edu.wpi.first.outlineviewer.controller;
 
+import com.google.common.primitives.Ints;
 import edu.wpi.first.networktables.NetworkTableInstance;
 import edu.wpi.first.outlineviewer.Preferences;
+import java.util.Optional;
 import javafx.application.Platform;
+import javafx.beans.property.BooleanProperty;
+import javafx.beans.property.SimpleBooleanProperty;
 import javafx.fxml.FXML;
 import javafx.scene.control.TextField;
 import org.controlsfx.control.ToggleSwitch;
+import org.controlsfx.validation.Severity;
+import org.controlsfx.validation.ValidationResult;
+import org.controlsfx.validation.ValidationSupport;
+import org.controlsfx.validation.decoration.StyleClassValidationDecoration;
 
 /**
  * Controller for the app preferences pane.
  */
 public class PreferencesController {
-
   @FXML
   private ToggleSwitch serverModeSwitch;
   @FXML
@@ -21,6 +28,12 @@ public class PreferencesController {
   @FXML
   private TextField portField;
 
+  private final BooleanProperty invalidPortProperty;
+
+  public PreferencesController() {
+    invalidPortProperty = new SimpleBooleanProperty(false);
+  }
+
   @FXML
   private void initialize() {
     idField.disableProperty().bind(serverModeSwitch.selectedProperty());
@@ -33,6 +46,36 @@ private void initialize() {
 
     //When the user selects default port from non-default port we need to update the port number
     //and port text field to the default port number
+
+    defaultPortSwitch.selectedProperty().addListener((observable, newValue, oldValue) -> {
+      if (defaultPortSwitch.selectedProperty().get()) {
+        Preferences.setPort(NetworkTableInstance.kDefaultPort);
+        portField.setText(String.valueOf(Preferences.getPort()));
+      }
+    });
+
+    //Display error message when the current port number is invalid
+    ValidationSupport validator = new ValidationSupport();
+    validator.setValidationDecorator(
+        new StyleClassValidationDecoration("text-field-error",
+            "text-field-warning"));
+    validator.registerValidator(portField, false, (control, value) -> {
+      if (value instanceof String) {
+        return ValidationResult.fromMessageIf(control,
+            "Invalid port number",
+            Severity.ERROR,
+            !validatePortNumber((String) value).isPresent());
+      }
+
+      return ValidationResult.fromMessageIf(control,
+          "Invalid port number",
+          Severity.ERROR,
+          false);
+    });
+
+    //Link the outward-facing valid port property to the validation code
+    invalidPortProperty.bind(validator.invalidProperty());
+
     defaultPortSwitch.selectedProperty().addListener((observable, oldValue, newValue) -> {
       if (defaultPortSwitch.selectedProperty().get()) {
         portField.setText(String.valueOf(NetworkTableInstance.kDefaultPort));
@@ -51,8 +94,10 @@ private void initialize() {
    * Starts running ntcore in the selected mode.
    */
   public void save() {
-    if (portField.getText().matches("[0-9]+")) {
-      Preferences.setPort(Integer.parseInt(portField.getText()));
+    Optional<Integer> portNum = validatePortNumber(portField.getText());
+
+    if (portNum.isPresent()) {
+      Preferences.setPort(portNum.get());
     } else {
       Preferences.setPort(NetworkTableInstance.kDefaultPort);
     }
@@ -67,4 +112,22 @@ public void save() {
     Preferences.setServer(serverModeSwitch.isSelected());
   }
 
+  private Optional<Integer> validatePortNumber(String rawPortNumber) {
+    Optional<Integer> portNum = Optional.empty();
+
+    Integer val = Ints.tryParse(rawPortNumber);
+    if (val != null && val > 0 && val <= 65535) {
+      portNum = Optional.of(val);
+    }
+
+    return portNum;
+  }
+
+  public boolean isInvalidPort() {
+    return invalidPortProperty.get();
+  }
+
+  public BooleanProperty invalidPortProperty() {
+    return invalidPortProperty;
+  }
 }

src/main/java/edu/wpi/first/outlineviewer/view/dialog/PreferencesDialog.java

@@ -6,6 +6,7 @@
 import javafx.scene.control.Dialog;
 
 import java.io.IOException;
+import java.util.Arrays;
 
 /**
  * A dialog for changing the app preferences.
@@ -29,6 +30,13 @@ public PreferencesDialog(ButtonType... buttonTypes) throws IOException {
     getDialogPane().getButtonTypes().addAll(buttonTypes);
     setResultConverter(buttonType -> !buttonType.getButtonData().isCancelButton());
     controller = loader.getController();
+
+    //Bind the disabled property of each button to the valid port property of the controller
+    //so user can't save with an invalid port
+    Arrays.stream(buttonTypes)
+        .filter(btn -> btn.getButtonData().isDefaultButton())
+        .map(btn -> getDialogPane().lookupButton(btn))
+        .forEach(btn -> btn.disableProperty().bind(controller.invalidPortProperty()));
   }
 
   public PreferencesController getController() {

src/main/resources/edu/wpi/first/outlineviewer/styles.css

@@ -49,3 +49,14 @@
 .list-cell:even:selected, .list-cell:odd:selected {
     -fx-background-color: #2196F3;
 }
+
+.text-field-error {
+    -fx-text-box-border: rgba(255, 0, 0, 0.6);
+    -fx-focus-color: rgba(255, 0, 0, 0.6);
+    -fx-faint-focus-color: transparent;
+}
+
+.text-field-warning {
+    -fx-text-box-border: yellow;
+    -fx-focus-color: yellow;
+}

src/main/resources/edu/wpi/first/outlineviewer/view/dialog/PreferencesDialog.fxml

@@ -8,8 +8,9 @@
 <?import javafx.scene.layout.Pane?>
 <?import javafx.scene.layout.VBox?>
 <?import org.controlsfx.control.ToggleSwitch?>
+<?import java.net.URL?>
 
-<AnchorPane xmlns="http://javafx.com/javafx/8.0.111" xmlns:fx="http://javafx.com/fxml/1" fx:id="preferencesPaneRoot" fx:controller="edu.wpi.first.outlineviewer.controller.PreferencesController">
+<AnchorPane xmlns="http://javafx.com/javafx/8.0.111" xmlns:fx="http://javafx.com/fxml/1" id="preferencesPaneRoot" fx:id="preferencesPaneRoot" fx:controller="edu.wpi.first.outlineviewer.controller.PreferencesController">
    <children>
       <VBox layoutX="10.0" layoutY="10.0" spacing="10.0" AnchorPane.bottomAnchor="10.0" AnchorPane.leftAnchor="10.0" AnchorPane.rightAnchor="10.0" AnchorPane.topAnchor="10.0">
          <children>
@@ -48,4 +49,7 @@
          </children>
       </VBox>
    </children>
+    <stylesheets>
+        <URL value="@../../styles.css"/>
+    </stylesheets>
 </AnchorPane>

src/test/java/edu/wpi/first/outlineviewer/controller/PreferencesControllerTest.java

@@ -1,24 +1,24 @@
 package edu.wpi.first.outlineviewer.controller;
 
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
 import edu.wpi.first.networktables.NetworkTableInstance;
 import edu.wpi.first.outlineviewer.FxHelper;
 import edu.wpi.first.outlineviewer.NetworkTableUtilities;
 import edu.wpi.first.outlineviewer.Preferences;
 import edu.wpi.first.outlineviewer.view.dialog.PreferencesDialog;
-import javafx.fxml.FXMLLoader;
-import javafx.scene.Scene;
+import javafx.scene.control.ButtonType;
 import javafx.scene.control.TextField;
-import javafx.scene.layout.Pane;
 import javafx.stage.Stage;
 import org.controlsfx.control.ToggleSwitch;
-import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.CsvSource;
 import org.testfx.framework.junit5.ApplicationTest;
 
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertTrue;
-
 public class PreferencesControllerTest extends ApplicationTest {
 
   private PreferencesController controller;
@@ -27,11 +27,9 @@ public class PreferencesControllerTest extends ApplicationTest {
   public void start(Stage stage) throws Exception {
     Preferences.reset();
 
-    FXMLLoader loader
-        = new FXMLLoader(PreferencesDialog.class.getResource("PreferencesDialog.fxml"));
-    Pane pane = loader.load();
-    controller = loader.getController();
-    stage.setScene(new Scene(pane));
+    PreferencesDialog dialog = new PreferencesDialog(ButtonType.CANCEL, ButtonType.OK);
+    controller = dialog.getController();
+    stage.setScene(dialog.getDialogPane().getScene());
     stage.show();
   }
 
@@ -75,6 +73,26 @@ void testIdEmpty() {
     assertEquals("localhost", Preferences.getIp());
   }
 
+  @ParameterizedTest
+  @CsvSource({"true, 0", "false, 65535", "true, 65536", "true, -1", "true, +1"})
+  void testPortValidationTextField(boolean result, String port) {
+    FxHelper.runAndWait(() -> {
+      ((ToggleSwitch) lookup("#defaultPortSwitch").query()).setSelected(false);
+      ((TextField) lookup("#portField").query()).setText(port);
+    });
+    assertEquals(result, controller.isInvalidPort());
+  }
+
+  @ParameterizedTest
+  @CsvSource({"true, 0", "false, 65535", "true, 65536", "true, -1", "true, +1"})
+  void testPortValidationButton(boolean result, String port) {
+    FxHelper.runAndWait(() -> {
+      ((ToggleSwitch) lookup("#defaultPortSwitch").query()).setSelected(false);
+      ((TextField) lookup("#portField").query()).setText(port);
+    });
+    assertEquals(result, lookup("OK").query().isDisabled());
+  }
+
   @Test
   void testDefaultPortButton() {
     FxHelper.runAndWait(() -> {