Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Redesigns handling of nonIntegerWinningThreshold and hareQuota in the GUI #512

Merged
merged 8 commits into from
Sep 27, 2020
Merged

Redesigns handling of nonIntegerWinningThreshold and hareQuota in the GUI #512

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
d00e3e1
Changes the `TextArea` on the right to display hints specific to the …
HEdingfield Sep 25, 2020
9059ecf
Creates `throwIfNoUwiLabel` helper function in `CommonDataFormatReade…
HEdingfield Sep 25, 2020
7f34217
Replaces `checkBoxNonIntegerWinningThreshold` and `checkBoxHareQuota`…
HEdingfield Sep 26, 2020
89f5eba
GUI now disables numberOfWinners field and sets it to 0 when WinnerEl…
HEdingfield Sep 26, 2020
0623206
Implements radio button array in GUI to determine nonIntegerWinningTh…
HEdingfield Sep 26, 2020
a97ce71
Merge branch 'develop' into threshold-calc-radio
HEdingfield Sep 26, 2020
7efb713
Add comment per review feedback.
HEdingfield Sep 27, 2020
96e4483
Addressing review feedback, including renaming `WinnerElectionMode.ST…
HEdingfield Sep 27, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 36 additions & 13 deletions src/main/java/network/brightspots/rcv/ContestConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,6 @@ class ContestConfig {
static final int SUGGESTED_CVR_FIRST_VOTE_ROW = 2;
static final int SUGGESTED_CVR_ID_COLUMN = 1;
static final int SUGGESTED_CVR_PRECINCT_COLUMN = 2;
static final int SUGGESTED_NUMBER_OF_WINNERS = 1;
static final int SUGGESTED_DECIMAL_PLACES_FOR_VOTE_ARITHMETIC = 4;
static final int SUGGESTED_MAX_SKIPPED_RANKS_ALLOWED = 1;
static final String SUGGESTED_OVERVOTE_LABEL = "overvote";
Expand Down Expand Up @@ -722,30 +721,28 @@ && getMaxRankingsAllowed() < MIN_MAX_RANKINGS_ALLOWED)) {
Logger.log(Level.SEVERE, "batchElimination can't be true in a multi-seat contest!");
}
} else { // numberOfWinners == 1
if (isMultiSeatSequentialWinnerTakesAllEnabled() ||
isMultiSeatBottomsUpUntilNWinnersEnabled() ||
isMultiSeatAllowOnlyOneWinnerPerRoundEnabled() ||
isMultiSeatAllowMultipleWinnersPerRoundEnabled()) {
if (!isSingleWinnerEnabled()) {
isValid = false;
Logger.log(
Level.SEVERE,
"winnerElectionMode can't be \"%s\" in a single-seat contest!",
winnerMode.toString()
);
}

if (isHareQuotaEnabled()) {
isValid = false;
Logger.log(Level.SEVERE, "hareQuota can only be true in a multi-seat contest!");
}
}
} else { // numberOfWinners == 0
if (!isMultiSeatBottomsUpWithThresholdEnabled()
|| getMultiSeatBottomsUpPercentageThreshold() == null) {
if (!isMultiSeatBottomsUpWithThresholdEnabled()) {
isValid = false;
Logger.log(Level.SEVERE,
"If numberOfWinners is zero, winnerElectionMode must be \"%s\" and multiSeatBottomsUpPercentageThreshold must be specified!",
winnerMode.toString());
WinnerElectionMode.MULTI_SEAT_BOTTOMS_UP_USING_PERCENTAGE_THRESHOLD);
} else {
if (getMultiSeatBottomsUpPercentageThreshold() == null) {
HEdingfield marked this conversation as resolved.
Show resolved Hide resolved
isValid = false;
Logger.log(Level.SEVERE,
"If winnerElectionMode is \"%s\", multiSeatBottomsUpPercentageThreshold must be specified!",
winnerMode.toString());
}
}
}
}
HEdingfield marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -756,6 +753,28 @@ && getMaxRankingsAllowed() < MIN_MAX_RANKINGS_ALLOWED)) {
winnerMode.toString());
}

// nonIntegerWinningThreshold and hareQuota are only allowed for multi-seat elections
if (!isMultiSeatAllowOnlyOneWinnerPerRoundEnabled()
&& !isMultiSeatAllowMultipleWinnersPerRoundEnabled()) {
HEdingfield marked this conversation as resolved.
Show resolved Hide resolved
Comment on lines +755 to +756
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps it would be useful to have a helper like isStandardMultiSeatEnabled() that matches these two modes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we end up doing the same check in the future, I'd be on board with this, but will leave as-is for now.

if (isNonIntegerWinningThresholdEnabled()) {
isValid = false;
Logger.log(Level.SEVERE,
"nonIntegerWinningThreshold can't be true when winnerElectionMode is \"%s\"!",
winnerMode.toString());
}
if (isHareQuotaEnabled()) {
isValid = false;
Logger.log(Level.SEVERE, "hareQuota can't be true when winnerElectionMode is \"%s\"!",
winnerMode.toString());
}
}

if (isNonIntegerWinningThresholdEnabled() && isHareQuotaEnabled()) {
isValid = false;
Logger.log(Level.SEVERE,
"nonIntegerWinningThreshold and hareQuota can't both be true at the same time!");
}

if (!isNullOrBlank(getOvervoteLabel())
&& stringAlreadyInUseElsewhere(getOvervoteLabel(), "overvoteLabel")) {
isValid = false;
Expand Down Expand Up @@ -821,6 +840,10 @@ WinnerElectionMode getWinnerElectionMode() {
return mode == null ? WinnerElectionMode.MODE_UNKNOWN : mode;
}

boolean isSingleWinnerEnabled() {
return getWinnerElectionMode() == WinnerElectionMode.STANDARD;
HEdingfield marked this conversation as resolved.
Show resolved Hide resolved
}

boolean isMultiSeatAllowOnlyOneWinnerPerRoundEnabled() {
return getWinnerElectionMode() == WinnerElectionMode.MULTI_SEAT_ALLOW_ONLY_ONE_WINNER_PER_ROUND;
}
Expand Down
55 changes: 33 additions & 22 deletions src/main/java/network/brightspots/rcv/GuiConfigController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@
import network.brightspots.rcv.Tabulator.TieBreakMode;
import network.brightspots.rcv.Tabulator.WinnerElectionMode;

@SuppressWarnings("WeakerAccess")
@SuppressWarnings({"WeakerAccess", "rawtypes"})
HEdingfield marked this conversation as resolved.
Show resolved Hide resolved
public class GuiConfigController implements Initializable {

private static final DateTimeFormatter DATE_TIME_FORMATTER =
Expand Down Expand Up @@ -202,9 +202,11 @@ public class GuiConfigController implements Initializable {
@FXML
private TextField textFieldRulesDescription;
@FXML
private CheckBox checkBoxNonIntegerWinningThreshold;
private RadioButton radioThresholdMostCommon;
@FXML
private CheckBox checkBoxHareQuota;
private RadioButton radioThresholdHbQuota;
@FXML
private RadioButton radioThresholdHareQuota;
@FXML
private CheckBox checkBoxBatchElimination;
@FXML
Expand Down Expand Up @@ -743,10 +745,12 @@ private void clearAndDisableWinningRuleFields() {
textFieldNumberOfWinners.setDisable(true);
textFieldMultiSeatBottomsUpPercentageThreshold.clear();
textFieldMultiSeatBottomsUpPercentageThreshold.setDisable(true);
checkBoxNonIntegerWinningThreshold.setSelected(false);
checkBoxNonIntegerWinningThreshold.setDisable(true);
checkBoxHareQuota.setSelected(false);
checkBoxHareQuota.setDisable(true);
radioThresholdMostCommon.setSelected(false);
radioThresholdMostCommon.setDisable(true);
radioThresholdHbQuota.setSelected(false);
radioThresholdHbQuota.setDisable(true);
radioThresholdHareQuota.setSelected(false);
radioThresholdHareQuota.setDisable(true);
textFieldDecimalPlacesForVoteArithmetic.clear();
textFieldDecimalPlacesForVoteArithmetic.setDisable(true);
}
Expand All @@ -757,13 +761,11 @@ private void clearAndDisableTiebreakFields() {
}

private void setWinningRulesDefaultValues() {
checkBoxNonIntegerWinningThreshold.setSelected(
ContestConfig.SUGGESTED_NON_INTEGER_WINNING_THRESHOLD);
checkBoxHareQuota.setSelected(ContestConfig.SUGGESTED_HARE_QUOTA);
setThresholdCalculationMethodRadioButton(ContestConfig.SUGGESTED_NON_INTEGER_WINNING_THRESHOLD,
ContestConfig.SUGGESTED_HARE_QUOTA);
checkBoxBatchElimination.setSelected(ContestConfig.SUGGESTED_BATCH_ELIMINATION);
checkBoxContinueUntilTwoCandidatesRemain
.setSelected(ContestConfig.SUGGESTED_CONTINUE_UNTIL_TWO_CANDIDATES_REMAIN);
textFieldNumberOfWinners.setText(String.valueOf(ContestConfig.SUGGESTED_NUMBER_OF_WINNERS));
textFieldDecimalPlacesForVoteArithmetic.setText(
String.valueOf(ContestConfig.SUGGESTED_DECIMAL_PLACES_FOR_VOTE_ARITHMETIC));
textFieldMaxRankingsAllowed.setText(ContestConfig.SUGGESTED_MAX_RANKINGS_ALLOWED);
Expand Down Expand Up @@ -1063,33 +1065,32 @@ public LocalDate fromString(String string) {
case STANDARD -> {
textFieldMaxRankingsAllowed.setDisable(false);
textFieldMinimumVoteThreshold.setDisable(false);
choiceTiebreakMode.setDisable(false);
checkBoxHareQuota.setDisable(false);
checkBoxBatchElimination.setDisable(false);
checkBoxContinueUntilTwoCandidatesRemain.setDisable(false);
choiceTiebreakMode.setDisable(false);
textFieldNumberOfWinners.setText("1");
}
case MULTI_SEAT_ALLOW_ONLY_ONE_WINNER_PER_ROUND, MULTI_SEAT_ALLOW_MULTIPLE_WINNERS_PER_ROUND -> {
textFieldMaxRankingsAllowed.setDisable(false);
textFieldMinimumVoteThreshold.setDisable(false);
choiceTiebreakMode.setDisable(false);
checkBoxNonIntegerWinningThreshold.setDisable(false);
checkBoxHareQuota.setDisable(false);
radioThresholdMostCommon.setDisable(false);
radioThresholdHbQuota.setDisable(false);
radioThresholdHareQuota.setDisable(false);
textFieldDecimalPlacesForVoteArithmetic.setDisable(false);
textFieldNumberOfWinners.setDisable(false);
}
case MULTI_SEAT_BOTTOMS_UP_UNTIL_N_WINNERS, MULTI_SEAT_SEQUENTIAL_WINNER_TAKES_ALL -> {
textFieldMaxRankingsAllowed.setDisable(false);
textFieldMinimumVoteThreshold.setDisable(false);
choiceTiebreakMode.setDisable(false);
checkBoxHareQuota.setDisable(false);
textFieldNumberOfWinners.setDisable(false);
}
case MULTI_SEAT_BOTTOMS_UP_USING_PERCENTAGE_THRESHOLD -> {
textFieldMaxRankingsAllowed.setDisable(false);
textFieldMinimumVoteThreshold.setDisable(false);
choiceTiebreakMode.setDisable(false);
checkBoxHareQuota.setDisable(false);
textFieldNumberOfWinners.setDisable(false);
textFieldNumberOfWinners.setText("0");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we hide it entirely in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The answer is yes, but it wouldn't move any of the options around, it'd just appear as a blank space there, which is awkward. I also think we've had this conversation a number of times and I've been pretty firmly in the camp of keeping the user as aware of what ends up in the config file as possible. Happy to reassess this in the future for the entire GUI if you'd like to file an issue for that. Chris would probably be excited about this idea.

textFieldMultiSeatBottomsUpPercentageThreshold.setDisable(false);
}
}
Expand Down Expand Up @@ -1257,14 +1258,24 @@ private void loadConfig(ContestConfig config) {
textFieldUndervoteLabel.setText(rules.undervoteLabel);
textFieldUndeclaredWriteInLabel.setText(rules.undeclaredWriteInLabel);
textFieldRulesDescription.setText(rules.rulesDescription);
checkBoxNonIntegerWinningThreshold.setSelected(rules.nonIntegerWinningThreshold);
checkBoxHareQuota.setSelected(rules.hareQuota);
setThresholdCalculationMethodRadioButton(rules.nonIntegerWinningThreshold, rules.hareQuota);
checkBoxBatchElimination.setSelected(rules.batchElimination);
checkBoxContinueUntilTwoCandidatesRemain.setSelected(rules.continueUntilTwoCandidatesRemain);
checkBoxExhaustOnDuplicateCandidate.setSelected(rules.exhaustOnDuplicateCandidate);
checkBoxTreatBlankAsUndeclaredWriteIn.setSelected(rules.treatBlankAsUndeclaredWriteIn);
}

private void setThresholdCalculationMethodRadioButton(boolean nonIntegerWinningThreshold,
boolean hareQuota) {
if (!nonIntegerWinningThreshold && !hareQuota) {
radioThresholdMostCommon.setSelected(true);
} else if (nonIntegerWinningThreshold && !hareQuota) {
radioThresholdHbQuota.setSelected(true);
} else if (!nonIntegerWinningThreshold) {
radioThresholdHareQuota.setSelected(true);
} // If both are true, don't select any option since this should no longer be valid
}

private void setOvervoteRuleRadioButton(OvervoteRule overvoteRule) {
switch (overvoteRule) {
case ALWAYS_SKIP_TO_NEXT_RANK -> radioOvervoteAlwaysSkip.setSelected(true);
Expand Down Expand Up @@ -1324,8 +1335,8 @@ private RawContestConfig createRawContestConfig() {
rules.minimumVoteThreshold = getTextOrEmptyString(textFieldMinimumVoteThreshold);
rules.maxSkippedRanksAllowed = getTextOrEmptyString(textFieldMaxSkippedRanksAllowed);
rules.maxRankingsAllowed = getTextOrEmptyString(textFieldMaxRankingsAllowed);
rules.nonIntegerWinningThreshold = checkBoxNonIntegerWinningThreshold.isSelected();
rules.hareQuota = checkBoxHareQuota.isSelected();
rules.nonIntegerWinningThreshold = radioThresholdHbQuota.isSelected();
rules.hareQuota = radioThresholdHareQuota.isSelected();
rules.batchElimination = checkBoxBatchElimination.isSelected();
rules.continueUntilTwoCandidatesRemain = checkBoxContinueUntilTwoCandidatesRemain.isSelected();
rules.exhaustOnDuplicateCandidate = checkBoxExhaustOnDuplicateCandidate.isSelected();
Expand Down
36 changes: 18 additions & 18 deletions src/main/resources/network/brightspots/rcv/GuiConfigLayout.fxml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -451,27 +451,27 @@
<Insets bottom="4.0" left="4.0" right="4.0" top="4.0"/>
</padding>
</HBox>
<HBox alignment="CENTER_LEFT" spacing="4.0">
<CheckBox fx:id="checkBoxNonIntegerWinningThreshold" mnemonicParsing="false"
text="Non-Integer Winning Threshold">
<VBox.margin>
<Insets top="8.0" bottom="8.0"/>
</VBox.margin>
</CheckBox>
<VBox spacing="4.0">
<fx:define>
<ToggleGroup fx:id="thresholdCalculationMethodToggleGroup"/>
</fx:define>
<Label text="Threshold Calculation Method *"/>
<RadioButton alignment="TOP_LEFT" mnemonicParsing="false"
toggleGroup="$thresholdCalculationMethodToggleGroup"
fx:id="radioThresholdMostCommon"
text="Compute using most common threshold formula&#xD; = ((Votes / (Seats + 1)) + 1, disregarding fractions"/>
<RadioButton alignment="TOP_LEFT" mnemonicParsing="false"
toggleGroup="$thresholdCalculationMethodToggleGroup"
fx:id="radioThresholdHbQuota"
text="Compute using HB Quota&#xD; > (Votes / (Seats +1))"/>
HEdingfield marked this conversation as resolved.
Show resolved Hide resolved
<RadioButton alignment="TOP_LEFT" mnemonicParsing="false"
toggleGroup="$thresholdCalculationMethodToggleGroup"
fx:id="radioThresholdHareQuota"
text="Compute using Hare Quota&#xD; = (Votes / Seats)"/>
<padding>
<Insets bottom="4.0" left="4.0" right="4.0" top="4.0"/>
</padding>
</HBox>
<HBox alignment="CENTER_LEFT" spacing="4.0">
<CheckBox fx:id="checkBoxHareQuota" mnemonicParsing="false" text="Hare Quota">
<VBox.margin>
<Insets top="8.0" bottom="8.0"/>
</VBox.margin>
</CheckBox>
<padding>
<Insets bottom="4.0" left="4.0" right="4.0" top="4.0"/>
</padding>
</HBox>
</VBox>
<HBox alignment="CENTER_LEFT" spacing="4.0">
<Label prefWidth="196.0"
text="Decimal Places for Vote Arithmetic&#xD; (Multi-Winner Only) *"/>
Expand Down