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

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.

<details><summary> Pre-Changes Screenshot </summary> <img width="611" alt="Screenshot 2023-10-06 at 11 30 14" src="https://github.com/bitcoin-core/gui/assets/5377650/ca10c97d-46e8-4aed-82da-068f2afbe25c"> </details>

<details><summary> Post-Changes Screenshot </summary> <img width="729" alt="Screenshot 2023-10-06 at 11 32 58" src="https://github.com/bitcoin-core/gui/assets/5377650/f6bdcb57-646a-43d8-86a7-476e3cca683f"> </details>
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.
hebasto commented at 1:36 PM on October 6, 2023: member

@furszy Thank you!

Concept ACK.
hebasto added this to the milestone 26.0 on Oct 6, 2023
hebasto renamed this:
~~gui: remove legacy wallet creation~~
Remove legacy wallet creation
on Oct 6, 2023
hebasto added the label Wallet on Oct 6, 2023
furszy force-pushed on Oct 6, 2023
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.
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
hebasto commented at 2:29 PM on October 6, 2023: member

Approach ACK 6b83663eb8ed06216351e76439b825de4840a481.

Tested a build compiled with no SQLite support:
furszy force-pushed on Oct 6, 2023
furszy force-pushed on Oct 6, 2023
hebasto approved
hebasto commented at 4:23 PM on October 6, 2023: member

ACK 038616b57e95197a421430cb179d967043de9999
hebasto commented at 4:24 PM on October 6, 2023: member

cc @achow101 @Sjors @luke-jr
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.

Type	Reviewers
ACK	pablomartin4btc, achow101, hebasto
Stale ACK	Sjors

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.

Sjors approved
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)
gui: remove legacy wallet creation b442580ed2
furszy force-pushed on Oct 6, 2023
furszy commented at 8:38 PM on October 6, 2023: member

Updated per Sjors's feedback. Thanks.
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.
pablomartin4btc approved
pablomartin4btc commented at 9:45 PM on October 6, 2023: contributor

tACK b442580ed2a6173f0cfb86f265887d783dde3ff8
DrahtBot requested review from Sjors on Oct 6, 2023
DrahtBot requested review from hebasto on Oct 6, 2023
achow101 commented at 10:53 PM on October 6, 2023: member

ACK b442580ed2a6173f0cfb86f265887d783dde3ff8
hebasto commented at 10:45 AM on October 7, 2023: member

re-ACK b442580ed2a6173f0cfb86f265887d783dde3ff8
DrahtBot removed review request from hebasto on Oct 7, 2023
hebasto merged this on Oct 7, 2023
hebasto closed this on Oct 7, 2023
furszy deleted the branch on Oct 7, 2023
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)
bitcoin-core locked this on Oct 6, 2024

Contributors

Review Requested
Sjors

Labels

Wallet

Milestone
26.0

Linked (view graph)

#763 Remove BDB checkbox in wallet creation dialog

Remove legacy wallet creation #764

Reviews