Fix the deploy UI #386
7 participants
Current coverage is
|
|
When do you plan to release the latest version? |
|
I'm thinking sometime this week. If depends on when all of these PRs get finished and merged |
|
If this is a more general deploy menu, should it be labeled with the FIRST logo? |
|
Maybe add a button that allows users to default to FRC defaults |
Good point, maybe a generic "play" icon would be better
It actually fills in FRC defaults automatically |
|
I agree with @AustinShalit on this one. |
Which one? The icon? |
|
Yea, the genericizing the icon and the menu. |
|
I don't know if you care, but currently your commit message won't automatically close those issues when this is merged. |
|
They're already closed except #377. Do I need to add it to a commit message to close it? |
JLLeitschuh
and 1 other
commented on an outdated diff
| - dialog.showAndWait(); | ||
| - } else { | ||
| - final Alert alert = new Alert(Alert.AlertType.INFORMATION, | ||
| - "You must have saved your project before it can be deployed to a remote device."); | ||
| - alert.getDialogPane().getStylesheets().addAll(root.getStylesheets()); | ||
| - alert.getDialogPane().setStyle(root.getStyle()); | ||
| - | ||
| - // Workaround for JavaFX's default Alert styling causing text to get cut off | ||
| - Label alertLabel = (Label) alert.getDialogPane().getChildren().filtered(node -> node instanceof Label).get(0); | ||
| - alertLabel.setMinHeight(Region.USE_PREF_SIZE); | ||
| - | ||
| - alert.showAndWait(); | ||
| - } | ||
| - | ||
| + public void deploy() { | ||
| + ImageView graphic = new ImageView(new Image("/edu/wpi/grip/ui/icons/first.png")); |
JLLeitschuh
Let's make this more generic since we don't just want to upload for FRC. |
| + */ | ||
| +@ParametrizedController(url = "Deploy.fxml") | ||
| +public class DeployController { | ||
| + | ||
| + // Default deploy information for FRC. This can be overridden by the use for applications outside of FRC or not | ||
| + // using the roboRIO (ie: coprocessors) | ||
| + public final static String DEFAULT_USER = "lvuser"; | ||
| + private final static String DEFAULT_PASSWORD = ""; | ||
| + private final static String DEFAULT_JAVA_HOME = "/usr/local/frc/JRE/"; | ||
| + private final static String DEFAULT_DIR = "/home/" + DEFAULT_USER; | ||
| + private final static String GRIP_JAR = "grip.jar"; | ||
| + private final static String PROJECT_FILE = "project.grip"; | ||
| + | ||
| + @FXML private TextField address; | ||
| + @FXML private TextField user; | ||
| + @FXML private TextField password; |
|
JLLeitschuh
Use a password field for this. |
|
ThomasJClark |
ade556e
|
|||
|
|
ThomasJClark |
7199ec6
|
|||
|
|
ThomasJClark |
Some checks were not successful |
778cbc4
|
||
|
|
ThomasJClark |
Some checks were not successful |
71668e7
|
||
|
|
ThomasJClark |
c68b22b
|
|||
|
|
ThomasJClark |
Some checks were not successful |
370f099
|
||
|
|
ThomasJClark |
Some checks were not successful |
bcea46a
|
| + private final static String PROJECT_FILE = "project.grip"; | ||
| + | ||
| + @FXML private TextField address; | ||
| + @FXML private TextField user; | ||
| + @FXML private TextField password; | ||
| + @FXML private TextField javaHome; | ||
| + @FXML private TextField deployDir; | ||
| + @FXML private ProgressIndicator progress; | ||
| + @FXML private Button deployButton; | ||
| + @FXML private Label status; | ||
| + @FXML private TextArea console; | ||
| + | ||
| + @Inject private EventBus eventBus; | ||
| + @Inject private Project project; | ||
| + @Inject private Pipeline pipeline; | ||
| + @Inject private Logger logger; |
ThomasJClark
Both ways are used in the project, and we have no officially documented style guide. I've mostly been going by the Google Java Style for now:
This is consistent with the project so far, and I think is much more readable than this: @FXML
private TextField address;
@FXML
private TextField user;
@FXML
private TextField password;
@FXML
private TextField javaHome;
@FXML
private TextField deployDir;
@FXML
private ProgressIndicator progress;
@FXML
private Button deployButton;
@FXML
private Label status;
@FXML
private TextArea console;
@Inject
private EventBus eventBus;
@Inject
private Project project;
@Inject
private Pipeline pipeline;
@Inject
private Logger logger;Of course, we should keep that "may" in mind. In particular,
JLLeitschuh
We haven't really made an official decision on styling. We decided originally to use the default for Intellij. If you want to change it then we need to talk about it as a team.
ThomasJClark
Like I said, this is true.
Like both of us said, this isn't true. Although I have recommended using IntelliJ's autoformatter, we never said our official style guide was the default settings in IntelliJ, and in practice our codebase more closely matches Google Java Style.
I'm not changing anything. This code does not violate any official style guidelines we have, or any written but unofficial ones, and it's consistent with our current code, and it's subjectively more readable. If we want to establish an official style guide, I would think that would involve meeting as a team (although really I don't see why anyone would object to officially adopting Google's style) |
JLLeitschuh
and 1 other
commented on an outdated diff
| + @Setting(label = "Default deploy directory", description = "The directory on the remote host to deploy GRIP to.") | ||
| + private String deployDir = "/home/lvuser"; | ||
| + | ||
| + @Setting(label = "Default deploy user", description = "The username to log in with when deploying over SSH.") | ||
| + private String deployUser = "lvuser"; | ||
| + | ||
| + @Setting(label = "Default deploy java home", description = "Where Java is installed on the robot.") | ||
| + private String deployJavaHome = "/usr/local/frc/JRE/"; |
|
JLLeitschuh
I don't like that this is specific to FRC by default.
ThomasJClark
FRC teams are really our primary audience, especially for the deploy tool. I think we'll have lots of issues/angry CD posts if we don't just make it work when you hit "Deploy". IMO researchers/hobbyists/college students can probably figure out pretty quickly how to configure the deploy tool, if they use it at all. Maybe eventually we can have a project wizard, and one of the templates you can select would be FRC |
| + | ||
| +<?import edu.wpi.grip.ui.util.DPIUtility?> | ||
| +<?import edu.wpi.grip.ui.DeployController?> | ||
| +<?import javafx.scene.control.*?> | ||
| +<?import javafx.scene.image.Image?> | ||
| +<?import javafx.scene.image.ImageView?> | ||
| +<?import javafx.scene.layout.*?> | ||
| +<VBox styleClass="deploy-pane" maxWidth="Infinity" xmlns:fx="http://javafx.com/fxml/1" | ||
| + xmlns="http://javafx.com/javafx/null" fx:controller="edu.wpi.grip.ui.DeployController"> | ||
| + <GridPane maxHeight="Infinity"> | ||
| + <columnConstraints> | ||
| + <ColumnConstraints hgrow="NEVER"/> | ||
| + <ColumnConstraints hgrow="ALWAYS"/> | ||
| + </columnConstraints> | ||
| + | ||
| + <Label disable="${deployButton.disabled}" text="Address" GridPane.columnIndex="0" GridPane.rowIndex="0"/> |
|
JLLeitschuh
For all labels make sure you set what the label is a label for. |
| + SCPFileTransfer scp = ssh.newSCPFileTransfer(); | ||
| + scp.setTransferListener(new LoggingTransferListener() { | ||
| + @Override | ||
| + public StreamCopier.Listener file(String name, long size) { | ||
| + setStatusAsync("Uploading " + name, false); | ||
| + return transferred -> { | ||
| + if (isNotCanceled()) | ||
| + Platform.runLater(() -> progress.setProgress((double) transferred / size)); | ||
| + }; | ||
| + } | ||
| + }); | ||
| + | ||
| + // The project may or may not be saved to a file (and even if it was saved, it might be modified), so we | ||
| + // serialize it to a string before deploying. | ||
| + StringWriter projectWriter = new StringWriter(); | ||
| + project.save(projectWriter); |
|
JLLeitschuh
Shouldn't we just ask if they want to save before they open the deploy UI?
ThomasJClark
Why? What if you've made some changes that you want to test before saving?
JLLeitschuh
Sure. Thats fine. But shouldn't you offer them the option of saving before deploying.
ThomasJClark
Why? That might mislead people into thinking you have to save to deploy, and I think it would get annoying after a while if you're not trying to save |
|
Closing the deploy UI doesn't stop the upload attempt. |
That's correct. I think people want to be able to work on their pipeline while they're running a test on the roboRIO to adjust things if it needs improvement. That's why I added separate "Stop" and "Close" buttons. |
|
The new deploy interface looks awesome and am especially happy about the new method of seeing errors great work. As for the whole prompt for saving as a user I just want to be able to quickly alter and test different values without overwriting a previous working file for this getting promoted to save all the time will eventually get annoying. When something works better I'll either save it as a seperate file or overwrite the existing one manually. |
|
Moving chat to here so it doesn't get deleted by updating the files:
.
I think we should have different deploy configurations. For example, deploying to a PI may require more configuration than to an FRC device. |
|
I'm with @ThomasJClark on this one—right now non-FRC systems are the exception, especially when it comes to deployment. I don't even really see it as a very useful option in many other contexts tbqh. maybe the right thing to do would be to provide a FRC-specific build that makes a lot of these things the default? |
|
|
ThomasJClark |
7dc3d72
|
|||
|
|
ThomasJClark |
1820184
|
|||
|
|
ThomasJClark |
Some checks were not successful |
a4c6ffb
|
||
|
|
ThomasJClark |
Some checks were not successful |
f287e4f
|
A lot of users seem to have problems using the deploy UI. This PR should hopefully address those problems, and help us get information about future ones.
Note: all of the text fields below are automatically filled in, so users don't have to know what to use, but they can change it if they want.
New error messages
No connection to the roboRIO
Wrong username/password (if manually overridden)
Java not installed (or in the wrong place if manually overridden)
Progress bars
Indeterminate (ex: waiting for connections)
Determinate (ex: uploading files)
Running
#383 needs to get merged before master will run on a roboRIO again, but this is what it looks like when it starts to run.
Misc. Improvements
TODO
Stop the pipeline when GRIP is running headlessly✓Add a button to stop GRIP✓Possible save username, directories, etc... to project settings?✓Closes #352
Closes #367
Closes #375
Closes #376
Closes #377
Closes #388
See also: