refactor: Add `transactionoverviewwidget.cpp` source file #618

pull hebasto wants to merge 1 commits into bitcoin-core:master from hebasto:220614-tow changing 4 files +32 −16

hebasto commented at 10:57 AM on June 14, 2022: member

The TransactionOverviewWidget class was added in bitcoin-core/gui#176 as a header-only one.

Apparently, in upcoming CMake project, CMake AUTOMOC could be integrated better/simpler, if QObject-derived class implementation been placed into a source file.

From our Developer Notes:

Implementation code should go into the .cpp file and not the .h, unless necessary due to template usage or when performance due to inlining is critical.
DrahtBot commented at 12:22 PM on June 14, 2022: contributor



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



Conflicts

No conflicts as of last run.
qt, refactor: Add `transactionoverviewwidget.cpp` source file
Required for better/simpler interaction with CMake AUTOMOC.
a50e0b1bcb
hebasto force-pushed on Jun 14, 2022
Sjors commented at 6:22 PM on June 14, 2022: member

tACK a50e0b1bcb120e097a2b23dbfb71533b2f9c4d6c

Using a cpp file is consistent with other widgets we have.
shaavan approved
shaavan commented at 9:07 AM on June 15, 2022: contributor
ACK a50e0b1bcb120e097a2b23dbfb71533b2f9c4d6c
- I agree with the idea of transforming transactionoverviewwidget from a header-only file to having a separate cpp file to implement functions.
- The code change and cpp implementation are cleanly implemented, with proper formatting.
- I successfully ran the PR on Ubuntu 22.04 with Qt version 5.15.3.
- Observed no functional difference between the master and this PR, showing that this is a pure refactor change.
in src/qt/transactionoverviewwidget.h:8 in a50e0b1bcb
```
   4 | @@ -5,11 +5,8 @@
   5 |  #ifndef BITCOIN_QT_TRANSACTIONOVERVIEWWIDGET_H
   6 |  #define BITCOIN_QT_TRANSACTIONOVERVIEWWIDGET_H
   7 |  
   8 | -#include <qt/transactiontablemodel.h>
   9 | -
  10 |  #include <QListView>
```
furszy commented at 3:37 PM on June 15, 2022:

QListView is being included in both files now. Could remove it from here.

hebasto commented at 3:43 PM on June 15, 2022:

From our Developer Notes:

Every .cpp and .h file should #include every header file it directly uses classes, functions or other definitions from, even if those headers are already included indirectly through other headers.

in src/qt/transactionoverviewwidget.cpp:10 in a50e0b1bcb

   5 | +#include <qt/transactionoverviewwidget.h>
   6 | +
   7 | +#include <qt/transactiontablemodel.h>
   8 | +
   9 | +#include <QListView>
  10 | +#include <QSize>

furszy commented at 3:38 PM on June 15, 2022:

QSize is included in the header file, could delete this line.

chetoo40 approved
hebasto merged this on Jun 15, 2022
hebasto closed this on Jun 15, 2022
sidhujag referenced this in commit 69c085e858 on Jun 15, 2022
jarolrod commented at 5:15 PM on June 16, 2022: member

post-merge ACK https://github.com/bitcoin-core/gui/commit/a50e0b1bcb120e097a2b23dbfb71533b2f9c4d6c
hebasto deleted the branch on Jun 21, 2022
bitcoin-core locked this on Jun 21, 2023

Contributors

refactor: Add `transactionoverviewwidget.cpp` source file #618

Conflicts