refactor: Add transactionoverviewwidget.cpp source file#618
Merged
hebasto merged 1 commit intobitcoin-core:masterfrom Jun 15, 2022
Merged
refactor: Add transactionoverviewwidget.cpp source file#618hebasto merged 1 commit intobitcoin-core:masterfrom
transactionoverviewwidget.cpp source file#618hebasto merged 1 commit intobitcoin-core:masterfrom
Conversation
13 tasks
Contributor
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
Required for better/simpler interaction with CMake AUTOMOC.
Member
|
tACK a50e0b1 Using a cpp file is consistent with other widgets we have. |
shaavan
approved these changes
Jun 15, 2022
Contributor
shaavan
left a comment
shaavan
left a comment
There was a problem hiding this comment.
ACK a50e0b1
- I agree with the idea of transforming
transactionoverviewwidgetfrom 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.
furszy
reviewed
Jun 15, 2022
|
|
||
| #include <qt/transactiontablemodel.h> | ||
|
|
||
| #include <QListView> |
Member
There was a problem hiding this comment.
QListView is being included in both files now. Could remove it from here.
Member
Author
There was a problem hiding this comment.
From our Developer Notes:
Every
.cppand.hfile should#includeevery header file it directly uses classes, functions or other definitions from, even if those headers are already included indirectly through other headers.
| #include <qt/transactiontablemodel.h> | ||
|
|
||
| #include <QListView> | ||
| #include <QSize> |
Member
There was a problem hiding this comment.
QSize is included in the header file, could delete this line.
chetoo40
approved these changes
Jun 15, 2022
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Jun 15, 2022
…cpp` source file a50e0b1 qt, refactor: Add `transactionoverviewwidget.cpp` source file (Hennadii Stepanov) Pull request description: The `TransactionOverviewWidget` class was added in bitcoin-core/gui#176 as a header-only one. Apparently, in upcoming [CMake project](hebasto#3), CMake [AUTOMOC](https://cmake.org/cmake/help/latest/prop_tgt/AUTOMOC.html) could be integrated better/simpler, if `QObject`-derived class implementation been placed into a source file. From our [Developer Notes](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#source-code-organization): > 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. ACKs for top commit: Sjors: tACK a50e0b1 shaavan: ACK a50e0b1 Tree-SHA512: 4707b6be1c5e794c4014475f826ac45ec833e472db11f12d29995f9c5a599ee98622ad54f0af72734b192144b626411c69acdafa0e6d1a390bdebfd7e570f377
jarolrod
reviewed
Jun 16, 2022
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The
TransactionOverviewWidgetclass was added in #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: