qt: Backup former GUI settings on `-resetguisettings` #11338

laanwj · 2017-09-15T09:57:19Z

Writes the GUI settings to guisettings.bak in the data directory before wiping them. This can be used to retroactively troubleshoot issues (e.g. #11262) where -resetguisettings solves the problem.
(as discussed in yesterday's IRC meeting)

meshcollider

utACK 232a108, really good idea 👍

Couple of small nits

meshcollider · 2017-09-15T10:00:39Z

src/qt/optionsmodel.cpp

nit, GetDataDir() argument defaults to true so it'd be clearer to leave it out imo

Isn't it the other way around: clearer to leave it in, because it's explicit?

Normally I'd say yes, but in this case it just leaves me wondering what the argument is doing, because in most other cases of GetDataDir in the code, the argument is left out. Personal preference

During the initialization sequence it's really important to get this right. E.g. I had to check the order in src/qt/bitcoin.cpp that calling GetDataDir(true) was allowed at this point.

Agree that boolean arguments are unclear, so are default arguments, so it's kind of choosing between evils here. Instead of an argument to GetDataDir we should have done GetSpecificDataDir and GetRootDataDir or such. Anyhow... I don't think this code is particularly unclear, I'll add 'to chain-specific datadir' in the comment.

meshcollider · 2017-09-15T10:01:44Z

src/qt/optionsmodel.cpp

nit, space between &key and the colon

meshcollider · 2017-09-15T10:14:37Z

re-utACK ~~bb37a03~~ 4e9dc85

laanwj · 2017-09-15T10:56:16Z

Travis was failing. Changed qInfo to qWarning, as qInfo was only introduced in Qt 5.5 and the result is the same anyhow: it unconditionally ends up in debug.log.

laanwj · 2017-09-15T13:17:48Z

Added a dst.clear (suggested by @promag) to avoid the edge case of older settings staying behind.

promag

utACK modulus nits.

promag · 2017-09-15T13:31:50Z

src/qt/optionsmodel.cpp

+/** Helper function to copy contents from one QSettings to another.
+ * By using allKeys this also covers nested settings in a hierarchy.
+ */
+static void copySettings(QSettings &dst, const QSettings &src)


Don't see the benefit of this function, move the code to backupSettings()? Otherwise & next to type name.

The point is that this is a general function, it could be also e.g. used to restore QSettings from a backup, if that is ever necessary. Would prefer to keep it. Better too much factoring than too little.

Ok then, just update to CopySettings and move &.

promag · 2017-09-15T13:32:44Z

src/qt/optionsmodel.cpp

+}
+
+/** Back up a QSettings to an ini-formatted file. */
+static void backupSettings(const fs::path &filename, const QSettings &src)


Rename to BackupSettings and & next to type name.

promag · 2017-09-15T13:33:36Z

src/qt/optionsmodel.cpp

+ */
+static void copySettings(QSettings &dst, const QSettings &src)
+{
+    for (const QString &key : src.allKeys()) {


& next to type name.

promag · 2017-09-15T13:35:11Z

doc/files.md

 * wallet.dat: personal wallet (BDB) with keys and transactions
 * .cookie: session RPC authentication cookie (written at start when cookie authentication is used, deleted on shutdown): since 0.12.0
 * onion_private_key: cached Tor hidden service private key for `-listenonion`: since 0.12.0
+* guisettings.bak: backup of former GUI settings after `-resetguisettings` is used


.ini.bak?

Yeah why not...

Missing doc update.

TheBlueMatt · 2017-09-15T21:18:50Z

Tested ACK f725dbb, needs squash obviously, didn't bother to try to review in-depth for whether this is going to put the guisettings.bak file in a strange datadir (bitcoin.conf-based? the old GUI settings one?) but there isnt a "correct" answer there anyway, so I cant say I care much.

sipa · 2017-09-15T23:26:45Z

Concept ACK

jonasschnelli · 2017-09-15T23:30:45Z

utACK f725dbb

Since this uses QSettings::IniFormat, there is no easy way how to restore a such backup. Is the backup file only intended to use for inspection of old values or also for a full restore? The later would very likely require a restore function (QSettings::IniFormat to native format or similar).

laanwj · 2017-09-16T05:04:54Z

didn't bother to try to review in-depth for whether this is going to put the guisettings.bak file in a strange datadir (bitcoin.conf-based? the old GUI settings one?)

I think that's handled, it puts it in the new datadir that you have to select when using -resetguisettings.

Since this uses QSettings::IniFormat, there is no easy way how to restore a such backup. Is the backup file only intended to use for inspection of old values or also for a full restore?

Not opposed to restore functionality if there is a pressing use-case, although the intended purpose (as mentioned in OP) is diagnostics. This is much easier with .ini format. Also windows' NativeFormat cannot be exported to an arbitrary file just to other registry paths.

promag

Tested ACK. It works even if the file already exists in either INI or other format (it's always overwritten).

There are still some code nits. One small and final thing, for me this is more of a dump than backup.

achow101 · 2017-09-16T16:19:02Z

there is no easy way how to restore a such backup.

I don't think there needs to be a way to restore such a backup. This backup is only created when you run -resetguisettings which implies that the settings are corrupted anyways and you don't want a backup of something that's corrupted.

promag · 2017-09-16T16:22:54Z

which implies that the settings are corrupted

Or settings are fine and there is a bug?

achow101 · 2017-09-16T16:43:03Z

tACK b28af38

jonasschnelli · 2017-09-18T05:07:23Z

Found a strange issue while testing this.
Compiling locally on OSX with MacOS 10.12.6 with Qt5.9.0 works perfect (guisettings.ini.bak is present after running with -resetguisettings).
However, using the gtitian build (https://bitcoin.jonasschnelli.ch/build/312) will result in not creating the guisettings.ini.bak file. Not sure if it is a local issue but would be good if someone could test this (via gitian)

maflcko · 2017-09-18T10:06:07Z

@jonasschnelli The embedded commit hash does not match the advertised commit hash on the website of the downloaded gitian binaries.

jonasschnelli · 2017-09-19T03:57:45Z

@MarcoFalke: the commit hash is different because my build script builds it on top of master. I just did everything again and the gtitian version does not create the backup file.
https://bitcoin.jonasschnelli.ch/build/315
(Maybe someone should double-check via an gitian build).

achow101 · 2017-09-19T04:54:00Z

@jonasschnelli I performed my own gitian build and tested it but could not reproduce your issue. It will create guisettings.ini.bak as it should.

I did notice that if you did not have any gui settings to begin with, the file will not be created. Perhaps that's what you are seeing?

jonasschnelli · 2017-09-19T14:08:02Z

@achow101: even with enabling coincontrol I had no success on OSX.
What OS did you test? It may be a platform issue?

achow101 · 2017-09-19T14:19:35Z

@jonasschnelli I am using Ubuntu 17.04

laanwj · 2017-09-19T14:29:21Z

I did notice that if you did not have any gui settings to begin with, the file will not be created. Perhaps that's what you are seeing?

Hm, that's interesting, haven't thought about that. I think that behavior makes sense, though.

gmaxwell

ut(not very useful at reviewing QT stuff)ACK.

Writes the GUI settings to `guisettings.bak` in the data directory before wiping them. This can be used to retroactively troubleshoot issues (e.g. bitcoin#11262) where `-resetguisettings` solves the problem.

laanwj · 2017-09-23T07:39:45Z

Squashed 4e9dc85 f725dbb 0f29983 b28af38 → 723aa1b

723aa1b qt: Backup former GUI settings on `-resetguisettings` (Wladimir J. van der Laan) Pull request description: Writes the GUI settings to `guisettings.bak` in the data directory before wiping them. This can be used to retroactively troubleshoot issues (e.g. #11262) where `-resetguisettings` solves the problem. (as discussed in yesterday's IRC meeting) Tree-SHA512: c64f5052d992eb02057ba285435f143c42d0cc456144a4c565e1c87be833737f9df750d0aee10810f85047c820d9b4f9f22fd94a6f09f4b28a9cf41b63a56586

Writes the GUI settings to `guisettings.bak` in the data directory before wiping them. This can be used to retroactively troubleshoot issues (e.g. bitcoin#11262) where `-resetguisettings` solves the problem. Github-Pull: bitcoin#11338 Rebased-From: 723aa1b

Summary: 723aa1b qt: Backup former GUI settings on `-resetguisettings` (Wladimir J. van der Laan) Pull request description: Writes the GUI settings to `guisettings.bak` in the data directory before wiping them. This can be used to retroactively troubleshoot issues (e.g. #11262) where `-resetguisettings` solves the problem. (as discussed in yesterday's IRC meeting) Tree-SHA512: c64f5052d992eb02057ba285435f143c42d0cc456144a4c565e1c87be833737f9df750d0aee10810f85047c820d9b4f9f22fd94a6f09f4b28a9cf41b63a56586 Backport of Core PR11338 bitcoin/bitcoin#11338 Test Plan: make check ./bitcoin-qt -resetguisettings guisettings.ini.bak should exist in your .bitcoin dir Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc Reviewed By: jasonbcox, O1 Bitcoin ABC, #bitcoin_abc Differential Revision: https://reviews.bitcoinabc.org/D3934

…ings` 723aa1b qt: Backup former GUI settings on `-resetguisettings` (Wladimir J. van der Laan) Pull request description: Writes the GUI settings to `guisettings.bak` in the data directory before wiping them. This can be used to retroactively troubleshoot issues (e.g. bitcoin#11262) where `-resetguisettings` solves the problem. (as discussed in yesterday's IRC meeting) Tree-SHA512: c64f5052d992eb02057ba285435f143c42d0cc456144a4c565e1c87be833737f9df750d0aee10810f85047c820d9b4f9f22fd94a6f09f4b28a9cf41b63a56586

Summary: 723aa1b qt: Backup former GUI settings on `-resetguisettings` (Wladimir J. van der Laan) Pull request description: Writes the GUI settings to `guisettings.bak` in the data directory before wiping them. This can be used to retroactively troubleshoot issues (e.g. #11262) where `-resetguisettings` solves the problem. (as discussed in yesterday's IRC meeting) Tree-SHA512: c64f5052d992eb02057ba285435f143c42d0cc456144a4c565e1c87be833737f9df750d0aee10810f85047c820d9b4f9f22fd94a6f09f4b28a9cf41b63a56586 Backport of Core PR11338 bitcoin/bitcoin#11338 Test Plan: make check ./bitcoin-qt -resetguisettings guisettings.ini.bak should exist in your .bitcoin dir Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc Reviewed By: jasonbcox, O1 Bitcoin ABC, #bitcoin_abc Differential Revision: https://reviews.bitcoinabc.org/D3934

laanwj added GUI Needs backport labels Sep 15, 2017

laanwj added this to the 0.15.1 milestone Sep 15, 2017

meshcollider reviewed Sep 15, 2017

View reviewed changes

laanwj force-pushed the 2017_10_backup_resetguisettings branch 2 times, most recently from 3141a2f to bb37a03 Compare September 15, 2017 10:12

laanwj force-pushed the 2017_10_backup_resetguisettings branch from bb37a03 to 4e9dc85 Compare September 15, 2017 10:55

promag reviewed Sep 15, 2017

View reviewed changes

promag reviewed Sep 16, 2017

View reviewed changes

gmaxwell approved these changes Sep 23, 2017

View reviewed changes

qt: Backup former GUI settings on -resetguisettings

723aa1b

Writes the GUI settings to `guisettings.bak` in the data directory before wiping them. This can be used to retroactively troubleshoot issues (e.g. bitcoin#11262) where `-resetguisettings` solves the problem.

laanwj force-pushed the 2017_10_backup_resetguisettings branch from b28af38 to 723aa1b Compare September 23, 2017 07:39

laanwj merged commit 723aa1b into bitcoin:master Sep 23, 2017

maflcko removed the Needs backport label Oct 4, 2017

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

qt: Backup former GUI settings on -resetguisettings #11338

qt: Backup former GUI settings on -resetguisettings #11338

Uh oh!

Conversation

laanwj commented Sep 15, 2017

Uh oh!

meshcollider left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meshcollider commented Sep 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Sep 15, 2017

Uh oh!

laanwj commented Sep 15, 2017

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Sep 15, 2017

Uh oh!

sipa commented Sep 15, 2017

Uh oh!

jonasschnelli commented Sep 15, 2017

Uh oh!

laanwj commented Sep 16, 2017

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 commented Sep 16, 2017

Uh oh!

promag commented Sep 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achow101 commented Sep 16, 2017

Uh oh!

jonasschnelli commented Sep 18, 2017

Uh oh!

maflcko commented Sep 18, 2017

Uh oh!

jonasschnelli commented Sep 19, 2017

Uh oh!

achow101 commented Sep 19, 2017

Uh oh!

jonasschnelli commented Sep 19, 2017

Uh oh!

achow101 commented Sep 19, 2017

Uh oh!

laanwj commented Sep 19, 2017

Uh oh!

gmaxwell left a comment

qt: Backup former GUI settings on `-resetguisettings` #11338

qt: Backup former GUI settings on `-resetguisettings` #11338

meshcollider commented Sep 15, 2017 •

edited

Loading

promag commented Sep 16, 2017 •

edited

Loading