[Kmymoney-devel] Review Request: Migration to AlkValue
Carlos da Silva
kaduardo at gmail.com
Thu Sep 16 23:24:57 CEST 2010
> On 2010-09-12 00:53:35, Alvaro Soliverez wrote:
> > Just one thing to note about this, before I review the whole patch.
> > If there is any change to libalkimia, that has to be ported upstream to the original repository. We will only have a copy of the original code.
>
> Carlos da Silva wrote:
> Just a clarification.
> I have created the patch assuming that I can not change libalkimia, since I don't know who is responsible for it, and it is probably used in other projects.
>
> Thomas Baumgart wrote:
> The KDE finance group - where KMyMoney is a member - takes care of it. We also make sure, that we don't break things in the future. So far, KMyMoney will be the first application using it.
>
> As I write, I have fixed your problem in upstream Alkimia code. Please update from its SVN repo.
>
> Jose Villanova wrote:
> 0.07 is a valid octal and 0.08, 0.09 aren't, maybe that's the problem
>
> I'm building my build environment, couldn't test it so far :-(
>
> Carlos da Silva wrote:
> Thanks Thomas and Jose,
>
> I have updated the libalkimia from the upstram Alkimia code and it is now working.
>
> Before I update the code in the review, I have another doubt about the String constructor of libalkimia.
> Apparently, AlkValue String constructor can not deal with "1,123.". It seems that the problem is caused by the "," in the converted string, which is not supported by the GMP mpq_class class.
>
> As a temporary fix, I copied the AlkValue string constructor to MyMoneyMoney class and eliminated the thousand separator. It is around line 192 of the mymoneymoney.cpp file.
>
> 192 // take care of thousand separator
> 193 while(res.count(_thousandSeparator) > 0) {
> 194 pos = res.indexOf(_thousandSeparator);
> 195 res.remove(pos, 1);
> 196 }
>
> Is it another fix for the AlkValue class?
>
> Thomas Baumgart wrote:
> Yes, unfortunately. Please see http://websvn.kde.org/trunk/playground/office/alkimia/libalkimia/alkvalue.cpp?r1=1174435&r2=1174787 for the details of the fix which is now in SVN.
I just found two possible issues, both related with the String constructor of libalkimia.
The first one is a problem when receiving "." as input.
Following the old constructor, it should create an MyMoneyMoney object containing "0/1", but it tries to create an mpq_class object using "/1" as input.
I have included the following code right before adding the fraction to the String res (it would be around line 135 of alkvalue.cpp file):
135 //Fix for dealing with "." as input
136 if (res.length() == 0 )
137 res += "0";
138 res += fraction;
The second issue is related with negative values.
The string constructor of alkvalue.cpp fails the testNegativeStringConstructor unit test with the following input: "x1.234,567- EUR".
Because of that, I have kept the code that deals with negative values from the old mymoneymoney.cpp file, which contains a FIXME comment.
if (_negativeMonetarySignPosition == ParensAround) {
// FIXME: If we want to allow '-' as well as '()' for negative entry
// we would have to replase '=' with '+=' in the next line
// Also, the logic in kMyMoneyEdit::theTextChanged has to be
// adjusted to allow both methods at the same time.
negChars = QString("()");
}
validChars += negChars;
Has this been considered in the alkvalue string constructor?
- Carlos
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://svn.reviewboard.kde.org/r/5310/#review7550
-----------------------------------------------------------
On 2010-09-13 17:13:45, Carlos da Silva wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://svn.reviewboard.kde.org/r/5310/
> -----------------------------------------------------------
>
> (Updated 2010-09-13 17:13:45)
>
>
> Review request for kmymoney.
>
>
> Summary
> -------
>
> Porting the mymoneymoney class to AlkValue (from alkimia) in order to deal with overflows (Bug 245214).
> The libalkimia, included in the patch, requires the installation of the GMP library (http://gmplib.org/).
>
> Notes about the development of the patch are in http://community.kde.org/KMyMoney/PlayingWithAlkValue.
>
> This is an updated version after the fixes in the upstream libalkimia.
>
>
> This addresses bug 245214.
> https://bugs.kde.org/show_bug.cgi?id=245214
>
>
> Diffs
> -----
>
> /trunk/extragear/office/kmymoney/CMakeLists.txt 1174934
> /trunk/extragear/office/kmymoney/kmymoney/CMakeLists.txt 1174934
> /trunk/extragear/office/kmymoney/kmymoney/mymoney/CMakeLists.txt 1174934
> /trunk/extragear/office/kmymoney/kmymoney/mymoney/mymoneymoney.h 1174934
> /trunk/extragear/office/kmymoney/kmymoney/mymoney/mymoneymoney.cpp 1174934
> /trunk/extragear/office/kmymoney/kmymoney/mymoney/mymoneymoneytest.h 1174934
> /trunk/extragear/office/kmymoney/kmymoney/mymoney/mymoneymoneytest.cpp 1174934
> /trunk/extragear/office/kmymoney/kmymoney/plugins/CMakeLists.txt 1174934
> /trunk/extragear/office/kmymoney/kmymoney/plugins/csvimport/CMakeLists.txt 1174934
> /trunk/extragear/office/kmymoney/kmymoney/plugins/icalendarexport/CMakeLists.txt 1174934
> /trunk/extragear/office/kmymoney/kmymoney/plugins/ofximport/CMakeLists.txt 1174934
> /trunk/extragear/office/kmymoney/kmymoney/plugins/printcheck/CMakeLists.txt 1174934
> /trunk/extragear/office/kmymoney/kmymoney/plugins/reconciliationreport/CMakeLists.txt 1174934
> /trunk/extragear/office/kmymoney/libalkimia/CMakeLists.txt PRE-CREATION
> /trunk/extragear/office/kmymoney/libalkimia/alkvalue.h PRE-CREATION
> /trunk/extragear/office/kmymoney/libalkimia/alkvalue.cpp PRE-CREATION
>
> Diff: http://svn.reviewboard.kde.org/r/5310/diff
>
>
> Testing
> -------
>
> All unit tests present in the code.
> Tested with two of my real files, and the files posted in the bug description.
>
>
> Thanks,
>
> Carlos
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/kmymoney-devel/attachments/20100916/e4128ac6/attachment-0001.htm
More information about the KMyMoney-devel
mailing list