refactor: Pass interfaces::Node references to OptionsModel constructor #601
Conversation
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. ConflictsReviewers, 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. |
There was a problem hiding this comment.
Updated 945e467 -> 31122aa (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.
| @@ -261,7 +261,7 @@ void BitcoinApplication::createPaymentServer() | |||
|
|
|||
| void BitcoinApplication::createOptionsModel(bool resetSettings) | |||
| { | |||
| optionsModel = new OptionsModel(this, resetSettings); | |||
| optionsModel = new OptionsModel(node(), this, resetSettings); | |||
There was a problem hiding this comment.
Just noting that node() asserts for m_node.
| @@ -633,6 +632,12 @@ int GuiMain(int argc, char* argv[]) | |||
| // Allow parameter interaction before we create the options model | |||
| app.parameterSetup(); | |||
| GUIUtil::LogQtInfo(); | |||
|
|
|||
There was a problem hiding this comment.
This hunk was moved because now createOptionsModel needs m_node instanced.
There was a problem hiding this comment.
Code ACK 31122aa
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.
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. |
| @@ -41,7 +41,7 @@ class OptionsModel : public QAbstractListModel | |||
| Q_OBJECT | |||
|
|
|||
| public: | |||
| explicit OptionsModel(QObject *parent = nullptr, bool resetSettings = false); | |||
| explicit OptionsModel(interfaces::Node& node, QObject *parent = nullptr, bool resetSettings = false); | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
nevermind, I see how this is needed in consideration of future changes :D
…s to OptionsModel constructor 31122aa refactor: Pass interfaces::Node references to OptionsModel constructor (Ryan Ofsky) Pull request description: Giving OptionsModel access to the node interface is needed as part of bitcoin#602 to get bitcoind and bitcoin-qt to use the same settings instead of different settings. It has been split off from bitcoin#602 to simplify that PR. Previously these commits were part of bitcoin#15936 and also had some review discussion there. ACKs for top commit: promag: Code review ACK 31122aa. furszy: Code ACK 31122aa jarolrod: ACK 31122aa Tree-SHA512: b8529322fd7ba97e19864129e6cf5f9acc58c124f2e5a7c50aca15772c8549de3c24e8b0c27e8cf2c06fd26529e9cdb898b0788a1de3cbfdfbdd3f85c9f0fe69
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.