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>
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
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.
ACK 945e4676dd3c92b353e13c78c8db673d8b64fe2d
142 | +SplashScreen::~SplashScreen() 143 | { 144 | - assert(!m_node); 145 | - m_node = &node; 146 | - subscribeToCoreSignals(); 147 | - if (m_shutdown) m_node->startShutdown();
Do you know if there was any reason for having this 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.
Code review ACK 945e4676
Updated 945e4676dd3c92b353e13c78c8db673d8b64fe2d -> 31122aa979c4c9a40e276cfc44243420c367ba4f (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.
Till me
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);
Just noting that 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 | +
This hunk was moved because now createOptionsModel needs m_node instanced.
Code review ACK 31122aa979c4c9a40e276cfc44243420c367ba4f.
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);
It seems to not be strictly necessary to have to initialize 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?
nevermind, I see how this is needed in consideration of future changes :D