qt: Add -guisettingsdir option #17636

emilengler commented at 12:12 AM on November 30, 2019: contributor

Short Description

This PR makes it possible to change the Qt relevant config files in a custom directory

Motivation

There are multiple usecases where it could be useful, for example when you use Bitcoin Core on a USB stick, you can keep the config files there as well. My motivation was that I don't need to move my production bitcoin-qt config file to a different place because I don't want that an unstable bulid corrupts my datadir. Currently I need to delete my config or reset the datadir path every time I switch between productive to development. With this I can have a config directory for my productive use and one for my development use.

Showcase

Having a directory called /home/emil/testconf and running bitcoin-qt will result in this hier: /home/emil/testconf/Bitcoin/Bitcoin-Qt.conf. The Bitcoin subdirectory is a feature, not a bug to make it possible to keep other Qt config files there without colliding. If the the user has no permissions, it won't save anything, if the dir does not exists it will be created.

fanquake added the label GUI on Nov 30, 2019

fanquake requested review from ryanofsky on Nov 30, 2019

in src/qt/bitcoin.cpp:49 in 96a1cafea1 outdated

  45 | @@ -46,6 +46,7 @@
  46 |  #include <QThread>
  47 |  #include <QTimer>
  48 |  #include <QTranslator>
  49 | +#include <QString>

hebasto commented at 6:07 AM on November 30, 2019:

nit: Could you apply clang-format-diff.py to your commit?

in src/qt/bitcoin.cpp:397 in 96a1cafea1 outdated

 393 | @@ -393,6 +394,7 @@ WId BitcoinApplication::getMainWinId() const
 394 |  static void SetupUIArgs()
 395 |  {
 396 |      gArgs.AddArg("-choosedatadir", strprintf("Choose data directory on startup (default: %u)", DEFAULT_CHOOSE_DATADIR), ArgsManager::ALLOW_ANY, OptionsCategory::GUI);
 397 | +    gArgs.AddArg("-guisettings=<path>", "Choose a custom data direcotry especially for the Qt Settings", ArgsManager::ALLOW_ANY, OptionsCategory::GUI);

hebasto commented at 6:09 AM on November 30, 2019:

typo: s/direcotry/directory/

hebasto commented at 6:10 AM on November 30, 2019: member

Concept ACK 96a1cafea1b63f750b960ed82279da0be01fd135

in src/qt/bitcoin.cpp:471 in 96a1cafea1 outdated

 467 | @@ -466,6 +468,10 @@ int GuiMain(int argc, char* argv[])
 468 |      QApplication::setOrganizationName(QAPP_ORG_NAME);
 469 |      QApplication::setOrganizationDomain(QAPP_ORG_DOMAIN);
 470 |      QApplication::setApplicationName(QAPP_APP_NAME_DEFAULT);
 471 | +    if (gArgs.IsArgSet("-guisettings") && gArgs.GetArg("-guisettings", "") != "") {

hebasto commented at 6:22 AM on November 30, 2019:

Is gArgs.IsArgSet("-guisettings") needed here?

paymog commented at 9:03 AM on November 30, 2019:

Is it possible to test this logic in UTs?

emilengler commented at 1:52 PM on November 30, 2019:

You're right, forgot that

emilengler commented at 2:12 PM on November 30, 2019:

Good question. I'm not an expert with the test framework but we could run bitcoin-qt --guisettings=/a/custom/path/ and see if /a/custom/path/Bitcoin/Bitcoin-Qt.conf exists. The problem with the Qt tests is that they test the actual GUI like clicking buttons. This belongs more to a startup test, don't know if this exists

emilengler force-pushed on Nov 30, 2019

in src/qt/bitcoin.cpp:476 in 9336f4cf6e outdated

 467 | @@ -466,6 +468,10 @@ int GuiMain(int argc, char* argv[])
 468 |      QApplication::setOrganizationName(QAPP_ORG_NAME);
 469 |      QApplication::setOrganizationDomain(QAPP_ORG_DOMAIN);
 470 |      QApplication::setApplicationName(QAPP_APP_NAME_DEFAULT);
 471 | +    if (gArgs.GetArg("-guisettings", "") != "") {
 472 | +        QSettings::setDefaultFormat(QSettings::IniFormat);

hebasto commented at 2:23 PM on November 30, 2019:

QSettings::IniFormat:

... Note that type information is not preserved when reading settings from INI files; all values will be returned as QString.

Could this cause any issue?

emilengler commented at 2:29 PM on November 30, 2019:

Good question. On UNIX it is always in IniFormat. On macOS and Windows it is in the registry (even if that's not the term on macOS). What would be the point in selecting a custom path to the registry? I don't know if this is even possible.

emilengler commented at 11:39 PM on November 30, 2019:

I think this as resolved as it is not possible (at least not by ease) to store registry stuff outside of the registry. IniFormat is the only cross platform settings format supported by Qt and it is only used with if the parameter is set.

jonasschnelli commented at 8:54 PM on December 1, 2019: contributor

I prefer #15936 in the long run. Maybe this makes sense as an intermediary step... but unsure (could also be a maintenance burden since this needs to go away with #15936 very likely). Pinging @ryanofsky.

emilengler commented at 9:19 PM on December 1, 2019: contributor

@jonasschnelli #15936 is something different. -guisettings is also intended to be the Qt relevant equivalent of bitcoind's -conf

in doc/files.md:84 in 9336f4cf6e outdated

  80 | @@ -81,6 +81,7 @@ Subdirectory | File(s)           | Description
  81 |  ## GUI settings
  82 |  
  83 |  `bitcoin-qt` uses [`QSettings`](https://doc.qt.io/qt-5/qsettings.html) class; this implies platform-specific [locations where application settings are stored](https://doc.qt.io/qt-5/qsettings.html#locations-where-application-settings-are-stored).
  84 | +You can change this with the `-guisettings=<path>` option.

promag commented at 9:39 PM on December 1, 2019:

Could mention that <path> is a directory where the .ini file is stored.

emilengler commented at 4:17 PM on December 2, 2019:

ACK

in src/qt/bitcoin.cpp:473 in 9336f4cf6e outdated

 467 | @@ -466,6 +468,10 @@ int GuiMain(int argc, char* argv[])
 468 |      QApplication::setOrganizationName(QAPP_ORG_NAME);
 469 |      QApplication::setOrganizationDomain(QAPP_ORG_DOMAIN);
 470 |      QApplication::setApplicationName(QAPP_APP_NAME_DEFAULT);
 471 | +    if (gArgs.GetArg("-guisettings", "") != "") {
 472 | +        QSettings::setDefaultFormat(QSettings::IniFormat);
 473 | +        QSettings::setPath(QSettings::IniFormat, QSettings::UserScope, QString::fromStdString(gArgs.GetArg("-guisettings", "")));

promag commented at 9:44 PM on December 1, 2019:

This should have some check, could be an invalid or read only path and then it wouldn't work as expected.

emilengler commented at 4:19 PM on December 2, 2019:

Sure that is that important? If the path does not exists it will create the directory recursively, if the user has no permission it will fallback to the default. Don't know if it is that much required to check for writing privileges.

promag commented at 9:44 PM on December 1, 2019: member

I understand the OP motivation, not sure if the new argument is the best. Specifying a directory gives the impression that multiple files could be stored there.

How about supporting specifying the filename like =my_settings_1.ini? This could be an absolute path or relative to datadir?

ryanofsky commented at 10:38 PM on December 1, 2019: member

Concept ACK. This seems at least marginally useful and I don't see drawbacks. It could use better documentation and error checking though. Documentation: should specified path be a file or directory path, or are both accepted? Error checking: it'd be nice to trigger InitErrors if specified location can't be opened or isn't writeable. It'd also be nice to trigger an init error if guisettings=path is specified in the bitcoin.conf file since right now it looks like that is silently ignored.

I'm also not sure if there are caveats to using QSettings::IniFormat instead of QSettings::NativeFormat. According to https://doc.qt.io/qt-5/qsettings.html#Format-enum, "Note that type information is not preserved when reading settings from INI files; all values will be returned as QString." So it'd be worth looking into whether thing there might be subtle bugs from using this option. I do see some types used in my current config file like MainWindowGeometry=@ByteArray(...)

re: #17636 (comment)

This PR is compatible with #15936, but #15936 will make it less practically useful because #15936 moves shared bitcoind / bitcoin-qt settings like pruning and proxy settings out of gui-only QSettings storage which is ignored by bitcoind into shared datadir storage used by both bitcoind and bitcoin-qt.

But even after #15936, QSettings storage does not go away. It still used for GUI-only settings like window positions, coin control modes, and the default datadir location. Longer term I think it makes sense to store as many settings as possible in the datadir and avoid using QSettings. But I think QSettings storage might never go away entirely, because it does give GUI users a simple way to choose a custom datadir location, and obviously the datadir location can't be stored in the datadir.

emilengler commented at 4:17 PM on December 2, 2019: contributor

I understand the OP motivation, not sure if the new argument is the best. Specifying a directory gives the impression that multiple files could be stored there.

That's a design choice. Other files should be stored there like Bitcoin-Qt-regtest.conf (forgot the actual filename, but I mean the other chain configs).

emilengler force-pushed on Dec 2, 2019

emilengler commented at 4:37 PM on December 2, 2019: contributor

It'd also be nice to trigger an init error if guisettings=path is specified in the bitcoin.conf file since right now it looks like that is silently ignored.

Is it even possible to add GUI related stuff to the bitcoin.conf? Also the -guisettings parsing needs to be done as one of the first operations. As the translation system is going to access QSettings for the language string.

I'm also not sure if there are caveats to using QSettings::IniFormat instead of QSettings::NativeFormat. According to https://doc.qt.io/qt-5/qsettings.html#Format-enum, "Note that type information is not preserved when reading settings from INI files; all values will be returned as QString." So it'd be worth looking into whether thing there might be subtle bugs from using this option. I do see some types used in my current config file like MainWindowGeometry=@ByteArray(...)

On UNIX system it just works fine as QSettings::IniFormat is the QSettings::NativeFormat. If you specify -guisettings, it will always use QSettings::IniFormat as it is nearly impossible to store the windows registry in one portable file.

ryanofsky commented at 5:08 PM on December 2, 2019: member

It'd also be nice to trigger an init error if guisettings=path is specified in the bitcoin.conf file since right now it looks like that is silently ignored.

Is it even possible to add GUI related stuff to the bitcoin.conf?

No, but that isn't obvious. I'm suggesting it should trigger an init error unless that's difficult, because most command line arguments do work in the config file and the fact that this is setting is silently ignored is just a quirk of the current implementation, and not something I was expecting, or would expect a user to know about.

I think it would be pretty easy to add an error with a new COMMAND_LINE_ONLY flag to complement existing DEBUG_ONLY and NETWORK_ONLY flags:

https://github.com/bitcoin/bitcoin/blob/35eda631ed3bd23d4a41761a85a96f925d4a6337/src/util/system.h#L141-L147

I'm also not sure if there are caveats to using QSettings::IniFormat instead of QSettings::NativeFormat. According to https://doc.qt.io/qt-5/qsettings.html#Format-enum, "Note that type information is not preserved when reading settings from INI files; all values will be returned as QString." So it'd be worth looking into whether thing there might be subtle bugs from using this option. I do see some types used in my current config file like MainWindowGeometry=@bytearray(...)

On UNIX system it just works fine as QSettings::IniFormat is the QSettings::NativeFormat. If you specify -guisettings, it will always use QSettings::IniFormat as it is nearly impossible to store the windows registry in one portable file.

I'll take your word for it but this seems to contradict the documentation which mentions differences between IniFormat and NativeFormat on unix, like the fact that they use different file extensions, and warns about IniFormat returning everything as a strings. But if there's no issue that's great.

emilengler commented at 6:45 PM on December 2, 2019: contributor

@ryanofsky Indeed the file extensions are different but the files are the same so it doesn't matter:

$ sha256sum testdir/Bitcoin/Bitcoin-Qt.ini
d00cb5f6ef6183e9752e56bbd13a95b31f39d7ba5a82933449d8bdf2576060f4  testdir/Bitcoin/Bitcoin-Qt.ini
 $ sha256sum .config/Bitcoin/Bitcoin-Qt.conf
d00cb5f6ef6183e9752e56bbd13a95b31f39d7ba5a82933449d8bdf2576060f4  .config/Bitcoin/Bitcoin-Qt.conf

ryanofsky commented at 7:31 PM on December 2, 2019: member

@ryanofsky Indeed the file extensions are different but the files are the same so it doesn't matter:

Question is how values are read, not really how they are written. e.g. is @bytearray(...) read as an actual byte array or as the string "@bytearray(...)"

in src/qt/bitcoin.cpp:397 in feed664436 outdated

 393 | @@ -393,6 +394,7 @@ WId BitcoinApplication::getMainWinId() const
 394 |  static void SetupUIArgs()
 395 |  {
 396 |      gArgs.AddArg("-choosedatadir", strprintf("Choose data directory on startup (default: %u)", DEFAULT_CHOOSE_DATADIR), ArgsManager::ALLOW_ANY, OptionsCategory::GUI);
 397 | +    gArgs.AddArg("-guisettings=<path>", "Choose a custom data directory especially for the Qt Settings", ArgsManager::ALLOW_ANY, OptionsCategory::GUI);

laanwj commented at 9:00 AM on December 3, 2019:

Note that due to initialization order constraints, this option can only be provided on the command line. It won't work in the configuration file because at that time, QSettings was already parsed.

emilengler commented at 5:58 PM on December 4, 2019:

I think you are wrong. Take a look at src/qt/bitcoin.cpp line 451. There you'll see that SetupUIArgs() is executed before the PR feature.

ryanofsky commented at 6:45 PM on December 4, 2019:

re: #17636 (review)

I concluded the same as Wladimir based on the fact that readConfigFiles is called after GetArg("-guisettings"), so presumably it could not affect the result of the earlier call.

Talkless commented at 6:47 PM on December 6, 2019:

I would suggest to call it -guisettingsdir, to be consistent with other *dir's options we have. Currently, it makes me fee as I am expected to write full path to the .ini file itself, though this is not the case, it's the parent(-parent) directory..

emilengler commented at 12:47 AM on December 7, 2019:

I think path value suggestion says that it is a path to a dir. Otherwise it would be <file>

Talkless commented at 9:14 AM on December 7, 2019:

"path" can mean file path or directory path.

A path, the general form of the name of a file or directory...

https://en.wikipedia.org/wiki/Path_(computing)

This is not hard argument but simply I dont't see the reason to make this option stand out from others. We have bunch of *dir options, not sure why this should not have same suffix.

Talkless commented at 2:48 PM on December 8, 2019:

@ryanofsky @laanwj what's your view about -guisettings vs -guisettingsdir (more consistent) ?

ryanofsky commented at 7:45 PM on December 16, 2019:

@ryanofsky @laanwj what's your view about -guisettings vs -guisettingsdir (more consistent) ?

If it actually is required to be a filename not a directory name then dir suffix could be better, but from the qt documentation, I got the impression that it would accept either an ini file path or a directory path. I keep asking for more documentation in every comment I make on this PR. I just think whatever the behavior is it should be documented.

Talkless commented at 6:15 PM on December 17, 2019:

@ryanofsky:

if I pass -guisettings=/tmp/mysettings, it creates /tmp/mysettings/Bitcoin/Bitcoin-Qt-regtest.ini file.
if I pass -guisettings=/tmp/mysettings.ini (file nor directory does not exist), it creates /tmp/mysettings.ini/Bitcoin/Bitcoin-Qt-regtest.ini
if I pass -guisettings=/tmp/mysettings.ini after touch /tmp/mysettings.ini, no directories are created, but neither mysettings.ini is used as settings file - it's empty. Looks like a silent failure.

So it seems that for Bitcoin it works as a parent directory, as Bitcoin subdirectory is always tried to be created within specified guisettings path.

ryanofsky commented at 6:23 PM on December 17, 2019:

Thanks for testing! If that's the case, I think it'd be good to add the dir suffix, document that the path is supposed to be a directory, and also adding error checking for the silent failure case. Error checking could probably be done by calling QSettings::sync() and checking QSettings::status() returns QSettings::NoError.

emilengler commented at 2:24 PM on December 18, 2019:

Ok, will switch to -guisettingsdir

laanwj commented at 9:02 AM on December 3, 2019: member

Concept ~0 on this. If there are actual users that have use for this, I'm okay with it (it doesn't add that much complexity), but, it's use is pretty constrained and this is heavily initialization-order dependent, and yet another thing to test when testing setting reading.

laanwj commented at 9:02 AM on December 3, 2019: member

This definitely needs a test of some kind.

fanquake added the label Waiting for author on Dec 3, 2019

emilengler commented at 6:06 PM on December 4, 2019: contributor

@laanwj Maybe I overlooked it but I haven't found any Qt tests which test CLI args. Is it really that worth to implement a complete new system just for that?

ryanofsky commented at 7:00 PM on December 4, 2019: member

Is it really that worth to implement a complete new system just for that?

I think it would be great to have a new test for this, not just for this feature but also because the framework could be extended to verify other init behavior. But you are right that there isn't currently an easy way to write tests covering the GuiMain function, so I personally don't think one needs to be added in this PR.

Perhaps the easiest test to write would be a python test invoking bitcoin-qt with QT_QPA_PLATFORM=minimal and -guisettings=path, calling the stop rpc right away, and checking for the ini file in the filesystem. I don't think it would be too hard to write a C++ unit test either, but it'd probably require breaking up the GuiMain function which might be overkill here.

My main feedback for this PR is still to add better error checking and documentation: #17636 (comment)

Talkless commented at 6:31 PM on December 6, 2019: none

Concept ACK. Looks useful.

Built & tested a bit - changing values to switch between two .ini files does change language, window position, listening, etc.

in src/qt/bitcoin.cpp:471 in feed664436 outdated

 467 | @@ -466,6 +468,10 @@ int GuiMain(int argc, char* argv[])
 468 |      QApplication::setOrganizationName(QAPP_ORG_NAME);
 469 |      QApplication::setOrganizationDomain(QAPP_ORG_DOMAIN);
 470 |      QApplication::setApplicationName(QAPP_APP_NAME_DEFAULT);
 471 | +    if (gArgs.GetArg("-guisettings", "") != "") {

Talkless commented at 6:45 PM on December 6, 2019:

nit: is it really necessary to call GetArg twice? We could take const std::string gui_settings = ... and use it twice. Pity we can't use C++17 if statement with initializer though...

nit2: it's better to use ! ... .empty() , often suggested by static analyzers. Less code bloat, no need to actually compare against const char*...

emilengler commented at 12:46 AM on December 7, 2019:

nit2: it's better to use ! ... .empty() , often suggested by static analyzers. Less code bloat, no need to actually compare against const char*...

Will add that

nit: is it really necessary to call GetArg twice? We could take const std::string gui_settings = ... and use it twice. Pity we can't use C++17 if statement with initializer though...

Will add that as well thanks

emilengler force-pushed on Dec 7, 2019

emilengler force-pushed on Dec 18, 2019

emilengler renamed this:
~~qt: Add -guisettings option~~
qt: Add -guisettingsdir option
on Dec 18, 2019

emilengler commented at 10:01 PM on December 23, 2019: contributor

Push, as I changed the title and added the dir suffix

DrahtBot commented at 1:49 AM on December 24, 2019: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#19461 (multiprocess: Add bitcoin-gui -ipcconnect option by ryanofsky)

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.

luke-jr approved

luke-jr commented at 5:57 PM on January 3, 2020: member

utACK

luke-jr referenced this in commit 1505b1d1b7 on Jan 5, 2020

fanquake removed the label Waiting for author on Jan 5, 2020

Talkless commented at 12:17 PM on January 5, 2020: none

I guess there's two questions left - should it really need test (https://github.com/bitcoin/bitcoin/pull/17636#issuecomment-561067975) and what about error handling in case path is invalid and settings file cannot be created?

emilengler commented at 3:48 PM on January 5, 2020: contributor

what about error handling in case path is invalid and settings file cannot be created?

I explained this in the showcase. If the config file can't be created for whatever reason, it won't save anything. However the user should notice this somehow at least when he restarts bitcoin-qt the next time.

Talkless commented at 7:33 PM on February 4, 2020: none

notice this somehow

I doubt this is good enough. Consider this setup:

$ fgrep bPrune ~/.config/Bitcoin/Bitcoin-Qt-testnet.conf 
bPrune=false

$ fgrep bPrune /tmp/myconfig/Bitcoin/Bitcoin-Qt-testnet.ini 
bPrune=true

Now imagine myconfig dir "disappears", like, forgot to plug-in USB drive, network drive not mounted, or whatever. Bitoinc-Qt will silently start downloading full chain due to old config, possibly filling users drive...

Consider taking @ryanofsky suggestion about using QSettings::sync().

emilengler commented at 7:59 PM on February 4, 2020: contributor

Now imagine myconfig dir "disappears", like, forgot to plug-in USB drive, network drive not mounted, or whatever. Bitoinc-Qt will silently start downloading full chain due to old config, possibly filling users drive...

No, bitcoin-qt doesn't necessarily know where the datadir is and the intro screen opens again. Also I wouldn't call this a direct issue of this PR...

Talkless commented at 7:06 PM on February 5, 2020: none

datadir

This PR is not about data dir (~/.bitcoin), but Qt settings dir (~/.config/Bitcoin). If you remove data dir - yes, wizard is show. But not when -guisettingsdir is missing. Try this:

mkdir /tmp/myconfig
bicoin-qt -testnet -guisettingsdir=/tmp/mysettings
# ... enable pruning in `Settings -> Options` window ..
# ... quit bitcoin-qt ...
mv /tmp/mysettings /tmp/mysettings_removed
bicoin-qt -testnet -guisettingsdir=/tmp/mysettings
# ... go to `Options` again, see pruning quietly disabled...

emilengler commented at 8:03 PM on February 5, 2020: contributor

I was talking about the Qt settings. The data dir path is set in the Qt settings. Sorry if you understood it wrong

jonasschnelli commented at 7:55 AM on February 6, 2020: contributor

utACK 5266efa964b1f29b2c334e7a942a664d335c8e18

Talkless commented at 5:35 PM on February 6, 2020: none

The data dir path is set in the Qt settings

It doesn't matter, you can remove strDataDir=/home/vincas/.bitcoin line from it and it will still use this path as default.

Anyways, I am not talking theoretically, I ACTAULLY tested your code and reproduced issue when invalid -guisettingsdir path makes pruning disabled (it's by default disabled, empty QSettings results in default), which is unexpected by user, and results your blockchain copy a full, unpruned archive, using up space.

If I set invalid -blocksdir=/tmp/foo/bar, I get hard error. If I set invalid -datadir=/tmp/foo/bar, I get hard error. If I set invalid -walletdir=/tmp/foo/bar, I get hard error. If I set invalid -guisettingsdir=/tmp/foo/bar, I DO NOT get hard error. This is incossistency, dangerous, and so wrong.

emilengler commented at 5:39 PM on February 6, 2020: contributor

Anyways, I am not talking theoretically, I ACTAULLY tested your code and reproduced issue when invalid -guisettingsdir path makes pruning disabled (it's by default disabled, empty QSettings results in default), which is unexpected by user, and results your blockchain copy a full, unpruned archive, using up space.

I bet that 99.99% percent of users will notice it if the blockchain gets re-downloaded (modaloverlay) opens again, etc.

Talkless commented at 6:46 PM on February 10, 2020: none

I bet that 99.99% percent of users will notice

bitcoin-qt shows me that overlay every day I launch it, as it has to keep up after being offline for the night. I just instinctively close window to minimize it without bothering much...

Anyway, this is only one of possible surprises user can get once -guisettingsdir is set to invalid path.

Talkless commented at 6:48 PM on February 10, 2020: none

Tested, Approach NACK, due to insufficient error handling which is inconsistent when compared to -blocksdir/-datadir/-walletdir command line options, and IMHO authors arguments keeping that way are not strong enough.

ryanofsky commented at 7:46 PM on March 5, 2020: member

From IRC

would be good to get ryanofsky's ack

IMO adding this with no error checking is adding a slightly useful feature that's a big footgun in practice. I don't want to block this change if other people want it, but would just again recommend error checking: #17636 (review) so there aren't silent failures and silently discarded settings with this

DrahtBot added the label Needs rebase on Mar 19, 2020

emilengler commented at 9:57 AM on March 20, 2020: contributor

Rebased on 5bf45fe2a9642f8ae8f8a12bcbf8f8b4770421ad, will now work ryanofskys suggestion

emilengler force-pushed on Mar 20, 2020

emilengler commented at 10:23 AM on March 20, 2020: contributor

The client now checks if the custom config file is accessible. If not, it returns an error. If it is not accessible but it is the first time you run bitcoin-qt (datadir does not exists), it will create the config at that location. If this fails it also returns an error.

DrahtBot removed the label Needs rebase on Mar 20, 2020

in src/qt/bitcoin.cpp:508 in 1d2c07b68a outdated

 504 | +        std::string settingspath = settings.fileName().toUtf8().constData();
 505 | +        // TODO: After C++17 move, port this to std::filesystem
 506 | +        std::fstream f;
 507 | +        f.open(settingspath.c_str());
 508 | +        // File could not be opened
 509 | +        if (f.fail()) {

Talkless commented at 9:39 AM on April 1, 2020:

Any reason not to use QFile? In that case would be more "straightforward" - no need to convert QString to std::string.

in src/qt/bitcoin.cpp:510 in 1d2c07b68a outdated

 506 | +        std::fstream f;
 507 | +        f.open(settingspath.c_str());
 508 | +        // File could not be opened
 509 | +        if (f.fail()) {
 510 | +            QMessageBox::critical(nullptr, PACKAGE_NAME,
 511 | +                QObject::tr("Error: Cannot open config file at \"%1\"").arg(QString::fromStdString(qt_settings_path)));

Talkless commented at 9:42 AM on April 1, 2020:

IMHO that message could be more concrete - that config file is a bit goo vague. Maybe mentioning GUI settings file would be more informative (since it's for -guisettingsdir).

in src/qt/bitcoin.cpp:502 in 1d2c07b68a outdated

 495 | @@ -489,6 +496,22 @@ int GuiMain(int argc, char* argv[])
 496 |      bool prune = false; // Intro dialog prune check box
 497 |      // Gracefully exit if the user cancels
 498 |      if (!Intro::showIfNeeded(*node, did_show_intro, prune)) return EXIT_SUCCESS;
 499 | +    // Now check if the custom config files are accecible
 500 | +    // If you run Bitcoin Qt the first time (no datadir) it will simply create the config files at that location
 501 | +    if(!qt_settings_path.empty())
 502 | +    {

luke-jr commented at 11:45 AM on April 6, 2020:

nit: Space after if, put { on same line

luke-jr commented at 12:54 PM on April 6, 2020:

(still missing the space)

luke-jr commented at 11:45 AM on April 6, 2020: member

If it is not accessible but it is the first time you run bitcoin-qt (datadir does not exists), it will create the config at that location.

I don't see code for this...?

emilengler commented at 12:15 PM on April 6, 2020: contributor

Thanks for your review @luke-jr

I don't see code for this...?

This is covered by the Qt library itself

qt: Add -guisettingsdir option 187f9684e0

emilengler force-pushed on Apr 6, 2020

luke-jr commented at 12:40 PM on April 6, 2020: member

This is covered by the Qt library itself

Qt doesn't know if -datadir exists or not...

jonasschnelli commented at 8:09 AM on May 29, 2020: contributor

Is it intentional that changing the guisettingsdir to an empty folder, once you have a datadir setup (say after IBD), will not work?

jonasschnelli commented at 8:12 AM on May 29, 2020: contributor

Tested a bit and does not work as expected:

Tried on an existing node/datadir (where I previously created a folder /tmp/gui):

./src/qt/bitcoin-qt --regtest --guisettingsdir=/tmp/gui

Leads to: <img width="520" alt="Bildschirmfoto 2020-05-29 um 10 10 37" src="https://user-images.githubusercontent.com/178464/83237039-d50f4180-a194-11ea-8a14-a87448e52ab6.png">

Same for creating a new datadir:

./src/qt/bitcoin-qt --regtest --datadir=/tmp/node --guisettingsdir=/tmp/gui

emilengler commented at 8:27 AM on May 29, 2020: contributor

@jonasschnelli Ok, I will have a look at this somewhere this weekend

fanquake commented at 7:10 AM on July 15, 2020: member

Progress here seems to have stalled, and this doesn't yet seem to work as intended. There have also been multiple requests for tests, and none have been added. I'm going to close this and suggest re-opening your changes in the gui repo if you are still interested in working on them, as aside from files.md this is contained to qt code.

fanquake closed this on Jul 15, 2020

DrahtBot locked this on Feb 15, 2022