Getting ready to Qt 6 (7/n). Do not pass WalletModel* to a queued connection #589

pull hebasto wants to merge 3 commits into bitcoin-core:master from hebasto:220420-walletmodel changing 2 files +21 −15
  1. hebasto commented at 2:39 pm on April 20, 2022: member

    On master (094d9fda5ccee7d78a2e3d8b1eec17b8b6a33466), the following queued connection https://github.com/bitcoin-core/gui/blob/094d9fda5ccee7d78a2e3d8b1eec17b8b6a33466/src/qt/rpcconsole.cpp#L1107 uses a const WalletModel* parameter regardless whether the ENABLE_WALLET macro is defined.

    Although this code works in Qt 5, it is flawed. On Qt 6, the code gets broken because the fully defined WalletModel type is required which is not the case if ENABLE_WALLET is undefined.

    This PR fixes the issue described above.

  2. refactor: Guard `RPCConsole::{add,remove}Wallet()` with `ENABLE_WALLET` 61457c179a
  3. hebasto force-pushed on Apr 20, 2022
  4. hebasto force-pushed on Apr 20, 2022
  5. hebasto renamed this:
    Getting ready to Qt 6 (7/n). Pass `WalletModel*` pointer to signal only when `ENABLE_WALLET` defined
    Getting ready to Qt 6 (7/n). Do not pass `WalletModel*` to a queued connection
    on Apr 20, 2022
  6. hebasto commented at 7:53 am on April 21, 2022: member
    Friendly ping @promag
  7. in src/qt/rpcconsole.h:57 in 61457c179a outdated 
    52@@ -49,8 +53,11 @@ class RPCConsole: public QWidget
53     }
54 
55     void setClientModel(ClientModel *model = nullptr, int bestblock_height = 0, int64_t bestblock_date = 0, double verification_progress = 0.0);
56-    void addWallet(WalletModel * const walletModel);
57+
58+#ifdef ENABLE_WALLET
    promag commented at 10:59 am on April 21, 2022:

    61457c179aec23227dcf3952c575052204103b50

    review note, implementation is already guarded in https://github.com/bitcoin-core/gui/blob/61457c179aec23227dcf3952c575052204103b50/src/qt/rpcconsole.cpp#L786

    promag commented at 12:00 pm on April 21, 2022:

    61457c179aec23227dcf3952c575052204103b50

    Now I wonder what’s the point of this guard 🤔

    hebasto commented at 12:08 pm on April 21, 2022:
    To make RPCConsole interface explicit?
  8. in src/qt/rpcconsole.cpp:91 in 5aa5ed9897 outdated 
    87@@ -88,24 +88,6 @@ const QStringList historyFilter = QStringList()
88 
89 }
90 
91-/* Object for executing console RPC commands in a separate thread.
    promag commented at 11:01 am on April 21, 2022:

    5aa5ed9897cfe3d5832daa9b25835b73da37f5a9

    A forward declaration would be fine for the m_executor member (so this commit could be dropped)?

    hebasto commented at 11:50 am on April 21, 2022:
    Thanks! Updated.
  9. in src/qt/rpcconsole.cpp:1076 in d36fa9707d outdated 
    1072@@ -1073,11 +1073,11 @@ void RPCConsole::browseHistory(int offset)
1073 
1074 void RPCConsole::startExecutor()
1075 {
1076-    RPCExecutor *executor = new RPCExecutor(m_node);
1077-    executor->moveToThread(&thread);
1078+    m_executor= new RPCExecutor(m_node);
    promag commented at 11:02 am on April 21, 2022:

    d36fa9707d9dc00317199633daac03ff0bae9050

    nit, space before =.

    hebasto commented at 11:50 am on April 21, 2022:
    Thanks! Updated.
  10. in src/qt/rpcconsole.cpp:1036 in b7257271c5 outdated 
    1030@@ -1031,7 +1031,10 @@ void RPCConsole::on_lineEdit_returnPressed()
1031     //: A console message indicating an entered command is currently being executed.
1032     message(CMD_REPLY, tr("Executing…"));
1033     m_is_executing = true;
1034-    Q_EMIT cmdRequest(cmd, m_last_wallet_model);
1035+
1036+    QMetaObject::invokeMethod(m_executor, [this, cmd]() {
1037+        m_executor->request(cmd, m_last_wallet_model);
    promag commented at 11:10 am on April 21, 2022:

    b7257271c533a97ea5e8b675ece6eee61aef23de

    I think we should avoid m_last_wallet_model since this is executed asynchronously, and in theory m_last_wallet_model could change meanwhile. I suggest above:

    0    WalletModel* wallet_model{nullptr};
1#ifdef ENABLE_WALLET
2    wallet_model = ui->WalletSelector->currentData().value<WalletModel*>();

    and here:

    0    QMetaObject::invokeMethod(m_executor, [=] {
1        m_executor->request(cmd, wallet_model);
2    });
    hebasto commented at 11:50 am on April 21, 2022:
    Thanks! Updated.
  11. promag changes_requested
  12. promag commented at 11:16 am on April 21, 2022: contributor

    Concept ACK.

    An alternative is to ditch the 2nd argument, like (untested):

    0Q_SIGNALS:
1    // For RPC command executor
2    void cmdRequest(const QString &command
3#ifdef ENABLE_WALLET
4        , const WalletModel* wallet_model
5#endif
6    );

    0    Q_EMIT cmdRequest(cmd
1#ifdef ENABLE_WALLET
2        , m_last_wallet_model
3#endif
4    );
  13. hebasto commented at 11:21 am on April 21, 2022: member

    @promag

    Concept ACK.

    An alternative is to ditch the 2nd argument, like (untested):

    0Q_SIGNALS:
1    // For RPC command executor
2    void cmdRequest(const QString &command
3#ifdef ENABLE_WALLET
4        , const WalletModel* wallet_model
5#endif
6    );

    0    Q_EMIT cmdRequest(cmd
1#ifdef ENABLE_WALLET
2        , m_last_wallet_model
3#endif
4    );

    A similar approach was in my initial implementation – https://github.com/hebasto/gui/commits/pr589.01 :)

    But it somehow breaks interaction with moc compiler when using MSVC.

  14. refactor: Make `RPCExecutor*` a member of the `RPCConsole` class fdf7285950
  15. hebasto force-pushed on Apr 21, 2022
  16. hebasto commented at 11:49 am on April 21, 2022: member

    @promag

    Thank you for your review! Agree with all of points you mentioned above.

    Updated.

  17. in src/qt/rpcconsole.cpp:1054 in 16d136ac51 outdated 
    1049@@ -1049,7 +1050,10 @@ void RPCConsole::on_lineEdit_returnPressed()
1050     //: A console message indicating an entered command is currently being executed.
1051     message(CMD_REPLY, tr("Executing…"));
1052     m_is_executing = true;
1053-    Q_EMIT cmdRequest(cmd, m_last_wallet_model);
1054+
1055+    QMetaObject::invokeMethod(m_executor, [this, cmd, wallet_model]() {
    promag commented at 11:58 am on April 21, 2022:

    16d136ac51f56d0d5fa799d000d264c685052c4e

    nit, drop ().

    hebasto commented at 12:08 pm on April 21, 2022:
    Updated.
  18. promag approved
  19. promag commented at 12:01 pm on April 21, 2022: contributor
    Code review ACK 16d136ac51f56d0d5fa799d000d264c685052c4e.
  20. Do not pass `WalletModel*` to queued connection 
    Passing a `WalletModel*` object to a queued connection when the
`ENABLE_WALLET` macro is undefined make code flawed.
    ab73d5985d
  21. hebasto force-pushed on Apr 21, 2022
  22. promag commented at 12:11 pm on April 21, 2022: contributor
    ACK ab73d5985de5d9c4d1e3fd0f4d9d88a0908ea319
  23. hebasto added the label Qt 6 on Apr 22, 2022
  24. jarolrod commented at 7:39 am on April 27, 2022: member
    code review ACK https://github.com/bitcoin-core/gui/commit/ab73d5985de5d9c4d1e3fd0f4d9d88a0908ea319
  25. hebasto merged this on Apr 27, 2022
  26. hebasto closed this on Apr 27, 2022
  27. hebasto deleted the branch on Apr 27, 2022
  28. sidhujag referenced this in commit 67306b74b7 on Apr 29, 2022
  29. bitcoin-core locked this on Apr 27, 2023


hebasto promag jarolrod

Labels
Qt 6

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-11-01 03:20 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me