Bug 328050 - KMymoney does not support space as thousand separator
Summary: KMymoney does not support space as thousand separator
Status: RESOLVED FIXED
Alias: None
Product: kmymoney
Classification: Applications
Component: general (show other bugs)
Version: 4.6.4
Platform: Microsoft Windows Microsoft Windows
: NOR normal
Target Milestone: ---
Assignee: KMyMoney Devel Mailing List
URL:
Keywords:
: 300070 (view as bug list)
Depends on:
Blocks:
 
Reported: 2013-11-25 09:23 UTC by Misi
Modified: 2021-04-16 09:06 UTC (History)
5 users (show)

See Also:
Latest Commit:
Version Fixed In: 4.8.3,5.0.2
ralf.habacker: version_5_applied+
ralf.habacker: Version_5+


Attachments
related method calls (45.44 KB, text/plain)
2017-07-13 07:10 UTC, Ralf Habacker
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Misi 2013-11-25 09:23:49 UTC
The language of my KDE program is hungarian. I"d like using the thousand separator a space, and the number of decimal 0. I set up these in the KDE language, number and time option, but doesn"t work properly after restart of PC also not.
Could you help me?

Reproducible: Always
Comment 1 Thomas Baumgart 2013-11-25 11:18:24 UTC
Did you set those up on the 'numbers' tab? You need to modify the settings on the 'money' tab of the locale settings in KDE which should be picked up by KMyMoney.
Comment 2 Misi 2013-11-25 11:27:44 UTC
Yes, I set up both tab numbers and money. Then I click Apply and restart my computer also, but there isn't any changes.
Comment 3 Cristian Oneț 2014-08-22 06:58:56 UTC
*** Bug 300070 has been marked as a duplicate of this bug. ***
Comment 4 Cristian Oneț 2014-08-22 07:09:42 UTC
It could be that KMyMoney will not allow a space as numbers separator, need to check this.
Comment 5 Chris 2017-02-24 12:51:22 UTC
Is there any progress on this issue ?

I'm french, and I used to have a space as group separator for numbers. Currently, kmymoney 4.8.0 (running with KDE 4.14.29 on Linux Arch 4.9.11-1) is not able to be configured this way.

Thx.
Comment 6 Ralf Habacker 2017-07-12 22:12:26 UTC
According to https://docs.oracle.com/cd/E19455-01/806-0169/overview-9/index.html the affected languages are Canadian, German, Dansk, Finish, French and Swedish.

Setting thousand separator is handled by MyMoneyMoney::setThousandSeparator() which simply ignores ' ' as separator. Changing is really simple by removing some lines inside the mentioned method.

The much bigger task is to verify that removing the former "ignore" behavior does not introduce unwanted regressions.
Comment 7 Jack 2017-07-12 22:17:37 UTC
(Candian French and Canadian English are two separate languages)
Does this only affect KMM, or is there anything at a higher level in KDE which should be changed also?
Comment 8 Ralf Habacker 2017-07-12 22:55:05 UTC
(In reply to Jack from comment #7)
> (Candian French and Canadian English are two separate languages)
> Does this only affect KMM, or is there anything at a higher level in KDE
> which should be changed also?

This does only affect KMM only. I kmm main function the KDE thousand separator system settings is copied into kmm, see below

main.cpp
138: // setup the MyMoneyMoney locale settings according to the KDE settings
139: MyMoneyMoney::setThousandSeparator(KGlobal::locale()->monetaryThousandsSeparator()[0]);

Because ' ' as thousand separator is ignored the whole application does not know anything about this value except the cases where KGlobal::locale()->monetaryThousandsSeparator() is used directly, which is 
1. MyMoneyQifProfile::clear()
2. kMyMoneyMoneyValidator::validate()

An interesting location is inside GncObject::hide() 
... 
      case MONEY2:
        in = MyMoneyMoney(data);
        if (data == "-1/0") in = MyMoneyMoney();
        mresult  = MyMoneyMoney(m_moneyHideFactor) * in;
        mresult.convert(10000);
        MyMoneyMoney::setThousandSeparator(' ');
        result = mresult.formatMoney("", 2);
        break;

Because ' ' is converted to '', which means no thousand separator this could be rewritten to 

        result = mresult.formatMoney("", 2, false);

The third parameter is a value for showThousandSeparator.

I'm talking about code because the task is to inspect any call of formatMoney() if it falls into one of the following category: 

1. used for debug output
2. used for gui output
3. used for further processing
4. used for file export

Category 1. and 2. are uncritical (from my current knowledge) but the other need to be inspected  and optional fixed like the mentioned example.

<offtopic>Refactoring the above mentioned code fragment also fixes an additional bug: Because the original thousand separator is not saved and restored (as it is done in the qif reader), this piece of code kills the thousand separator in the whole application until kmymoney will be been restarted.</offtopic>
Comment 9 Ralf Habacker 2017-07-13 07:10:49 UTC
Created attachment 106587 [details]
related method calls

With the help of the following script I got a list of related calls:
for i in $(find -name '*.cpp' | grep -v test.cpp); do  grep -A4 -B4 -HnP "formatMoney\(.*[, ]+[0-9]+[ ]*\)" $i; done > bug-328050-grep-result.txt

This list excludes all calls with 

- formatMoney(..., false) -> safe case
- formatMoney(..., true)  -> not used
- all calls in test code
Comment 10 Ralf Habacker 2018-06-04 08:35:16 UTC
/kmymoney/plugins/ofximport/dialogs/mymoneyofxconnector.cpp
- The ofx spec 2.2 at http://www.ofx.net/downloads/OFX%202.2.pdf mentions in chapter '3.2.9.1 Basic Format' Format: A-32 that amounts should not include any punctuation separating thousands, millions, and so forth, so the solution is to not use thousand separator anymore here

./kmymoney/plugins/csvexport/csvwriter.cpp
- there are two cases were the money are formatted with thousand separator, but the separator is removed afterwards -> do not format with thousand separator

./kmymoney/plugins/weboob/dialogs/mapaccount.cpp
- The formatted money is displayed in an account list -> safe to use thousand separator

./kmymoney/views/kgloballedgerview.cpp
- the money is formatted for internal debugging -> safe to use thousand separator

./kmymoney/reports/listtable.cpp
- the money is formatted for the html report -> safe to use thousand separator

/kmymoney/widgets/kbudgetvalues.cpp
- the money is formatted for a dialog box -> safe to use thousand separator
Comment 11 Thomas Baumgart 2018-06-04 09:40:03 UTC
Git commit 0e75cd0973d7e19d09efd4ac99a4eee0e521f6c5 by Thomas Baumgart.
Committed on 04/06/2018 at 09:40.
Pushed by tbaumgart into branch 'master'.

Adjust usage of thousands separator

Modifications according to the analysis in bug 328050

OFX: the modified code is commented out and only used for test purposes
CSV: just a simplification of the code, no change in functionality

M  +2    -7    kmymoney/plugins/csv/export/csvwriter.cpp
M  +13   -13   kmymoney/plugins/ofx/import/dialogs/mymoneyofxconnector.cpp

https://commits.kde.org/kmymoney/0e75cd0973d7e19d09efd4ac99a4eee0e521f6c5
Comment 12 Ralf Habacker 2018-06-04 10:14:08 UTC
./kmymoney/widgets/transaction.cpp
- the money is formatted for transactions in the ledger view -> safe to use

./kmymoney/converter/mymoneyqifprofile.cpp
- qif profile editor setup thousand separator for its own -> safe to use

./kmymoney/converter/transactionmatchfinder.cpp
- formatted money is used for debug output -> safe to use

./kmymoney/converter/mymoneyqifwriter.cpp
- same as qif profile editor -> safe to use

./kmymoney/converter/mymoneyqifreader.cpp
- formatted money is used for debug output -> safe to use

./kmymoney/converter/mymoneystatementreader.cpp
- formatted money is used for debug output -> safe to use

./kmymoney/dialogs/kfindtransactiondlg.cpp
- formatted money is used for ui output -> safe to use

./kmymoney/models/accountsmodel.cpp
- formatted money is used for displaying vat rate in accounts table -> safe to use

/kmymoney/mymoney/mymoneytransaction.cpp
- formatted money is used for generating hashes -> supporting space thousand separator is safe to use

./kmymoney/mymoney/storage/mymoneystoragedump.cpp
- formatted money is used for dumping storage -> supporting space thousand separator is safe to use

./kmymoney/mymoney/storage/mymoneystoragesql.cpp:2410
- formatted money is saved as column 'priceFormatted' to database only for informational purpose -> will now write space as thousand separator too, which is safe to use

./kmymoney/wizards/endingbalancedlg/kendingbalancedlg.cpp
- formatted money is used for debug output -> safe to use

./kmymoney/wizards/newloanwizard/keditloanwizard.cpp:129
- thousand separator is not shown in loan rate and other money related fields -> need to be fixed in MyMoneyEdit::setValue()

./kmymoney/wizards/newloanwizard/knewloanwizard.cpp:!67
./kmymoney/wizards/newloanwizard/knewloanwizard.cpp:239
./kmymoney/wizards/newloanwizard/knewloanwizard.cpp:415
- formatted money is used for displaying interest rate in loan wizard -> safe to use

./kmymoney/wizards/newaccountwizard/knewaccountwizard.cpp
- formatted money is used for displaying money related edit fields in new account wizard -> safe to use
- thousand separator is not shown in loan rate and other money related fields -> need to be fixed in MyMoneyEdit::setValue()
Comment 13 Ralf Habacker 2018-06-04 10:17:49 UTC
> ./kmymoney/wizards/newloanwizard/keditloanwizard.cpp:129
> - thousand separator is not shown in loan rate and other money related
> fields -> need to be fixed in MyMoneyEdit::setValue()

MyMoneyEdit cares about only allowing the configured language specific thousand separators (tested with de and en_us language)
Comment 14 Ralf Habacker 2018-06-04 10:31:15 UTC
(In reply to Thomas Baumgart from comment #11)
> Git commit 0e75cd0973d7e19d09efd4ac99a4eee0e521f6c5 by Thomas Baumgart.
> Committed on 04/06/2018 at 09:40.
> Pushed by tbaumgart into branch 'master'.
Because master diverged too much from 4.8 I'm going to recreate similar patch(es) for 4.8
Comment 15 Ralf Habacker 2018-06-04 11:21:16 UTC
(In reply to Thomas Baumgart from comment #11)
> Git commit 0e75cd0973d7e19d09efd4ac99a4eee0e521f6c5 by Thomas Baumgart.
> Committed on 04/06/2018 at 09:40.
> Pushed by tbaumgart into branch 'master'.
> 
> Adjust usage of thousands separator
> 
> Modifications according to the analysis in bug 328050
> 
> OFX: the modified code is commented out and only used for test purposes
> CSV: just a simplification of the code, no change in functionality
> 
> M  +2    -7    kmymoney/plugins/csv/export/csvwriter.cpp
> M  +13   -13   kmymoney/plugins/ofx/import/dialogs/mymoneyofxconnector.cpp
> 
> https://commits.kde.org/kmymoney/0e75cd0973d7e19d09efd4ac99a4eee0e521f6c5
You fixed this in CsvWriter::writeTransactionEntry() but not in  CsvWriter::writeSplitEntry(), see https://cgit.kde.org/kmymoney.git/tree/kmymoney/plugins/csv/export/csvwriter.cpp#n256

BTW: It looks that removing this replacement, which intention was to fixes clash with field separator, introduces a new bug resulting into invalid csv file in case the memo field contains the field delimiter.
Comment 16 Ralf Habacker 2018-06-04 11:21:58 UTC
Hi Thomas, see my 4.8 staging branch for this bug - https://github.com/rhabacker/kmymoney/tree/328050-thousand-separator
Comment 17 Ralf Habacker 2018-06-04 11:28:46 UTC
Git commit 06322c4b889b019c6a15c7f140dd5d43b9864f5f by Ralf Habacker.
Committed on 04/06/2018 at 06:41.
Pushed by habacker into branch '4.8'.

Fix resetting of thousand separator in gnc reader

M  +1    -2    kmymoney/converter/mymoneygncreader.cpp

https://commits.kde.org/kmymoney/06322c4b889b019c6a15c7f140dd5d43b9864f5f
Comment 18 Ralf Habacker 2018-06-04 11:28:46 UTC
Git commit be41c8d8e23b665619505bf0577be09f11510d05 by Ralf Habacker.
Committed on 04/06/2018 at 10:21.
Pushed by habacker into branch '4.8'.

Fix formatting MyMoneyMoney instances in csvwriter

Format the MoneyMoney instance without the thousands separator instead
of formatting it with thousands separators and then removing it.

M  +2    -3    kmymoney/plugins/csvexport/csvwriter.cpp

https://commits.kde.org/kmymoney/be41c8d8e23b665619505bf0577be09f11510d05
Comment 19 Ralf Habacker 2018-06-04 11:28:46 UTC
Git commit f5d5764af24ef75f889ae3ba2200ce616e9270ea by Ralf Habacker.
Committed on 04/06/2018 at 10:20.
Pushed by habacker into branch '4.8'.

In ofx connector do not format MyMoneyMoney instances with thousand separator

The ofx spec 2.2 at http://www.ofx.net/downloads/OFX%202.2.pdf mentions
in chapter '3.2.9.1 Basic Format' Format: A-32 that amounts should not
include any punctuation separating thousands, millions, and so forth.

M  +13   -13   kmymoney/plugins/ofximport/dialogs/mymoneyofxconnector.cpp

https://commits.kde.org/kmymoney/f5d5764af24ef75f889ae3ba2200ce616e9270ea
Comment 20 Ralf Habacker 2018-06-04 11:46:51 UTC
Git commit a5edf47d2566a3259607c13edf480b48244ad80c by Ralf Habacker.
Committed on 04/06/2018 at 11:26.
Pushed by habacker into branch '4.8'.

Show thousand separator in money edit field
FIXED-IN:4.8.3

M  +1    -2    kmymoney/widgets/kmymoneyedit.cpp

https://commits.kde.org/kmymoney/a5edf47d2566a3259607c13edf480b48244ad80c