Bitcoin-Qt: add a Reset button to the options dialog #1685
Conversation
|
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/3ba81511f1d9409c85ec00a67fa7d5ac3d21a806 for binaries and test log. |
| @@ -167,7 +185,7 @@ QVariant OptionsModel::data(const QModelIndex & index, int role) const | |||
| case DisplayAddresses: | |||
| return QVariant(bDisplayAddresses); | |||
| case DetachDatabases: | |||
| return QVariant(bitdb.GetDetach()); | |||
| return settings.value("detachDB", false); | |||
There was a problem hiding this comment.
Didn't we agree last time that the preferences dialog shows the settings currently active. Thus, as overridden by command line options etc? In that case, this change is not valid.
There was a problem hiding this comment.
With the original code, there is no default, when doing a reset, as the detach DB checkbox always reflects the current state. This seemed like the easiest way to fix that. Have you got a better idea?
There was a problem hiding this comment.
@laanwj What do you say? Is the general idea for that reset button a good one and what about the detach thing?
There was a problem hiding this comment.
Yes I like the general idea of a reset "to factory settings" button. I don't agree with this change though as it makes detach different from the other core settings.
There was a problem hiding this comment.
I can revert this, but please consider the following:
This patch has the following behaviour:
1.
1.1. enable Detach Databases at Shutdown in settings -> Apply and close
1.2. click Reset -> Detach Databases at Shutdown get's disabled
2.
1.1. enable Detach Databases at Shutdown in settings -> Apply and close
1.2. disable Detach Databases at Shutdown in settings -> Apply and close
1.3. click Reset -> Detach Databases at Shutdown stays disabled
With current code:
1.
1.1. enable Detach Databases at Shutdown in settings -> Apply and close
1.2. click Reset -> Detach Databases at Shutdown stays enabled
2.
1.1. enable Detach Databases at Shutdown in settings -> Apply and close
1.2. disable Detach Databases at Shutdown in settings -> Apply and close
1.3. click Reset -> Detach Databases at Shutdown stays disabled
So current code is special and IMHO a user expects it to be the way after this patch, no?
There was a problem hiding this comment.
@laanwj Alright, as you did not respond, let me re-phrase, are there any further comments left, when I revert that single change :)?
|
@Diapolo my time is pretty limited, and I haven't found time to test and look at this in further detail yet. It will be a while before 0.8.0 is released, I guess, so there's no need to hurry. |
|
@laanwj I found a method to achieve what I wanted, without the need to change the detachDB stuff in OptionsModel::data(). |
|
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/35ff85be9d53d082c4748472dd87fd98572f8798 for binaries and test log. |
|
Updated and removed special casing for detachDB as that was removed entirely. |
|
Does this correctly handle the Cancel case? That is, if I Reset and Cancel, no changes should be made (and making changes should require OK/Apply). |
|
No, when you use reset button, this is a non-reversible action. It's IMO not worth the trouble (perhaps not even possible the way this is working), but I could add a warning message where you need to confirm the process, would this be sufficient then? Making changes DOES already require OK/Apply. |
|
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/92ffc1b5a9dbebc281da38782d99fb357ba77f15 for binaries and test log. |
|
What is the status on this? |
|
I still think it's a usability bug for it to conflict with the Cancel action, but I don't really care enough to argue over it. |
- a click on "Reset Options" sets all options to the default values by removing all stored settings (QSettings), loading the defaults and saving them as the new settings - before the reset is executed the user is presented a confirmation dialog - special casing was needed for StartAtStartup
|
Updated to include a confirmation dialog before executing the reset. |
|
Much better with confirmation dialog, ACK |
Bitcoin-Qt: add a Reset button to the options dialog
Bitcoin-Qt: add a Reset button to the options dialog
…ons. (bitcoin#1685) 1ab21cf Remove bogus assert on number of oubound connections. (Matt Corallo)
removing all stored settings (QSettings), loading the defaults and
saving them as the new settings