Persist the datadir after option reset #8487

achow101 commented at 6:27 PM on August 8, 2016: member

Clicking the "Reset Options" button in "Settings" will remove all of the settings, including any custom data directory that the user sets during the first start of the software. This PR persists the datadir setting following option reset since setting the datadir afterwards can be difficult for the average user given that the only way to set the datadir after the first startup is to use the --datadir option.

MarcoFalke added the label GUI on Aug 8, 2016

achow101 force-pushed on Aug 8, 2016

jonasschnelli commented at 7:05 AM on August 9, 2016: contributor

Concept ACK. Issue with this PR: If the user passes -resetguisettings, I think we should also evict the custom set data dir.

MarcoFalke commented at 7:43 AM on August 9, 2016: member

If the user passes -resetguisettings, I think we should also evict the custom set data dir.

Which raises the question if -resetguisettings should cause the intro to pop up again (force -choosedatadir).

achow101 commented at 1:25 PM on August 9, 2016: member

Issue with this PR: If the user passes -resetguisettings, I think we should also evict the custom set data dir.

Why should it? The primary issue is that the user doesn't know the datadir has been reset. So then resetting the options should either keep the datadir or have the user choose the datadir again.

laanwj commented at 1:07 PM on August 10, 2016: member

Which raises the question if -resetguisettings should cause the intro to pop up again (force -choosedatadir).

I think it should either reset the data directory and reask with the dialog at start, or keep the data directory the same. The middle ground makes no sense.

jonasschnelli commented at 1:15 PM on August 10, 2016: contributor

I think it should either reset the data directory and reask with the dialog at start, or keep the data directory the same. The middle ground makes no sense.

Agree and I think it should re-ask with the init dialog at start.

achow101 commented at 1:26 PM on August 10, 2016: member

Ok, I will change this PR to reask for the datadir at start.

Persist the datadir after option reset

After a reset is performed, the datadir setting is saved and readded to the settings so that it is persisted across option resets.

15df3c196b

achow101 force-pushed on Aug 10, 2016

achow101 commented at 8:43 PM on August 10, 2016: member

I've updated it so that it will reask for the datadir at the start. I've also left the datadir persist stuff since it is still needed for if -resetguisettings is used.

laanwj commented at 11:46 AM on August 17, 2016: member

Separate from the changes here, we have a bug in this logic. There are two user actions that can result in a settings reset:

A1: Start bitcoin-qt with -resetguisettings. This results in app.createOptionsModel(true), which calls OptionsModel::Reset(), before going into OptionsModel::Init.
A2: Go to the options dialog and click the "Reset options" button. This calls model->Reset() and quits the application.

A2 is okay. A1 however, happens after it is no longer legal to change the data directory! (this should happen before step 6 in main).

We need to move the -resetguisettings logic before (or into) Intro::pickDataDirectory(). This will make sure that A1 and A2 act in the same, sane, way.

laanwj commented at 11:52 AM on August 17, 2016: member

Oh, shit, that won't work either.

Step 7 determines the network (testnet or mainnet)
And we want -resetguisettings to clear the options for the current network, not the mainnet ones, so doing it before step 6 would clear the wrong settings.

This initialization sequence is so damn complex because it is possible to put testnet=1 in the bitcoin.conf file, as this file is in the mainnet directory even for testnet/regtest... so it can override the real data directory used later.

achow101 commented at 12:42 PM on August 17, 2016: member

Yeah, so to deal with A1, I left in the code that keeps the datadir. So what happens is that if the user does anything to reset the options, the choose datadir window will appear and let them choose the datadir. Then, if A1 happens, the datadir is still set and will remain the same after the options are reset.

laanwj commented at 2:45 PM on August 18, 2016: member

Ok, makes sense.

Another question: what is the specific need for fReset? What makes a reset state different from just a clean slate? Both should get you into the datadir choose dialog, assuming the default datadir doesn't exist (which would be really strange if the user selected a different one).

achow101 commented at 2:51 PM on August 18, 2016: member

A reset state does not remove the datadir. A user may be using the default datadir and reset the options. We want the behavior to be consistent regardless of the datadir.

Also, it is possible that the default datadir could already exist, perhaps from moving the datadir but not deleting it, or testnet or regtest used the default datadir but mainnet does not. This would mean that it finds the datadir and thus doesn't show the choose dialog.

On August 18, 2016 10:46:05 AM "Wladimir J. van der Laan" notifications@github.com wrote:

Ok, makes sense.

Another question: what is the specific need for fReset? What makes a reset state different from just a clean slate? Both should get you into the datadir choose dialog, assuming the default datadir doesn't exist.

You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: #8487 (comment)

in src/qt/intro.cpp:None in 57b85246d3 outdated

 203 | @@ -204,6 +204,7 @@ void Intro::pickDataDirectory()
 204 |          }
 205 |  
 206 |          settings.setValue("strDataDir", dataDir);
 207 | +	settings.setValue("fReset", false);

laanwj commented at 2:52 PM on August 18, 2016:

nit: wrong indentation

jonasschnelli commented at 9:29 AM on August 24, 2016: contributor

Tested ACK https://github.com/bitcoin/bitcoin/tree/57b85246d313ac5af94cc86ed98ae4a665e40800 modulo indentation nit.

laanwj commented at 9:30 AM on August 24, 2016: member

@achow101 this seems ready to merge apart from the indentation nit ,can you please fix that?

Load choose datadir dialog after options reset 57acb82e70

achow101 force-pushed on Aug 24, 2016

achow101 commented at 1:41 PM on August 24, 2016: member

Nit addressed

jonasschnelli commented at 9:42 AM on August 25, 2016: contributor

Tested ACK 15df3c1

jonasschnelli merged this on Aug 25, 2016

jonasschnelli closed this on Aug 25, 2016

jonasschnelli referenced this in commit d26234a9e2 on Aug 25, 2016

luke-jr commented at 9:07 AM on September 10, 2016: member

Backport needed?

achow101 commented at 1:27 PM on September 10, 2016: member

Backport needed?

Is it?

MarcoFalke referenced this in commit a37cec537b on Sep 13, 2016

achow101 deleted the branch on Oct 29, 2016

DrahtBot locked this on Sep 8, 2021