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.

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

Redesigns handling of nonIntegerWinningThreshold and hareQuota in the GUI #512

Merged
merged 8 commits into from
Sep 27, 2020

Conversation

HEdingfield
Copy link
Contributor

  • Replaces checkBoxNonIntegerWinningThreshold and checkBoxHareQuota with a radio button array in the FXML file (addressing Redesign nonIntegerWinningThreshold and hareQuota in GUI and add new validation rule #501)
  • Implements radio button array in GUI to determine nonIntegerWinningThreshold and hareQuota values, and adds new validation rules for those settings.
  • GUI now disables numberOfWinners field and sets it to 1 only when winnerElectionMode is "Single-winner majority determines winner".
  • GUI now disables numberOfWinners field and sets it to 0 when winnerElectionMode is "Bottoms-up using percentage threshold".
  • Fixes bugs in validation error messages when numberOfWinners is 0
  • Suppresses rawtypes warnings in GuiConfigController.

…r`, with minor refactoring. Suppresses warnings that aren't useful. Auto-rearranging of some methods.
… with a radio button array in the FXML file.
…ectionMode is "Bottoms-up using percentage threshold"; fixes bugs in validation error messages when numberOfWinners is 0; suppresses rawtypes warnings in `GuiConfigController`.
…reshold and hareQuota values, and adds new validation rules for those settings; GUI now disables numberOfWinners field and sets it to 1 only when WinnerElectionMode is "Single-winner majority determines winner".
Copy link
Contributor

@moldover moldover left a comment

Choose a reason for hiding this comment

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

LGTM. Will let @tarheel have a pass.

Copy link
Contributor

@tarheel tarheel left a comment

Choose a reason for hiding this comment

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

just some minor things inline

src/main/java/network/brightspots/rcv/ContestConfig.java Outdated Show resolved Hide resolved
src/main/java/network/brightspots/rcv/ContestConfig.java Outdated Show resolved Hide resolved
Comment on lines +757 to +758
if (!isMultiSeatAllowOnlyOneWinnerPerRoundEnabled()
&& !isMultiSeatAllowMultipleWinnersPerRoundEnabled()) {
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.

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.

…ANDARD` to `WinnerElectionMode.STANDARD_SINGLE_WINNER`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants