wallet: Translate [default wallet] string in progress messages #31296

pull ryanofsky wants to merge 2 commits into bitcoin:master from ryanofsky:pr/dtran changing 5 files +20 −13
  1. ryanofsky commented at 5:07 pm on November 15, 2024: contributor

    Noticed while reviewing #31287 (review) that the [default wallet] part of progress messages remains untranslated while the rest of the string is translated.

    Fix this in all places where CWallet::ShowProgress (which has a cancel button) and Chain::showProgress (which doesn’t have a cancel button) are called by making “default wallet” into a translated string.

  2. wallet: Translate [default wallet] string in progress messages
    Noticed while reviewing #31287
    (https://github.com/bitcoin/bitcoin/pull/31287#discussion_r1843809721) that the
    [default wallet] part of progress messages remains untranslated while the rest
    of the string is translated. Fix this in all places where Wallet::ShowProgress
    (which has a cancel button) and chain::showProgress (which doesn't have a
    cancel button) are called by making "default wallet" into a translated string.
    
    To minimize scope of this bugfix, this introduces a new wallet DisplayName()
    method which behaves differently than the existing GetDisplayName() method. The
    existing method will be cleaned up in the following commit.
    df45123dc0
  3. DrahtBot commented at 5:08 pm on November 15, 2024: contributor

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

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31296.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    Concept ACK hebasto

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #31061 (refactor: Check translatable format strings at compile-time by maflcko)
    • #30221 (wallet: Ensure best block matches wallet scan state by achow101)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  4. DrahtBot added the label Wallet on Nov 15, 2024
  5. hebasto commented at 4:43 pm on November 16, 2024: member
    Concept ACK.
  6. maflcko commented at 11:17 am on November 17, 2024: member
    What are the steps to test this with sqlite? If it isn’t needed after the bdb removal, it can probably be skipped?
  7. wallet, refactor: Replace GetDisplayName() with LogName()
    The GetDisplayName() method name was confusing because it suggested the return
    value could be used for display, while documentation and implementation
    indicated it only meant to be used for logging. Also the name didn't suggest
    that it was formatting the wallet names, which made it harder understand how
    messages were formatted in the places it was called. Fix these issues by
    splitting up the GetDisplayName() method and replacing it with LogName() /
    DisplayName() methods.
    
    This commit is a refactoring that does not change any behavior.
    ebb77ab77d
  8. ryanofsky force-pushed on Nov 18, 2024
  9. ryanofsky commented at 3:49 pm on November 18, 2024: contributor

    re: #31296 (comment)

    What are the steps to test this with sqlite? If it isn’t needed after the bdb removal, it can probably be skipped?

    Not sure if this will always be the case, but it is currently possible to create an sqlite wallet with no name with bitcoin-cli -regtest createwallet "".

    Regardless of the details of the bug though, I think the real problem is that the GetDisplayName() method name suggests it should be used to get wallet names for display while the code comment and implementation suggest it’s only meant be used for logging. So I updated the bugfix and added a second refactoring commit to this PR to clean up the interface and prevent this type of problem in the future. That might motivate the PR more beyond the current bugfix.

    Updated 87152db6668edbdb863adfb12c45533b37522ca6 -> ebb77ab77df4081f27ce55e8b28b0d35990caac8 (pr/dtran.1 -> pr/dtran.2, compare) adding some refactoring after the bugfix.


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-03 18:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me