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

ui: Add ability to change vote choice #25

Merged
merged 7 commits into from
Dec 18, 2019
Merged

Conversation

JoeGruffins
Copy link
Member

Add a screen to show current agendas and a dropdown menu to choose how
to vote.

screens

votingmain
votingchoose

When connecting to dcrdata, pull down the current agendas for use when
voting.
Add a screen to show current agendas and a dropdown menu to choose how
to vote.
Copy link
Member

@buck54321 buck54321 left a comment

Choose a reason for hiding this comment

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

Nice work.

pydecred/dcrdata.py Outdated Show resolved Hide resolved
ui/screens.py Outdated Show resolved Hide resolved
ui/screens.py Outdated Show resolved Hide resolved
pydecred/dcrdata.py Outdated Show resolved Hide resolved
pydecred/dcrdata.py Outdated Show resolved Hide resolved
ui/screens.py Outdated Show resolved Hide resolved
ui/screens.py Outdated Show resolved Hide resolved
ui/screens.py Outdated Show resolved Hide resolved
ui/screens.py Outdated

self.pages = []
self.page = 0
self.ignoreVoteIndexChange = False
Copy link
Member

Choose a reason for hiding this comment

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

Can you comment on the need for synchronization here? See my comment below, but If you use the full waiting screen provided by app.waitThread, this wouldn't be needed either.

Copy link
Member Author

Choose a reason for hiding this comment

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

do you mean to set the vote?

Copy link
Member

Choose a reason for hiding this comment

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

This property appears to be used to prevent concurrent calls to onChooseChoice. Am I reading that right?

This is okay, though I question the utility. Were you seeing some undesirable behavior without it?

Copy link
Member Author

Choose a reason for hiding this comment

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

There is an onconnected() function listening to changes in the dropdowns/comboboxes. It is triggered when a user clicks a new choice, which is good. It also triggers when we initially set the vote, which is not optimal.

For instance we make the dropdown and default index is zero. After checking with the user's pool their choice is "yes" at index 1, so we change it. Because we change it the onconnected () fires and sends an unnecessary request to the pool to set the vote to yes, although it is already yes.

Another way to avoid this is to ignore changes until we are synced, but I made this with the idea that a user can have different vote choices for different pools. So the problem happens again upon switching pools. I could change it so that the user has one one choice per wallet, and then we just set everything to that choice and set all new, old pools to the same vote choice, then I don't think this variable would be necessary.

Unless you have a better way?

Copy link
Member

Choose a reason for hiding this comment

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

Instead of currentIndexChanged, connect to the activated signal. The only caveat is that you'll have to verify something has actually changed before proceeding, because activated will be triggered even if they select the same thing that was already selected.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I'll do it that way. Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

ok. I think this is optimized, but let me know.

ui/screens.py Outdated Show resolved Hide resolved
@JoeGruffins
Copy link
Member Author

I'm still not quite sure about the waitThread, where would I throw that in? It would cover the entire agendas screen with a wait gif?

pydecred/vsp.py Outdated Show resolved Hide resolved
pydecred/vsp.py Outdated Show resolved Hide resolved
ui/screens.py Outdated Show resolved Hide resolved
ui/screens.py Outdated Show resolved Hide resolved
ui/screens.py Show resolved Hide resolved
ui/qutilities.py Show resolved Hide resolved
@JoeGruffins
Copy link
Member Author

still not completely sure that this is what you want as far as syncing. You are locked out of the voting screen unless certain conditions are met. s'ok?

pydecred/dcrdata.py Outdated Show resolved Hide resolved
ui/screens.py Outdated Show resolved Hide resolved
pydecred/dcrdata.py Outdated Show resolved Hide resolved
Save a reference to the drowdown in the function that fires when
activated. Move agendasInfo back to dcrdata blockchain.
Copy link
Member

@buck54321 buck54321 left a comment

Choose a reason for hiding this comment

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

I think these should be the last changes needed.

ui/screens.py Outdated
Comment on lines 1738 to 1739
# Refresh purchase info
pools[0].getPurchaseInfo()
Copy link
Member

Choose a reason for hiding this comment

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

Need at least of a try/except here. Also just realized that this is being called every time the PoolAccountScreen is stacked, which seems excessive. We'll need to maybe implement an expiration time on this information.

Also realized that we don't have a good testnet VSP to test against right now. I'm getting an expired certificate error for teststakepool.decred.org, and all of the others have issues too. Solution needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

https://www.dcrstakedinner.com

How long do you think the expiration should be? Because the user could change this via the vsp's web interface, I think too long is not good. Maybe an hour?

ui/screens.py Outdated Show resolved Hide resolved
@buck54321 buck54321 merged commit 2e517d6 into decred:master Dec 18, 2019
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.

2 participants