[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