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

new GUI modal: tabulation confirmation page #812

Merged
merged 26 commits into from
Jun 11, 2024

Conversation

artoonie
Copy link
Collaborator

@artoonie artoonie commented Mar 23, 2024

closes #679
closes #133 (but not yet #825)

  • Creates new GUI Tabulation Dialog (see corresponding issue)
  • Has a check to make sure the # of ballots didn't change between "Check Ballot Count" and "Tabulate."
  • Extensible so we can later has the entirety of the CVRs and check that there was no change whatsoever, not just in ballot counts
  • Extensible so we can pull additional metadata and show on the GUI before hitting tabulate

@artoonie artoonie added the WIP label Mar 23, 2024
@artoonie artoonie changed the title new GUI modal: tabulation confirmation page WIP: new GUI modal: tabulation confirmation page Mar 31, 2024
@artoonie artoonie force-pushed the feature/issue-679_confirm-num-ballots branch from f88126f to fc56e93 Compare March 31, 2024 16:34
@artoonie artoonie marked this pull request as draft March 31, 2024 23:34
@artoonie artoonie force-pushed the feature/issue-679_confirm-num-ballots branch from 6b12f87 to 256b816 Compare April 13, 2024 21:00
@artoonie artoonie force-pushed the feature/issue-679_confirm-num-ballots branch from 2c50723 to 842fa95 Compare April 21, 2024 23:32
@artoonie artoonie force-pushed the feature/issue-679_confirm-num-ballots branch from d8def3b to 1f1f9d4 Compare April 29, 2024 19:23
@artoonie artoonie force-pushed the feature/issue-679_confirm-num-ballots branch from 1f1f9d4 to 3eca021 Compare April 29, 2024 19:54
@artoonie artoonie removed the WIP label Apr 29, 2024
@artoonie artoonie marked this pull request as ready for review April 29, 2024 20:26
@artoonie artoonie changed the title WIP: new GUI modal: tabulation confirmation page new GUI modal: tabulation confirmation page Apr 30, 2024
@yezr
Copy link
Collaborator

yezr commented May 8, 2024

Two things I noticed while looking over this

  • The progress bar hangs during tabulation. It starts out moving, then at some point during tabulation hangs. Here's a video.
  • Each run of tabulate takes a little bit longer than the previous one. Here's some splits I did with the Minneapolis Mayor test data. It got about 5 seconds longer each time I ran it. Not the end of the world, but might be worth looking into...
image

@artoonie
Copy link
Collaborator Author

artoonie commented May 8, 2024

Good catch! Both of these are present in 1.3.2 as well, though I agree this PR makes them more evident (especially the GUI lag).

I'll tackle the increase in timing first, then look at the GUI freezing. Since both exist outside of this PR, it may be useful to have separate tickets for them? But not super necessary.

@yezr
Copy link
Collaborator

yezr commented May 8, 2024

I'll make new tickets. A P0 for the UI hanging #830. A P2 for the increased duration on consecutive runs #831

@artoonie
Copy link
Collaborator Author

artoonie commented May 8, 2024

Turns out both had the same underlying cause, and both are fixed in #832

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.

@HEdingfield since this is mostly UI, would you have time to review it?

@artoonie artoonie force-pushed the feature/issue-679_confirm-num-ballots branch from 3fa0575 to 83b58eb Compare May 30, 2024 17:34
* make the log output faster, and add colors too

* spruce up the style

* Issue 830: prevent lagging

* remove unused import

* re-enable and fix word wrap

* better warning color

---------

Co-authored-by: Armin Samii <[email protected]>
@HEdingfield HEdingfield self-requested a review June 3, 2024 22:41
@yezr
Copy link
Collaborator

yezr commented Jun 3, 2024

can we make the Number of Ballots number formatted with commas on the thousands like 1,000 or 1,000,000

@HEdingfield
Copy link
Contributor

can we make the Number of Ballots number formatted with commas on the thousands like 1,000 or 1,000,000

Sure, I'll handle this in my nit-handling commit shortly.

@artoonie
Copy link
Collaborator Author

artoonie commented Jun 4, 2024

Done!

…ank()`; removes "please" from user comms per Google style; addresses nits, minor refactoring, and other clean-up.
config/spotbugs/exclude.xml Show resolved Hide resolved
config/spotbugs/exclude.xml Show resolved Hide resolved
src/main/java/network/brightspots/rcv/Logger.java Outdated Show resolved Hide resolved
item.setPrefWidth(listView.getWidth() - 50);

// Fix the label padding
setPadding(new Insets(0, 0, 0, 3));
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure how this looks on your system, but this leaves an awkwardly-large amount of spacing at the bottom of each line for me. Can we maybe try 0.5 or 1 instead? Actually, playing with these values doesn't seem to help the weird spacing. Could you please get to the bottom of this?

image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was fixed here: 9b3b5f7

Not sure what git snafu happened here, but if you grab the latest, do you still see this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, still seeing this. :(

Copy link
Contributor

Choose a reason for hiding this comment

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

Ping on this. We can boot out to another issue if you'd like. CC: @yezr

Copy link
Collaborator

Choose a reason for hiding this comment

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

Like Armin, I'm also not seeing the extra line spacing behavior with the latest code. image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this is Windows-specific, but haven't had a chance to boot up my windows machine and check yet

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep, this is a windows line ending issue. Fixed here, as the bug exists on develop too: #842

Copy link
Contributor

Choose a reason for hiding this comment

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

Excellent, thanks for fixing! I'll test that PR separately.

@artoonie artoonie force-pushed the feature/issue-679_confirm-num-ballots branch from 3e375d5 to 85c4e51 Compare June 4, 2024 16:16
@artoonie artoonie force-pushed the feature/issue-679_confirm-num-ballots branch from 9f10c5e to 0e84c5b Compare June 6, 2024 18:07
item.setPrefWidth(listView.getWidth() - 50);

// Fix the label padding
setPadding(new Insets(0, 0, 0, 3));
Copy link
Contributor

Choose a reason for hiding this comment

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

Ping on this. We can boot out to another issue if you'd like. CC: @yezr

@artoonie artoonie mentioned this pull request Jun 9, 2024
@artoonie artoonie force-pushed the feature/issue-679_confirm-num-ballots branch from b1bfff7 to 89c370e Compare June 10, 2024 15:35
Copy link
Contributor

@HEdingfield HEdingfield left a comment

Choose a reason for hiding this comment

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

Great work, all set to merge!

item.setPrefWidth(listView.getWidth() - 50);

// Fix the label padding
setPadding(new Insets(0, 0, 0, 3));
Copy link
Contributor

Choose a reason for hiding this comment

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

Excellent, thanks for fixing! I'll test that PR separately.

@artoonie artoonie merged commit 6171d29 into develop Jun 11, 2024
1 check passed
@artoonie artoonie deleted the feature/issue-679_confirm-num-ballots branch June 11, 2024 16:51
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.

Add total ballots cast pop up validation during tabulation Maybe add progress bar for GUI tabulation
4 participants