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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #16963 (wallet: Fix unique_ptr usage in boost::signals2 by promag)
• #16432 (qt: Add privacy to the Overview page by hebasto)

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.

Concept ACK, but let's put this in the Open Wallet sub menu. You could put a separator underneath the list of known wallets and then call the menu item "Open file...". I don't like the word "External" because it could get confusing if we add (e.g.) hardware wallet support.

@Sjors yes that was already brought up by @jnewbery and in IRC I suggested "File -> Open Wallet -> Other..." On the end of the menu.

@Sjors how about:

@Sjors how about:

Yes, this looks ideal to me

Updated.

Updated, still rebased on #15195.

tACK b537d29, but note that you can only open directory based wallets

@Sjors right, and it looks like a pain to try to open both with Qt 😕

Is this for 0.18?

Needs rebase because Open Wallet got merged. That shouldn't impact my earlier tACK, so we might be able to add this to 0.18 today (update: too late).

Needs rebase

@promag Given that this is tagged for 0.19.0, can you rebase?

It'd be good if the GUI realised when we opened a wallet that's actually inside our wallet directory via the Other... menu, otherwise you end up with the wallet loaded, but not greyed out:

Yes, different PR I guess.

@promag Thanks for the update. While testing f60f80e599284b4f098b54766e2221ebcd11f587, I reproduced the issue mentioned above on Debian 4.19.37-5+deb10u2 (2019-08-08) x86_64 GNU/Linux. Opening a wallet-less folder causes the GUI to display the error (all good) but then a second later the GUI attempts to open the wallet anyway and hangs in this state. Reproduced several times with different locations and directories.

Same GUI hang issue when opening a wallet dir moved to ~/ and wallet.dat file inside touched to be empty:

In both cases, quitting the GUI unhangs the wallet but the wallet hangs until I quit for at least several minutes.

Copying a wallet dir elsewhere with a different name but same wallet files inside, and attempting to load it in the GUI (EDIT: this is a separate issue that has been reported and a PR has been proposed):

$ qt/bitcoin-qt -testnet
terminate called after throwing an instance of 'std::runtime_error'
  what():  BerkeleyBatch: Can't open database wallet.dat (duplicates fileid c4003e0100fe0000fd9fb0c2a56b000000000000 from wallet.dat)
Aborted

Screenshot of a wallet successfully loaded after I moved it from ~/.bitcoin to ~/ with an ampersand (&) displayed correctly in the title bar and the open wallet list:

(FWIW a GUI wallet dark mode would be great).

@jonatack regarding the "duplicates fileid " crash see #16776 (comment)

@jonatack just to be sure, if you repeat the above tests with loadwallet in the console you would get the same problems?

Tested a bit.
What I think is hard to understand is that one needs to select a folder rather than a wallet.dat file.

I think either we allow to select wallet.dat files or inform the user that only wallet-directories are allowed ("select a wallet folder").

select a wallet folder

I like that.

If you make OpenExternalWalletActivity a subclass of OpenWalletActivity it would save duplicate code, which is especially nice in Open[External]WalletActivity::open() with its QTimer::singleShot.

Needs addressing comments or should otherwise be closed.

Will rebase and open in gui repo.

Tested a bit.
What I think is hard to understand is that one needs to select a folder rather than a wallet.dat file.

I think either we allow to select wallet.dat files or inform the user that only wallet-directories are allowed ("select a wallet folder").

I've tried to support selecting files and folders but this kind of sucks because in order be intuitive it needs non native file dialog - it allows to set a proxy model.

Anyway, I think the best approach, and considering sqlite wallets, selecting wallet.dat is indeed the best option for the moment.