gui: Drop RecentRequestsTableModel dependency to WalletModel #18618

promag commented at 9:30 AM on April 13, 2020: member

No description provided.

fanquake added the label GUI on Apr 13, 2020

promag cross-referenced this on Apr 13, 2020 from issue gui: Drop WalletModel dependency to RecentRequestsTableModel by promag

promag commented at 9:32 AM on April 13, 2020: member

Currently based on #18608.

promag force-pushed on Apr 13, 2020

in src/qt/recentrequeststablemodel.cpp:19 in 8aa9b53e9a outdated

  15 | @@ -17,18 +16,18 @@
  16 |  
  17 |  #include <utility>
  18 |  
  19 | -RecentRequestsTableModel::RecentRequestsTableModel(WalletModel *parent) :
  20 | -    QAbstractTableModel(parent), walletModel(parent)
  21 | +RecentRequestsTableModel::RecentRequestsTableModel(interfaces::Wallet& wallet, OptionsModel* options_model, QObject* parent) :

ryanofsky commented at 3:50 PM on April 13, 2020:

In commit "gui: Drop RecentRequestsTableModel dependency to WalletModel" (8aa9b53e9a47810d3ebe90743123b130cc4493c3)

Would be nice to make options_model and m_options_model references instead of pointers if they are not allowed to be null

promag commented at 11:52 PM on April 19, 2020:

Sure, I think its unrelated to this PR though. For instance, the check in L126 could be removed. Lots of other members could be references including cases like WalletView::walletModel where there's a setter.

ryanofsky approved

ryanofsky commented at 3:51 PM on April 13, 2020: contributor

Code review ACK 8aa9b53e9a47810d3ebe90743123b130cc4493c3

DrahtBot commented at 4:38 PM on April 13, 2020: 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:

#19292 (wallet: Refactor BerkeleyBatch Read, Write, Erase, and Exists functions into non-template functions by achow101)
#19290 (wallet: move BDB specific classes to bdb.{cpp/h} by achow101)
#19289 (wallet: GetWalletTx and IsMine require cs_wallet lock by promag)
#19245 ([WIP DONOTMERGE] Replace boost::filesystem with std::filesystem (in c++17) by kiminuo)
#19137 (wallettool: Add dump and createfromdump commands by achow101)
#19102 (wallet: Introduce and use DummyDatabase instead of dummy BerkeleyDatabase by achow101)
#19077 (wallet: Add sqlite as an alternative wallet database and use it for new descriptor wallets by achow101)
#18971 (wallet: Refactor the classes in wallet/db.{cpp/h} by achow101)
#18354 (Use shared pointers only in validation interface by bvbfan)
#17966 (qt, refactor: Optimize signal-slot connections logic by hebasto)
#17877 (qt, refactor: Make enums in BitcoinUnits class scoped by hebasto)

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.

DrahtBot cross-referenced this on Apr 13, 2020 from issue Store destdata for change in separate key for backward compatibility by luke-jr

DrahtBot cross-referenced this on Apr 13, 2020 from issue util: Make our stringstream usage locale independent by practicalswift

DrahtBot cross-referenced this on Apr 13, 2020 from issue qt, refactor: Optimize signal-slot connections logic by hebasto

DrahtBot cross-referenced this on Apr 13, 2020 from issue qt, refactor: Make BitcoinUnits::Unit a scoped enum by hebasto

DrahtBot cross-referenced this on Apr 13, 2020 from issue Native Descriptor Wallets using DescriptorScriptPubKeyMan by achow101

DrahtBot cross-referenced this on Apr 13, 2020 from issue rpc: gui: Don't change behavior based on private keys disabled, instead add new buttons/rpcs/menu items by achow101

DrahtBot cross-referenced this on Apr 15, 2020 from issue gui: Add bumpFeePSBT action instead of changing normal bumpfee behavior by achow101

DrahtBot cross-referenced this on Apr 18, 2020 from issue wallet: Avoid translating RPC errors by MarcoFalke

DrahtBot cross-referenced this on Apr 23, 2020 from issue External signer support - Wallet Box edition by Sjors

DrahtBot cross-referenced this on Apr 23, 2020 from issue UI external signer support (e.g. hardware wallet) by Sjors

DrahtBot added the label Needs rebase on Apr 27, 2020

promag force-pushed on May 4, 2020

DrahtBot removed the label Needs rebase on May 4, 2020

DrahtBot cross-referenced this on May 4, 2020 from issue Use shared pointers only in validation interface by bvbfan

DrahtBot added the label Needs rebase on May 4, 2020

ryanofsky approved

ryanofsky commented at 8:59 PM on May 13, 2020: contributor

Code review ACK aa2b55a8845de5f4be6a0f5a9aee60447838112d. No changes since last review, just rebased base PR

promag force-pushed on May 17, 2020

DrahtBot removed the label Needs rebase on May 17, 2020

DrahtBot cross-referenced this on May 18, 2020 from issue wallet: Refactor the classes in wallet/db.{cpp/h} by achow101

DrahtBot cross-referenced this on May 18, 2020 from issue wallet: Move salvagewallet into wallettool by achow101

ryanofsky commented at 8:26 PM on May 19, 2020: contributor

Code review ACK c5ebb4db6894bec60e00f9247f6a6806cf30d06a. Only change since last review is rebasing to fix bilingual_str conflict

DrahtBot added the label Needs rebase on May 27, 2020

refactor: Remove CAddressBookData::destdata

This is cleanup that doesn't change external behavior.

- Removes awkward `StringMap` intermediate representation
- Deals with receive request "rr" keys in walletdb.cpp instead of all over qt, wallet, and interfaces code
- Deals with destination "used" keys in walletdb.cpp instead of all over wallet code
- Adds test coverage
- Reduces code (+85/-138 lines)
- Reduces memory usage

This PR doesn't change externally observable behavior. Internally, only change in behavior is that EraseDestData deletes directly from database because the `StringMap` is gone. This is more direct and efficient because it uses a single btree lookup and scan instead of multiple lookups

Motivation for this cleanup is making changes like #18550, #18192, #13756 easier to reason about and less likely to result in unintended behavior and bugs

3a58f2ae17

gui: Drop RecentRequestsTableModel dependency to WalletModel 47a85bce35

promag force-pushed on Jun 1, 2020

DrahtBot removed the label Needs rebase on Jun 1, 2020

DrahtBot cross-referenced this on Jun 1, 2020 from issue wallet: Introduce and use DummyDatabase instead of dummy BerkeleyDatabase by achow101

DrahtBot cross-referenced this on Jun 1, 2020 from issue refactor: remove ::vpwallets and related global variables by ryanofsky

DrahtBot cross-referenced this on Jun 1, 2020 from issue wallet: Add sqlite as an alternative wallet database and use it for new descriptor wallets by achow101

DrahtBot cross-referenced this on Jun 2, 2020 from issue wallettool: Add dump and createfromdump commands by achow101

DrahtBot cross-referenced this on Jun 6, 2020 from issue [WIP DONOTMERGE] Replace boost with C++17 (std::shared_mutex) by MarcoFalke

DrahtBot cross-referenced this on Jun 16, 2020 from issue wallet: Refactor BerkeleyBatch Read, Write, Erase, and Exists functions into non-template functions by achow101

DrahtBot cross-referenced this on Jun 16, 2020 from issue wallet: move BDB specific classes to bdb.{cpp/h} by achow101

DrahtBot cross-referenced this on Jun 16, 2020 from issue wallet: GetWalletTx and IsMine require cs_wallet lock by promag

DrahtBot cross-referenced this on Jun 16, 2020 from issue Replace boost::filesystem with std::filesystem by kiminuo

DrahtBot added the label Needs rebase on Jun 17, 2020

DrahtBot commented at 10:01 AM on June 17, 2020: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

promag marked this as a draft on Jun 17, 2020

fanquake commented at 1:08 PM on July 9, 2020: member

This is based on #18608 (which itself has 1, I think, refuted Concept NACK & needs a rebase), is purely a qt circular dependency refactor and also needs a rebase. I'm going to suggest reopening this in https://github.com/bitcoin-core/gui.

fanquake closed this on Jul 9, 2020

ryanofsky commented at 8:12 PM on July 10, 2020: contributor

#18608 is up to date (and I think was only conflicted for a few hours)

bitcoin locked this on Feb 15, 2022