Giving OptionsModel access to the node interface is needed as part of #602 to get bitcoind and bitcoin-qt to use the same settings instead of different settings.
It has been split off from #602 to simplify that PR. Previously these commits were part of bitcoin/bitcoin#15936 and also had some review discussion there.
Will allow OptionsModel to read/write settings to the node settings.json
file and share settings with the node, instead of storing them
externally in QSettings.
Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
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.
142+SplashScreen::~SplashScreen()
143 {
144- assert(!m_node);
145- m_node = &node;
146- subscribeToCoreSignals();
147- if (m_shutdown) m_node->startShutdown();
startShutdown here?
In commit “refactor: Remove SplashScreen::setNode method” (945e4676dd3c92b353e13c78c8db673d8b64fe2d)
re: #601 (review)
Do you know if there was any reason for having this
startShutdownhere?
Good catch. I didn’t actually notice this line was removed, and removing it is not really correct. This comes from #35 commit https://github.com/bitcoin-core/gui/pull/35/commits/519cae8fd6e44aef3470415d7c5e12acb0acd9f4 and the idea is that with IPC in bitcoin/bitcoin#19461, the node could be running in another process or on another machine, and you shouldn’t have to wait for a connection to the node to show the splash screen. I reworked the PR with this in mind, and the PR is a little simpler than before, even if it’s removing less code now.
pr/qtnode.1 -> pr/qtnode.2, compare) removing code that was changing splash screen init, now initializing OptionsModel after the splash screen, instead of before.
260@@ -261,7 +261,7 @@ void BitcoinApplication::createPaymentServer()
261
262 void BitcoinApplication::createOptionsModel(bool resetSettings)
263 {
264- optionsModel = new OptionsModel(this, resetSettings);
265+ optionsModel = new OptionsModel(node(), this, resetSettings);
node() asserts for m_node.
631@@ -633,6 +632,12 @@ int GuiMain(int argc, char* argv[])
632 // Allow parameter interaction before we create the options model
633 app.parameterSetup();
634 GUIUtil::LogQtInfo();
635+
createOptionsModel needs m_node instanced.
Code ACK 31122aa9
With a small note for:
now initializing OptionsModel after the splash screen, instead of before.
We can probably initialize the SplashScreen after the OptionsModel, so at presentation time (show()) the widget can get access to the user selected language from there and not fallback to the system’s language.
Still.. probably a big meh as we aren’t having anything to translate in that screen atm.
We can probably initialize the SplashScreen after the
OptionsModel, so at presentation time (show()) the widget can get access to the user selected language from there and not fallback to the system’s language
I think it actually shouldn’t need OptionsModel for this, should just use gArgs.GetArg("-lang"). The early-init gui code uses ArgsManager and QSettings directly to get started, then it switches to interfaces::Node when the node is running. Probably unless the splash screen gets really fancy, it shouldn’t need an OptionsModel pointer, and if it does it could be passed after it is started.
40@@ -41,7 +41,7 @@ class OptionsModel : public QAbstractListModel
41 Q_OBJECT
42
43 public:
44- explicit OptionsModel(QObject *parent = nullptr, bool resetSettings = false);
45+ explicit OptionsModel(interfaces::Node& node, QObject *parent = nullptr, bool resetSettings = false);
OptionsModel with a reference to interfaces::Node and instead could just be set at a later point which is similar to how we do it now. Wondering what is the justification to add this to the constructor?