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

FIX:When opening or autoloading wallets there should be clear messages about rescanning in progress and wallets' names. #786

Closed
wants to merge 2 commits into from

Conversation

xonx4l
Copy link

@xonx4l xonx4l commented Jan 10, 2024

issue-: #259

The "Opening Wallet" popup would now show "Opening Wallet: " instead of just "Opening Wallet". This identifies the specific wallet.

The "Rescanning" popup would show "Rescanning: " or "Rescanning - 0%" if a progress bar is added. Again, this identifies the wallet.

The main window title would include the wallet name after being opened/loaded thanks to the change in WalletView::showNormalIfMinimized().

Other dialogs like Send/Receive coins would also show the wallet name in the title.

So in summary, by appending the wallet name retrieved from WalletModel::getWalletName() to titles and popup messages, it provides clearer feedback to users about which wallet is being loaded and rescanned. No more confusion about which wallet Bitcoin Core is working on.

@DrahtBot
Copy link
Contributor

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

Reviews

See the guideline for information on the review process.
A summary of reviews will appear here.

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.

Thanks @xonx4l for working on this.

I don't think this PR fixes #259 as, according to its description and what I understand, a fix should be add more clarification (percentage?) about the rescanning of a wallet when they get open in 2 situations:

  1. During bitcoin-qt startup, as author of the PR specified, the user could add the wallets manually into the settings.json qt config file. In this case, perhaps the name of each wallet (if there are many, and may be also x/ total) should be added after the "rescanning..." text on the spash-screen (initial/ startup window).
  2. When a user open a wallet for the first time thru the menu->File->Open Wallet. For this case, perhaps a "rescanning..." (and maybe a % or progress bar - check issue Change "Open Wallet" popup text #679) text could be added at the bottom of the opening window.

@hebasto on his comment on the issue, mentioned to add the wallet name on the title of "Receiving addresses" & "Sending addresses" windows (which has been done and merged already on #757), not to the receivecoins or sendcoins dialogs which this PR changes, moreover since these dialogs are within the main window and titles won't be displayed:

image

Other than that, the title and description of the PR doesn't reflect the proper change that's making, please check the contributor and developer guidelines (e.g. commit name may start with prefix gui:, please check other PRs as reference).

Please don't get discouraged by these observations. Feel free to reach out if you have questions or need clarification on any points. I encourage you to continue working on this issue or any others. Thanks again!

@@ -72,6 +72,9 @@ void ReceiveCoinsDialog::setModel(WalletModel *_model)
{
this->model = _model;

setWindowTitle(tr("Request payments - %1").arg(model->getWalletName()));
Copy link
Contributor

Choose a reason for hiding this comment

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

CI lint is failing due to unnecessary white-spaces in the files (you can run the script lint-whitespace.py locally or even all linters)

Suggested change
setWindowTitle(tr("Request payments - %1").arg(model->getWalletName()));
setWindowTitle(tr("Request payments - %1").arg(model->getWalletName()));

@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

DrahtBot commented Aug 2, 2024

🤔 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.

@hebasto
Copy link
Member

hebasto commented Aug 12, 2024

Closing due to lack of support from the PR author (unaddressed comments, failed CI).

Feel free to re-open.

@hebasto hebasto closed this Aug 12, 2024
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.

4 participants