Changeset View
Standalone View
kmymoney/plugins/checkprinting/checkprinting.cpp
1 | /*************************************************************************** | 1 | /*************************************************************************** | ||
---|---|---|---|---|---|
2 | * This file is part of KMyMoney, A Personal Finance Manager by KDE * | 2 | * This file is part of KMyMoney, A Personal Finance Manager by KDE * | ||
3 | * * | 3 | * * | ||
4 | * Copyright (C) 2009 Cristian Onet <onet.cristian@gmail.com> * | 4 | * Copyright (C) 2009 Cristian Onet <onet.cristian@gmail.com> * | ||
5 | * Copyright (C) 2019 Thomas Baumgart <tbaumgart@kde.org> * | 5 | * Copyright (C) 2019 Thomas Baumgart <tbaumgart@kde.org> * | ||
6 | * * | 6 | * * | ||
7 | * This program is free software; you can redistribute it and/or * | 7 | * This program is free software; you can redistribute it and/or * | ||
8 | * modify it under the terms of the GNU General Public License as * | 8 | * modify it under the terms of the GNU General Public License as * | ||
9 | * published by the Free Software Foundation; either version 2 of * | 9 | * published by the Free Software Foundation; either version 2 of * | ||
10 | * the License or (at your option) version 3 or any later version * | 10 | * the License or (at your option) version 3 or any later version * | ||
tbaumgart: Change these
```
checkHTML.replace( valueVariable, MyMoneyUtils::formatMoney((*it). | |||||
11 | * accepted by the membership of KDE e.V. (or its successor approved * | 11 | * accepted by the membership of KDE e.V. (or its successor approved * | ||
12 | * by the membership of KDE e.V.), which shall act as a proxy * | 12 | * by the membership of KDE e.V.), which shall act as a proxy * | ||
13 | * defined in Section 14 of version 3 of the license. * | 13 | * defined in Section 14 of version 3 of the license. * | ||
14 | * * | 14 | * * | ||
15 | * This program is distributed in the hope that it will be useful, * | 15 | * This program is distributed in the hope that it will be useful, * | ||
16 | * but WITHOUT ANY WARRANTY; without even the implied warranty of * | 16 | * but WITHOUT ANY WARRANTY; without even the implied warranty of * | ||
17 | * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the * | 17 | * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the * | ||
18 | * GNU General Public License for more details. * | 18 | * GNU General Public License for more details. * | ||
▲ Show 20 Lines • Show All 165 Lines • ▼ Show 20 Line(s) | 161 | for (it = d->m_transactions.constBegin(); it != d->m_transactions.constEnd(); ++it) { | |||
184 | // data about the transaction | 184 | // data about the transaction | ||
185 | checkHTML.replace("$DATE", QLocale().toString((*it).transaction().postDate(), QLocale::ShortFormat)); | 185 | checkHTML.replace("$DATE", QLocale().toString((*it).transaction().postDate(), QLocale::ShortFormat)); | ||
186 | checkHTML.replace("$CHECK_NUMBER", (*it).split().number()); | 186 | checkHTML.replace("$CHECK_NUMBER", (*it).split().number()); | ||
187 | checkHTML.replace("$PAYEE_NAME", file->payee((*it).split().payeeId()).name()); | 187 | checkHTML.replace("$PAYEE_NAME", file->payee((*it).split().payeeId()).name()); | ||
188 | checkHTML.replace("$PAYEE_ADDRESS", file->payee((*it).split().payeeId()).address()); | 188 | checkHTML.replace("$PAYEE_ADDRESS", file->payee((*it).split().payeeId()).address()); | ||
189 | checkHTML.replace("$PAYEE_CITY", file->payee((*it).split().payeeId()).city()); | 189 | checkHTML.replace("$PAYEE_CITY", file->payee((*it).split().payeeId()).city()); | ||
190 | checkHTML.replace("$PAYEE_POSTCODE", file->payee((*it).split().payeeId()).postcode()); | 190 | checkHTML.replace("$PAYEE_POSTCODE", file->payee((*it).split().payeeId()).postcode()); | ||
191 | checkHTML.replace("$PAYEE_STATE", file->payee((*it).split().payeeId()).state()); | 191 | checkHTML.replace("$PAYEE_STATE", file->payee((*it).split().payeeId()).state()); | ||
192 | checkHTML.replace("$AMOUNT_STRING", converter.convert((*it).split().shares().abs(), currency.smallestAccountFraction())); | 192 | checkHTML.replace("$AMOUNT_STRING", converter.convert((*it).split().value().abs(), currency.smallestAccountFraction())); | ||
193 | checkHTML.replace("$AMOUNT_DECIMAL", MyMoneyUtils::formatMoney((*it).split().shares().abs(), currency)); | 193 | checkHTML.replace("$AMOUNT_DECIMAL", MyMoneyUtils::formatMoney((*it).split().value().abs(), currency)); | ||
194 | checkHTML.replace("$MEMO", (*it).split().memo()); | 194 | checkHTML.replace("$MEMO", (*it).split().memo()); | ||
195 | const auto currencyId = (*it).transaction().commodity(); | ||||
196 | const auto accountcurrency = MyMoneyFile::instance()->currency(currencyId); | ||||
197 | checkHTML.replace("$TRANSACTIONCURRENCY", accountcurrency.tradingSymbol()); | ||||
198 | int numSplits = (*it).transaction().splitCount(); | ||||
199 | const int maxSplits = 11; | ||||
200 | for (int i = 0; i < maxSplits; ++i) { | ||||
201 | const QString valueVariable = QString("$SPLITVALUE%1").arg(i); | ||||
202 | const QString accountVariable = QString("$SPLITACCOUNTNAME%1").arg(i); | ||||
203 | if (i < numSplits) { | ||||
204 | checkHTML.replace( valueVariable, MyMoneyUtils::formatMoney((*it).transaction().splits()[i].value().abs(), currency)); | ||||
205 | checkHTML.replace( accountVariable, (file->account((*it).transaction().splits()[i].accountId())).name()); | ||||
206 | } else { | ||||
207 | checkHTML.replace( valueVariable, " "); | ||||
The two lines here should then also use the split().value() method. Also, see my comment below. tbaumgart: The two lines here should then also use the `split().value()` method. Also, see my comment… | |||||
tbaumgart: I still see `split.shares()` here instead of `split.value()`. | |||||
philhopkins: I updated those old shares t value. | |||||
208 | checkHTML.replace( accountVariable, " "); | ||||
209 | } | ||||
tbaumgart: Indentation seems to be off here. | |||||
philhopkins: Should be fixed. | |||||
It must have slipped through, but could you name the variable numSplits (lowercase initial letter). Also, you can safely make it a const int instead of int because it may not change anyway in this context. tbaumgart: It must have slipped through, but could you name the variable `numSplits` (lowercase initial… | |||||
(*it).transaction().commodity() returns the ID (which is the ISO 4217 code). In general we display the currencySymbol() of a security/currency. tbaumgart: `(*it).transaction().commodity()` returns the ID (which is the ISO 4217 code). In general we… | |||||
If I look at what is in the database for my user, I see "USD" not the ISO code. If the ISO code is stored for other currencies, how do I convert it to the currenctSysmbol()? Something like: philhopkins: If I look at what is in the database for my user, I see "USD" not the ISO code. If the ISO code… | |||||
Well, USD is the ISO 4217 code said currency. See https://en.wikipedia.org/wiki/ISO_4217 To get the currency symbol you go as follows: const auto currencyId = (*it).transaction().commodity(); const auto currency = MyMoneyFile::instance()->currency(currencyId); checkHTML.replace("$TRANSACTIONCURRENCY", currency.tradingSymbol()); tbaumgart: Well, USD is the ISO 4217 code said currency. See https://en.wikipedia.org/wiki/ISO_4217 To… | |||||
Thanks for the code sample, I have updated the code to user your suggestion and it now prints the currency symbol. philhopkins: Thanks for the code sample, I have updated the code to user your suggestion and it now prints… | |||||
210 | } | ||||
Can you make the 11 a named constant? E.g. in the beginning of this function around line 166 by saying const int maxSplits = 11; and using maxSplits in the above for-loop? tbaumgart: Can you make the `11` a named constant? E.g. in the beginning of this function around line 166… | |||||
195 | 211 | | |||
196 | // print the check | 212 | // print the check | ||
197 | htmlPart->setHtml(checkHTML, QUrl("file://")); | 213 | htmlPart->setHtml(checkHTML, QUrl("file://")); | ||
198 | auto printer = KMyMoneyPrinter::startPrint(); | 214 | auto printer = KMyMoneyPrinter::startPrint(); | ||
199 | if (printer != nullptr) { | 215 | if (printer != nullptr) { | ||
Change these checkHTML.replace( valueVariable, MyMoneyUtils::formatMoney((*it).transaction().splits()[i].shares().abs(), currency)); checkHTML.replace( accountVariable, (file->account((*it).transaction().splits()[i].accountId())).name()); to const MyMoneyAccount splitAccount = file->account((*it).transaction().splits()[i].accountId()); const MyMoneySecurity accountCurrency = file->currency(account.currencyId()); checkHTML.replace( valueVariable, MyMoneyUtils::formatMoney((*it).transaction().splits()[i].shares().abs(), accountCurrency)); checkHTML.replace( accountVariable, splitAccount.name()); See also my explanation in the other comment. tbaumgart: Change these
```
checkHTML.replace( valueVariable, MyMoneyUtils::formatMoney((*it).transaction… | |||||
200 | #ifdef ENABLE_WEBENGINE | 216 | #ifdef ENABLE_WEBENGINE | ||
201 | htmlPart->page()->print(printer, [=] (bool) {}); | 217 | htmlPart->page()->print(printer, [=] (bool) {}); | ||
202 | #else | 218 | #else | ||
A bit of improved and more Qt style version of the above: for (int i = 0; i < 11; ++i) { const QString valueVariable = QString("$SPLITVALUE%1").arg(i); const QString accountVariable = QString("$SPLITACCOUNTNAME%1").arg(i); if (i < numSplits) { so no need to use std::string and a lot of conversion back and forth. tbaumgart: A bit of improved and more Qt style version of the above:
```
for (int i = 0; i < 11; ++i) {… | |||||
philhopkins: Shoud be fixed. | |||||
203 | htmlPart->print(printer); | 219 | htmlPart->print(printer); | ||
A split could be denominated in a different currency (depending on the account found in splits()[i].accountId() ). This is not supported with your code. See e.g. line 206 above how to do a possible conversion. tbaumgart: A split could be denominated in a different currency (depending on the account found in `splits… | |||||
I don't understand, my code is the same as line 207 as I want the value in numbers not text as line 206 gives. Am I missing something here? philhopkins: I don't understand, my code is the same as line 207 as I want the value in numbers not text as… | |||||
Sorry about the confusion. Not seeing the full context of the function here in the patch, I thought convert() does currency conversion but it rather converts a number into text. Nevertheless, we need to take into account here that a split can be denominated in a different currency than the original one (multi-currency). The shares() method of a split always returns the amount in the currency of the account/category of the split. The value() method of the split returns the value in the common currency of the transaction, hence shares()/value() is the exchange rate used when entering the transaction. Using shares() with the currency of the transaction might therefore not be correct and you get false results. Of course, you won't notice any of the above if you only have one currency on file. tbaumgart: Sorry about the confusion. Not seeing the full context of the function here in the patch, I… | |||||
I have replaced shares with value due to the potential currency problems. This should give the proper amount. philhopkins: I have replaced shares with value due to the potential currency problems. This should give the… | |||||
This might bite you. currency currently represents the currency in which the selected split's account/category is denominated. Depending on the circumstances, this may not be the currency in which the value is represented. The split's value is always given in the currency provided as (*it).transaction().commodity(). But this may be different from the currency of your split. I know this will never happen in a single currency dataset, but it can if you manage your finances in multiple currencies and we shoud be prepared, since I know of users who do that. tbaumgart: This might bite you. `currency` currently represents the currency in which the selected split's… | |||||
Thomas, Thanks for all of your advice, it has been helpful. I haven't written C++ in over 15 years so I am very rusty. With that said I am somewhat confused. I thought that by changing to value() is would show in the amount of the currency for the split but I don't understand wat else I should do here? Should I be showing the exchange rate? or something else? any advice will be appreciated. Phil philhopkins: Thomas, Thanks for all of your advice, it has been helpful. I haven't written C++ in over 15… | |||||
This is not C++ related but how the application manages the data. Each split has two attributes: shares and value. Each transaction is denominated in a currency (transaction.currency()). The sum of the value attributes over all splits of a transaction must be zero for the transaction to be balanced (that is what double entry accounting is all about), where it does not matter in which currency that happens. The shares attribute represents the units in the currency of the account that the split references. So the sum of all shares over all splits of a specific account provides the balance of the account. In the end, it depends on how you want the different currencies printed on the check. All this is only important in case one has multiple currencies. I just want to avoid that we add some code that works in single currency environments but gives the user a headache when using it with multiple currencies. My initial code suggestion was to use the share value and print the respective currency symbol. This is correct and easy to accomplish. Everything else is much more complex as there are more corner cases. Hope that makes it a bit more meaningful. If not, please let me know. tbaumgart: This is not C++ related but how the application manages the data. Each split has two attributes… | |||||
Thanks, I agree I want this to be a generalized solution, not one that just works for me. I will add a variable to allow the curriency for the transaction to be printed. I am not sure if that is enough, though. Is there anything ese that I should include? Any I do realize that this is not a C++ issue but one of properly offering clear data to the user. Thanks again for your help. Phil philhopkins: Thanks, I agree I want this to be a generalized solution, not one that just works for me. I… | |||||
I do notice that for the transaction this seems to be transaction().commodity(). Phil philhopkins: I do notice that for the transaction this seems to be transaction().commodity().
Phil | |||||
transaction.commodity() is the currency of the account in which the transaction was entered (either manually or via import). So in most cases (when the user also prints the check from this account) it is OK to use it for our purpose here. tbaumgart: `transaction.commodity()` is the currency of the account in which the transaction was entered… | |||||
204 | #endif | 220 | #endif | ||
205 | } | 221 | } | ||
206 | 222 | | |||
tbaumgart: Variable names need to be adjusted according to the above change. | |||||
philhopkins: Fixed | |||||
207 | // mark the transaction as printed | 223 | // mark the transaction as printed | ||
208 | markAsPrinted(*it); | 224 | markAsPrinted(*it); | ||
209 | } | 225 | } | ||
210 | 226 | | |||
211 | PluginSettings::setPrintedChecks(d->m_printedTransactionIdList); | 227 | PluginSettings::setPrintedChecks(d->m_printedTransactionIdList); | ||
212 | delete htmlPart; | 228 | delete htmlPart; | ||
213 | } | 229 | } | ||
214 | 230 | | |||
Show All 28 Lines |
Change these
to