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

Add used balance to overview page #775

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

BrandonOdiwuor
Copy link
Contributor

@BrandonOdiwuor BrandonOdiwuor commented Nov 3, 2023

Second part: Fixes #769

Add used balance to the overview page for wallets with the avoid_reuse flag enabled

Prerequsite:

overview page when avoid_reuse is enabled
Screenshot from 2023-11-02 18-10-06

overview page when avoid_reuse is not enabled
Screenshot from 2023-11-03 11-07-45

Add used balance to overview page for wallets with avoid_reuse flag enabled
@DrahtBot
Copy link
Contributor

DrahtBot commented Nov 3, 2023

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type Reviewers
Concept ACK katesalazar, pablomartin4btc

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Copy link
Contributor

@katesalazar katesalazar left a comment

Choose a reason for hiding this comment

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

Concept ACK

<string>Your current used balance</string>
</property>
<property name="text">
<string notr="true">21 000 000.00000000 BTC</string>
Copy link
Contributor

Choose a reason for hiding this comment

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

<cursorShape>IBeamCursor</cursorShape>
</property>
<property name="toolTip">
<string>Your current used balance</string>
Copy link
Contributor

Choose a reason for hiding this comment

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

Because the space is tiny, and the impact should be big, every word in an effective tooltip needs to pack a punch.

Source: Internet

as the tooltip for the "used" line of the "balances" block,
"your current used balance" doesnt feel like a great tooltip

@hebasto hebasto changed the title gui: add used balance to overview page Add used balance to overview page Nov 20, 2023
Copy link
Contributor

@pablomartin4btc pablomartin4btc left a comment

Choose a reason for hiding this comment

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

Concept ACK

As mentioned on the first part/ core PR, please consider to add the wallet interface commit as the very first commit here on this PR so it's easier to test this new feature.

@DrahtBot
Copy link
Contributor

🤔 There hasn't been much activity lately and the CI seems to be failing.

If no one reviewed the current pull request by commit hash, a rebase can be considered. While the CI failure may be a false positive, the CI hasn't been running for some time, so there may be a real issue hiding as well. A rebase triggers the latest CI and makes sure that no silent merge conflicts have snuck in.

@luke-jr
Copy link
Member

luke-jr commented Apr 24, 2024

What is the use case here? (And the avoid_reuse flag in general...)

@hebasto
Copy link
Member

hebasto commented May 12, 2024

@BrandonOdiwuor

Second part: Fixes #769

Add used balance to the overview page for wallets with the avoid_reuse flag enabled

Prerequsite:

If you are still working on this PR, please address bitcoin/bitcoin#28776 (comment).

@BrandonOdiwuor
Copy link
Contributor Author

@hebasto Sorry I will be updating the PR description

There was a change in implementation. I created a different PR (bitcoin/bitcoin#29062) based on the recommendation to refactor the getbalance() function which touches the bitcoin repo. trying to get it merged so that I can update this PR

@hebasto
Copy link
Member

hebasto commented May 15, 2024

There was a change in implementation. I created a different PR (bitcoin/bitcoin#29062) based on the recommendation to refactor the getbalance() function which touches the bitcoin repo. trying to get it merged so that I can update this PR

Okay. Until then, marking this PR as a draft.

@hebasto hebasto marked this pull request as draft May 15, 2024 16:54
@DrahtBot
Copy link
Contributor

🤔 There hasn't been much activity lately and the CI seems to be failing.

If no one reviewed the current pull request by commit hash, a rebase can be considered. While the CI failure may be a false positive, the CI hasn't been running for some time, so there may be a real issue hiding as well. A rebase triggers the latest CI and makes sure that no silent merge conflicts have snuck in.

1 similar comment
@DrahtBot
Copy link
Contributor

🤔 There hasn't been much activity lately and the CI seems to be failing.

If no one reviewed the current pull request by commit hash, a rebase can be considered. While the CI failure may be a false positive, the CI hasn't been running for some time, so there may be a real issue hiding as well. A rebase triggers the latest CI and makes sure that no silent merge conflicts have snuck in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

used balance should be shown on overview page
6 participants