refactor: Pass interfaces::Node references to OptionsModel constructor #601

pull ryanofsky wants to merge 1 commits into bitcoin-core:master from ryanofsky:pr/qtnode changing 7 files +18 −20
  1. ryanofsky commented at 7:53 pm on May 19, 2022: contributor

    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.

  2. refactor: Pass interfaces::Node references to OptionsModel constructor
    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>
    31122aa979
  3. DrahtBot commented at 8:46 am on May 20, 2022: contributor

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #363 (Peers window: Show direction in a new column, with clearer icon by luke-jr)

    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.

  4. hebasto added the label Refactoring on May 20, 2022
  5. vasild approved
  6. vasild commented at 12:15 pm on May 20, 2022: contributor
    ACK 945e4676dd3c92b353e13c78c8db673d8b64fe2d
  7. in src/qt/splashscreen.cpp:145 in 945e4676dd outdated
    142+SplashScreen::~SplashScreen()
    143 {
    144-    assert(!m_node);
    145-    m_node = &node;
    146-    subscribeToCoreSignals();
    147-    if (m_shutdown) m_node->startShutdown();
    


    furszy commented at 2:12 pm on May 20, 2022:
    Do you know if there was any reason for having this startShutdown here?

    ryanofsky commented at 3:34 pm on May 20, 2022:

    In commit “refactor: Remove SplashScreen::setNode method” (945e4676dd3c92b353e13c78c8db673d8b64fe2d)

    re: #601 (review)

    Do you know if there was any reason for having this startShutdown here?

    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.

  8. furszy approved
  9. furszy commented at 2:17 pm on May 20, 2022: contributor
    Code review ACK 945e4676
  10. ryanofsky force-pushed on May 20, 2022
  11. ryanofsky commented at 4:25 pm on May 20, 2022: contributor
    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.
  12. chetooo40 commented at 4:37 pm on May 20, 2022: none
    Till me
  13. in src/qt/bitcoin.cpp:264 in 31122aa979
    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);
    


    promag commented at 11:02 am on May 21, 2022:
    Just noting that node() asserts for m_node.
  14. in src/qt/bitcoin.cpp:635 in 31122aa979
    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+
    


    promag commented at 11:07 am on May 21, 2022:
    This hunk was moved because now createOptionsModel needs m_node instanced.
  15. promag commented at 11:07 am on May 21, 2022: contributor
    Code review ACK 31122aa979c4c9a40e276cfc44243420c367ba4f.
  16. furszy approved
  17. furszy commented at 2:10 pm on May 23, 2022: contributor

    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.

  18. ryanofsky commented at 5:00 pm on May 23, 2022: contributor

    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.

  19. in src/qt/optionsmodel.h:44 in 31122aa979
    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);
    


    jarolrod commented at 1:33 am on May 24, 2022:
    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?

    jarolrod commented at 1:40 am on May 24, 2022:
    nevermind, I see how this is needed in consideration of future changes :D
  20. jarolrod approved
  21. jarolrod commented at 1:42 am on May 24, 2022: member

    ACK 31122aa979c4c9a40e276cfc44243420c367ba4f

    Reviewed the code and agree it can be merged. Tested this with future changes in #602.

  22. hebasto merged this on May 24, 2022
  23. hebasto closed this on May 24, 2022

  24. sidhujag referenced this in commit c58ab277af on May 28, 2022
  25. bitcoin-core locked this on May 24, 2023

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/gui. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-26 17:20 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me