New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allows users to specify multiple CVR files at once in the GUI #617
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool! Just some minor things...
* Example: C:\Users\test data\precinct14.json | ||
* Example: /test data/2015-portland-mayor-cvr.xlsx | ||
* Example: C:\test data\2015-portland-mayor-cvr.xlsx | ||
NOTE: You can select multiple files by holding down Shift in the dialog. You can also manually add multiple if you enter the values in the Path input box, separated with semicolons: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This guidance is Windows-specific. On macOS, you can select a set of consecutive files by Shift-clicking, or you can select two or more non-adjacent files by Command-clicking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated this, thanks!
} | ||
}); | ||
// If any entries failed validation, preserve them in the text box so the user can try again | ||
textFieldCvrFilePath.setText(String.join(CVR_FILE_PATH_DELIMITER, failedFilePaths)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this standard behavior elsewhere, or just something you thought of on your own? I can see how it might be useful, but it may be confusing if we don't tell the user explicitly what just happened (x files added successfully; y invalid files remain).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's standard to leave the values they previously entered in place if validation failed (I believe this was specifically request a while back).
In practice, we should never have a situation where something is invalid for only one of the items in the list (since they're all going to have the same properties; the one exception is if you do something like ;;;;;;
between other valid entries), but even if that were the case, the logging console should make it clear below what happened.
Essentially what this ends up doing is preserving everything in the Path input if validation fails for one or more of the properties (like a missing first vote column value), which I think is a good and intuitive thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I just think it might be more intuitive to make the code explicitly all or none. If somehow we successfully added some but not all, that would be kind of weird.
@@ -96,6 +98,7 @@ public class GuiConfigController implements Initializable { | |||
private static final String HINTS_VOTER_ERROR_RULES_FILENAME = | |||
"network/brightspots/rcv/hints_voter_error_rules.txt"; | |||
private static final String HINTS_OUTPUT_FILENAME = "network/brightspots/rcv/hints_output.txt"; | |||
private static final String CVR_FILE_PATH_DELIMITER = ";"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On macOS/Linux, it's possible to have a semicolon in a filename. :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same in Windows, actually, I just chose something that I thought was unlikely. It seems the only illegal character for *nix is /, which is obviously not viable to use as a delimiter. I changed to to ;;
to make it less likely to interfere with a legit file path.
…tions around multiple CVR paths.
Fixes #589 and #592.