Remove legacy wallet creation #764

pull furszy wants to merge 1 commits into bitcoin-core:master from furszy:2023_gui_remove_legacy_wallet_creation changing 6 files +77 −50
  1. furszy commented at 1:35 pm on October 6, 2023: member

    Fixes #763

    Preventing users from creating a legacy wallet prior to its deprecation in the upcoming releases.

    Note: This is still available using the createwallet RPC command.

    Future Note: Would be nice to re-write this modal as a wizard. And improve the design.

  2. DrahtBot commented at 1:35 pm on October 6, 2023: 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 pablomartin4btc, achow101, hebasto
    Stale ACK Sjors

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

  3. hebasto commented at 1:36 pm on October 6, 2023: member

    @furszy Thank you!

    Concept ACK.

  4. hebasto added this to the milestone 26.0 on Oct 6, 2023
  5. hebasto renamed this:
    gui: remove legacy wallet creation
    Remove legacy wallet creation
    on Oct 6, 2023
  6. hebasto added the label Wallet on Oct 6, 2023
  7. furszy force-pushed on Oct 6, 2023
  8. hebasto commented at 2:09 pm on October 6, 2023: member
    It would be nice to have screenshots “before” and “after” in the PR description.
  9. in src/qt/forms/createwalletdialog.ui:29 in 6b83663eb8 outdated
    24+       <horstretch>0</horstretch>
    25+       <verstretch>0</verstretch>
    26+      </sizepolicy>
    27+     </property>
    28+     <property name="text">
    29+      <string>&lt;html&gt;&lt;head/&gt;&lt;body&gt;&lt;p&gt;You are one step away from creating your new descriptors wallet!&lt;/p&gt;&lt;p&gt;Please enter the name and, if desired, enable any custom options&lt;/p&gt;&lt;/body&gt;&lt;/html&gt;</string>
    


    hebasto commented at 2:20 pm on October 6, 2023:

    HTML markups will go as a part translatable strings. Can it be avoided?

    Is it better to replace “custom options” with “advanced options” as they are named in https://github.com/bitcoin-core/gui/blob/6b83663eb8ed06216351e76439b825de4840a481/src/qt/forms/createwalletdialog.ui#L111 ?


    hebasto commented at 2:22 pm on October 6, 2023:
    And “new descriptors wallet” –> “new descriptor wallet” probably?

    furszy commented at 3:00 pm on October 6, 2023:
    thx, pushed
  10. hebasto commented at 2:29 pm on October 6, 2023: member

    Approach ACK 6b83663eb8ed06216351e76439b825de4840a481.

    Tested a build compiled with no SQLite support:

    image

  11. furszy force-pushed on Oct 6, 2023
  12. furszy force-pushed on Oct 6, 2023
  13. hebasto approved
  14. hebasto commented at 4:23 pm on October 6, 2023: member
    ACK 038616b57e95197a421430cb179d967043de9999
  15. hebasto commented at 4:24 pm on October 6, 2023: member
  16. in src/qt/forms/createwalletdialog.ui:29 in 038616b57e outdated
    24+       <horstretch>0</horstretch>
    25+       <verstretch>0</verstretch>
    26+      </sizepolicy>
    27+     </property>
    28+     <property name="text">
    29+      <string>You are one step away from creating your new descriptor wallet!</string>
    


    Sjors commented at 5:03 pm on October 6, 2023:
    This line just seems extra clutter. In any case please don’t use the scary word “descriptor”.

    achow101 commented at 7:38 pm on October 6, 2023:
    Agree that there’s no reason to mention “descriptor” here.

    furszy commented at 8:33 pm on October 6, 2023:

    Yeah sure. Word removed 👍🏼 .

    About the extra line topic: Based on my experience, the GUI should be explicative on what is going to happen (and also friendly if possible). An actionable pop up with no description and only buttons is quite bad in terms of UX (unless the design is great, which isn’t the case here).

    Still, this line is just a tiny tiny thing. I think this dialog is just bad. Would be nice to re-write it as a wizard and improve its design in the future.

  17. in src/qt/forms/createwalletdialog.ui:42 in 038616b57e outdated
    37+    </widget>
    38+   </item>
    39+   <item>
    40+    <widget class="QLabel" name="label_subdescription">
    41+     <property name="text">
    42+      <string>Please enter the name and, if desired, enable any advanced options</string>
    


    Sjors commented at 5:13 pm on October 6, 2023:
    Please provide a name …

    furszy commented at 8:33 pm on October 6, 2023:
    Done, thx.
  18. Sjors approved
  19. Sjors commented at 5:13 pm on October 6, 2023: member

    tACK 038616b57e95197a421430cb179d967043de9999

    I also checked the wallet creation error message when sqlite is not compiled (./configure --without-sqlite).

    (text can always be changed later, so don’t let my remarks block a merge)

  20. gui: remove legacy wallet creation b442580ed2
  21. furszy force-pushed on Oct 6, 2023
  22. furszy commented at 8:38 pm on October 6, 2023: member
    Updated per Sjors’s feedback. Thanks.
  23. in src/qt/forms/createwalletdialog.ui:45 in b442580ed2
    40+    <widget class="QLabel" name="label_subdescription">
    41+     <property name="text">
    42+      <string>Please provide a name and, if desired, enable any advanced options</string>
    43+     </property>
    44+    </widget>
    45+   </item>
    


    pablomartin4btc commented at 9:44 pm on October 6, 2023:

    nit: is this not implicit? Perhaps we can set it as the tool-tip of the “Create” button? Also I think extracomment attribute for the <string> tag would be needed for the translation.

  24. pablomartin4btc approved
  25. pablomartin4btc commented at 9:45 pm on October 6, 2023: contributor
    tACK b442580ed2a6173f0cfb86f265887d783dde3ff8
  26. DrahtBot requested review from Sjors on Oct 6, 2023
  27. DrahtBot requested review from hebasto on Oct 6, 2023
  28. achow101 commented at 10:53 pm on October 6, 2023: member
    ACK b442580ed2a6173f0cfb86f265887d783dde3ff8
  29. hebasto commented at 10:45 am on October 7, 2023: member
    re-ACK b442580ed2a6173f0cfb86f265887d783dde3ff8
  30. DrahtBot removed review request from hebasto on Oct 7, 2023
  31. hebasto merged this on Oct 7, 2023
  32. hebasto closed this on Oct 7, 2023

  33. furszy deleted the branch on Oct 7, 2023
  34. pablomartin4btc commented at 8:58 pm on October 7, 2023: contributor

    post-merge, error message displayed as expected when sqlite is not available (forced with ./configure --without-sqlite)

    image

  35. bitcoin-core locked this on Oct 6, 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