move most explicit getters in optionsmodel to header #1900
Conversation
Diapolo
commented
Oct 2, 2012
Diapolo
commented
- is more consistent and saves quite some lines of code
|
OK to the changes, except the one that requires an "extern" global variable The idea is to keep the communication with the bitcoin core limited to the |
|
That one is easy to revert, I wasn't aware of the reason you mentioned above. |
|
It's violated in a few places, for example signmessage.cpp directly uses |
| @@ -1,6 +1,8 @@ | |||
| #ifndef OPTIONSMODEL_H | |||
| #define OPTIONSMODEL_H | |||
|
|
|||
| #include "uint256.h" | |||
There was a problem hiding this comment.
Indeed, I forgot to remove it while reverting the getTransactionFee() change, thanks for noticing!
- is more consistent and saves quite some lines of code
|
@laanwj Thanks for explaining the idea to limit core access to *model.cpp, but I have to ask what makes the difference between having them in a .cpp or a .h only in the end? |
|
Compilation time, and it makes it easier to separate out the UI code, for example if we make the UI a separate process. |
|
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/7bc65ff1083c726391923fddac86e5abc4b0f2db for binaries and test log. |
|
Any reason why having the getters in .h is preferrable? |
|
I'm neutral on this. This change is balancing on the ugly edges of C++, on one side this cuts down on the amount of boilerplate lines to compensate for the lack of properties, on the other hand this moves implementation details to the interface description which is also undesirable (but in many cases unavoidable, at least if you want to make full use of the language with templates and such). |
|
@laanwj Ok, good to know. There has been a general trend towards moving code to .cpp files, but I wasn't sure to what extent we want to pursue this. I don't care about having such oneliners in .h files. |
|
Right, in the case of non-type-parametrized and non-trivial code, the implementation should certainly be in the implementation file. |
|
I found it rather hard to follow your discussion as I'm missing some tech-english here. In the end is such a change wanted or should it be avoided (ACK / NACK)? Edit: And as a remainder, the getLanguage() function was already in the header :). |
|
Yes, ACK. |
|
ACK |
move most explicit getters in optionsmodel to header
35cc25c fix flags deprecation (furszy) Pull request description: Removing an obsolete `QFlags` member. Not compiling in qt 5.15. Issue reported by @rhubarbarian in discord 👌 . ACKs for top commit: random-zebra: ACK 35cc25c Fuzzbawls: utACK 35cc25c Tree-SHA512: 3148774b0be552eb02740baa7cb697afe43d1396fee993f6a90808b8c97dae7fe990bfc81fc6598e202ee25762136556e573665b6abb8b916c1cb5b1c24feb27
