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

[Qt] Add spend all button to the SendCoinsDialog #11098

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@CryptAxe
Copy link
Contributor

CryptAxe commented Aug 20, 2017

Related to: #11033

[Qt] Add spend all button to the SendCoinsDialog
Get the balance of the wallet (with CoinControl if enabled).
Then divide the amount available by all of the SendCoinEntry(s) and
check the subtract fee from amount checkbox for all SendCoinEntry(s).

@fanquake fanquake added the GUI label Aug 20, 2017

@MeshCollider

This comment has been minimized.

Copy link
Member

MeshCollider commented Aug 20, 2017

I don't like the dividing equally between outputs behaviour, I think the button should only be possible on single entry spends, makes little sense to me otherwise.
From discussion in #11033, @laanwj didn't just want a button added for appearance reasons.
Also nits, please update to snake_case and modify braces as per style guideline.

@CryptAxe

This comment has been minimized.

Copy link
Contributor Author

CryptAxe commented Aug 20, 2017

I don't like the dividing equally between outputs behaviour, I think the button should only be possible on single entry spends, makes little sense to me otherwise.

Don't use the button with multiple outputs if you don't like it, no reason to stop other people from dividing their balance between multiple recipients. The way the SendCoinsDialog works right now assumes that there can be any number of recipients.

From discussion in #11033, @laanwj didn't just want a button added for appearance reasons.

Feel free to suggest a better option. I put the button in an area so that it doesn't take up any vertical space and mess up the appearance of the entire page. It would be possible to put something on the SendCoinEntry(s) instead and send a signal to the SendCoinsDialog instance for example but that was more cluttered visually and in code when I tried it.

Also nits, please update to snake_case and modify braces as per style guideline.

You'll have to point out where you want me to use snake case...

@promag

This comment has been minimized.

Copy link
Member

promag commented Aug 20, 2017

You'll have to point out where you want me to use snake case...

See developer notes.

@CryptAxe

This comment has been minimized.

Copy link
Contributor Author

CryptAxe commented Aug 21, 2017

The best assumption that I can make from your response is that you want me to use snake case for variable names. I see that is in the developer notes now, but it is certainly not in line with most of the code that I see in Bitcoin core. Am I correct that we are now going to diverge from the style in the existing codebase, and require all new code to look different, creating a big mess of many different styles?

I don't know how much value a style guideline has besides maintaining clarity and uniformity of code, aren't we throwing that out the window here?

@achow101

This comment has been minimized.

Copy link
Member

achow101 commented Aug 21, 2017

@CryptAxe Yes, use snake case for all new code. Please follow the developer guidelines for all new code, regardless of the style currently in the codebase. Since the codebase changes a lot, the style of the codebase will shift over to the new style as things are changed and replaced.

@CryptAxe

This comment has been minimized.

Copy link
Contributor Author

CryptAxe commented Aug 21, 2017

Why snake case, what led to that change? Who even made this decision and why would we diverge from the existing code in the first place? If this rule is followed we are making Bitcoin's code worse, it will be a mix of several styles (the new one being especially ugly in my opinion). The point of style guidelines are to maintain clarity and uniformity. There is no reason for Bitcoin to have style guidelines if they are going to change all the time. Do we really think 100% of the codebase is going to be re-written in the new style, or is it more likely that we will end up with an ugly mess as a result of this?

Also I'm not trying to roast anyone who has commented here, I'm addressing the broader developer community with my questions..

@promag

This comment has been minimized.

Copy link
Member

promag commented Aug 21, 2017

Why snake case, what led to that change? Who even made this decision and why would we diverge from the existing code in the first place?

Some group of people did, git blame can point you in the right direction. But who cares, for me consistency wins.

Do we really think 100% of the codebase is going to be re-written in the new style

Eventually.

or is it more likely that we will end up with an ugly mess as a result of this?

I guess it is a mess because of the early lack of rules to follow and time to enforce those rules. Even nowadays, there is lot of new code that doesn't met those rules.

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Sep 7, 2017

Concept ACK.
I think the send all button needs to be in the send-to-box context (not in the global). Ideally close to where you enter the amount (it's mostly related to that IMO).

I think it should be [amount] [denomination] [send-all] [subtract-from-amount].

Maybe we should call it not send-all because the max amount may change after you have pressed the button == you no longer sending "all". Maybe something like "select all", or "use max available" makes more sense.

bildschirmfoto 2017-09-07 um 10 28 10

@CryptAxe

This comment has been minimized.

Copy link
Contributor Author

CryptAxe commented Sep 7, 2017

@MeshCollider

This comment has been minimized.

Copy link
Member

MeshCollider commented Sep 8, 2017

What's wrong with using a button like you're currently doing? Why are you talking about a checkbox?

@CryptAxe

This comment has been minimized.

Copy link
Contributor Author

CryptAxe commented Sep 8, 2017

@promag

This comment has been minimized.

Copy link
Member

promag commented Sep 8, 2017

Maybe something like "select all", or "use max available" makes more sense

Agree with @jonasschnelli, and that "button" should fill the amount field and toggle the checkbox. And I think it works well with multiple outputs: So when pressed the amount field will have the available amount minus other input amounts.

@achow101 achow101 referenced this pull request Oct 19, 2017

Closed

[QT] - Send-all Button #11033

MarcoFalke added a commit that referenced this pull request Nov 10, 2017

Merge #11316: [qt] Add use available balance in send coins dialog (Cr…
…yptAxe, promag)

d052e38 [qt] Add use available balance in send coins dialog (CryptAxe)

Pull request description:

  This is an alternative to #11098 to handle #11033 where a new button `Use available balance` is added to each entry. When activated, the available balance is calculated by using the coin control (if any) and then it's subtracted the remaining recipient amounts. If this amount is positive then the `Subtract fee from amount` is automatically selected.

  Comparing to #11098, this has the advantage to avoid the fair amount division over the recipients and allows to fine adjust the amounts in multiple iterations.

  Started from @CryptAxe commit 89e9eda to credit some code.

  <img width="965" alt="screen shot 2017-09-13 at 01 32 44" src="https://user-images.githubusercontent.com/3534524/30354518-e1bee31c-9824-11e7-9354-300aa63cdfd0.png">
  <img width="964" alt="screen shot 2017-09-13 at 01 44 57" src="https://user-images.githubusercontent.com/3534524/30354598-5731ac9c-9825-11e7-9d5f-8781988ed219.png">

Tree-SHA512: 01d20c13fd8b6c2a0ca1d74d3a9027c6922e6dccd3b08e59d5a72636be7072ed5eca7ebc5d431299497dd3374e83753220ad4174d8bc46dadb4b2f54973036a5
@CryptAxe

This comment has been minimized.

Copy link
Contributor Author

CryptAxe commented Nov 17, 2017

#11316 which improved this was merged so I'll close it.

@CryptAxe CryptAxe closed this Nov 17, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.