Rendering an amp characters in the wallet name for QMenu #828

pull knst wants to merge 1 commits into bitcoin-core:master from knst:fix-qt-wallet-underscore changing 1 files +3 −4
  1. knst commented at 12:32 pm on July 13, 2024: contributor

    In the current implementation Qt uses ‘&’ as a signal to underscore letter and use it as a hot-key, which is not expected for case of wallet name.

    The comment in the code regarding the use of an “&” on a menu item is misleading. If a wallet name has an “&” in it, it is not supposed to be interpreted as a hot-key, but it should be shown as it is without replacing it to an underscore.

    See screenshots before & after: Screenshot_20240713_122454 Screenshot_20240713_131304

  2. DrahtBot commented at 12:32 pm on July 13, 2024: 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.

    Type Reviewers
    ACK hebasto, pablomartin4btc, BrandonOdiwuor

    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:

    • #824 (Migrate legacy wallets that are not loaded 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.

  3. hebasto commented at 10:59 am on July 14, 2024: member

    In the current implementation Qt uses ‘&’ as a signal to underscore letter and use it as a hot-key, which is not expected for case of wallet name.

    Does this hold for all supported Qt versions?

    The comment in the code is misleading, if you replace one & to double &&, the next & will be a “single one”. All of them in the string should be replaced, not only the first occurrence.

    I’m not certain about that, but the mentioned comment might have been correct for some earlier version of Qt.

  4. knst commented at 12:52 pm on July 14, 2024: contributor

    I’m not certain about that, but the mentioned comment might have been correct for some earlier version of Qt.

    Doc for qt 6 says:

    The ampersand in the menu item’s text sets Alt+F as a shortcut for this menu. (You can use “&&” to get a real ampersand in the menu bar.)

    Doc for qt 5 says the same:

    The ampersand in the menu item’s text sets Alt+F as a shortcut for this menu. (You can use “&&” to get a real ampersand in the menu bar.)

    And even doc for qt 4 says the same:

    The ampersand in the menu item’s text sets Alt+F as a shortcut for this menu. (You can use “&&” to get a real ampersand in the menu bar.)

    I believe that in original PR introduced a “partial fix” which works for the first ampersand no one just tested a wallet with several ampersands.

    I tested this fix for 2 versions: Qt 5.15.14 and Qt 5.15.11, I don’t have an older one to test, but I am pretty sure that if there’s no bug in the qt itself: it should works starting from 4.8 or even earlier and up to now

  5. hebasto commented at 1:20 pm on July 14, 2024: member

    I believe that in original PR introduced a “partial fix” which works for the first ampersand no one just tested a wallet with several ampersands.

    It might be a platform-specific issue though. See https://bugreports.qt.io/browse/QTBUG-63361.

  6. in src/qt/bitcoingui.cpp:404 in 948520b72d outdated
    398@@ -399,10 +399,7 @@ void BitcoinGUI::createActions()
    399             for (const std::pair<const std::string, bool>& i : m_wallet_controller->listWalletDir()) {
    400                 const std::string& path = i.first;
    401                 QString name = path.empty() ? QString("["+tr("default wallet")+"]") : QString::fromStdString(path);
    402-                // Menu items remove single &. Single & are shown when && is in
    403-                // the string, but only the first occurrence. So replace only
    404-                // the first & with &&.
    


    hebasto commented at 1:29 pm on July 14, 2024:
    I think that the comment should be adjusted rather than being deleted entirely.

    hebasto commented at 8:39 am on July 15, 2024:

    Maybe:

    0                // Menu items remove single &. Single & are shown when && is in
    1                // the string. So replace & with &&.
    

    ?


    pablomartin4btc commented at 10:30 pm on July 17, 2024:

    I agree with @hebasto. Small nit:

    0                // Menu items remove single &. Single & is shown when && are in
    1                // the string. So replace & with &&.
    
  7. hebasto commented at 1:29 pm on July 14, 2024: member

    Tested 948520b72d7fe8e7a3aa0d774159807058188627 on Ubuntu 24.04 with Qt 5.15.3. It works as expected.

    Going to test other platforms.

  8. hebasto commented at 1:32 pm on July 14, 2024: member
  9. knst commented at 4:25 pm on July 14, 2024: contributor

    It might be a platform-specific issue though. See https://bugreports.qt.io/browse/QTBUG-63361.

    could someone test on Mac? I will make probably platform’s depend implementation of char replacement

  10. hebasto approved
  11. hebasto commented at 8:39 am on July 15, 2024: member

    ACK 948520b72d7fe8e7a3aa0d774159807058188627, modulo https://github.com/bitcoin-core/gui/pull/828/files#r1677136023.

    Tested on Ubuntu 24.04, Windows 11 Pro and macOS 12.7.5 Monterey (x86).

  12. knst force-pushed on Jul 15, 2024
  13. knst requested review from hebasto on Jul 15, 2024
  14. in src/qt/bitcoingui.cpp:402 in 7581817826 outdated
    398@@ -399,10 +399,9 @@ void BitcoinGUI::createActions()
    399             for (const std::pair<const std::string, bool>& i : m_wallet_controller->listWalletDir()) {
    400                 const std::string& path = i.first;
    401                 QString name = path.empty() ? QString("["+tr("default wallet")+"]") : QString::fromStdString(path);
    402-                // Menu items remove single &. Single & are shown when && is in
    403-                // the string, but only the first occurrence. So replace only
    404-                // the first & with &&.
    405-                name.replace(name.indexOf(QChar('&')), 1, QString("&&"));
    406+                // The single ampersand in the menu item's text sets a shortcut for this menu.
    


    hebasto commented at 1:11 pm on July 15, 2024:

    Maybe

    0                // A single ampersand in the menu item's text sets a shortcut for this item.
    

    ?

  15. hebasto approved
  16. hebasto commented at 1:11 pm on July 15, 2024: member
    re-ACK 7581817826e000c4f0a505e7f2785a611cac8f55
  17. pablomartin4btc commented at 3:10 pm on July 15, 2024: contributor

    Concept ACK - I’ll test it soon.

    There’s a typo in the commit body (“missleading”), since you are there, perhaps a better prefix for its title could be “gui:” and a nit on the text could be:

    0The comment in the code regarding the use of an "&"
    1on a menu item is misleading. If a wallet name has an "&" in it,
    2it is not supposed to be interpreted as a hot-key, but it should be
    3shown as it is without replacing it to an underscore.
    
  18. knst renamed this:
    fix: rendering an amp characters in the wallet name for QMenu
    gui: rendering an amp characters in the wallet name for QMenu
    on Jul 15, 2024
  19. gui: correct replacement of amp character in the wallet name for QMenu
    The comment in the code regarding the use of an "&"
    on a menu item is misleading. If a wallet name has an "&" in it,
    it is not supposed to be interpreted as a hot-key, but it should be
    shown as it is without replacing it to an underscore.
    8233ee41ab
  20. knst force-pushed on Jul 15, 2024
  21. DrahtBot added the label CI failed on Jul 15, 2024
  22. hebasto approved
  23. hebasto commented at 6:27 pm on July 15, 2024: member

    re-ACK 8233ee41ab9648cd0c3bd78bc2a8d692a54d9ea0.

    CI failures are unrelated.

  24. DrahtBot requested review from pablomartin4btc on Jul 15, 2024
  25. hebasto renamed this:
    gui: rendering an amp characters in the wallet name for QMenu
    Rendering an amp characters in the wallet name for QMenu
    on Jul 15, 2024
  26. 1alexbc1 approved
  27. in src/qt/bitcoingui.cpp:402 in 8233ee41ab
    398@@ -399,10 +399,9 @@ void BitcoinGUI::createActions()
    399             for (const std::pair<const std::string, bool>& i : m_wallet_controller->listWalletDir()) {
    400                 const std::string& path = i.first;
    401                 QString name = path.empty() ? QString("["+tr("default wallet")+"]") : QString::fromStdString(path);
    402-                // Menu items remove single &. Single & are shown when && is in
    403-                // the string, but only the first occurrence. So replace only
    404-                // the first & with &&.
    405-                name.replace(name.indexOf(QChar('&')), 1, QString("&&"));
    406+                // An single ampersand in the menu item's text sets a shortcut for this item.
    


    pablomartin4btc commented at 10:35 pm on July 17, 2024:

    In case the above suggestion is not taken, typo here:

    0                // A single ampersand in the menu item's text sets a shortcut for this item.
    
  28. pablomartin4btc approved
  29. pablomartin4btc commented at 10:45 pm on July 17, 2024: contributor

    tACK 8233ee41ab9648cd0c3bd78bc2a8d692a54d9ea0

    • before image

    • after image

  30. hebasto added this to the milestone 28.0 on Jul 18, 2024
  31. BrandonOdiwuor approved
  32. BrandonOdiwuor commented at 7:34 am on July 25, 2024: contributor
    ACK 8233ee41ab9648cd0c3bd78bc2a8d692a54d9ea0. Tested on Ubuntu 22.04 using Qt version 5.15.3
  33. hebasto merged this on Jul 29, 2024
  34. hebasto closed this on Jul 29, 2024

  35. knst deleted the branch on Jul 29, 2024

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/gui. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-11 14:20 UTC

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