<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://svn.reviewboard.kde.org/r/5310/">http://svn.reviewboard.kde.org/r/5310/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On September 12th, 2010, 12:53 a.m., <b>Alvaro Soliverez</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
<p>On September 12th, 2010, 9:45 a.m., <b>Carlos da Silva</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
<p>On September 12th, 2010, 12:16 p.m., <b>Thomas Baumgart</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
<p>On September 12th, 2010, 2:31 p.m., <b>Jose Villanova</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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 :-(</pre>
</blockquote>
<p>On September 12th, 2010, 10:05 p.m., <b>Carlos da Silva</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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?</pre>
</blockquote>
<p>On September 13th, 2010, 8:25 a.m., <b>Thomas Baumgart</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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?</pre>
<br />
<p>- Carlos</p>
<br />
<p>On September 13th, 2010, 5:13 p.m., Carlos da Silva wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://svn.reviewboard.kde.orgrb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for kmymoney.</div>
<div>By Carlos da Silva.</div>
<p style="color: grey;"><i>Updated 2010-09-13 17:13:45</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">All unit tests present in the code.
Tested with two of my real files, and the files posted in the bug description.</pre>
</td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://bugs.kde.org/show_bug.cgi?id=245214">245214</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/trunk/extragear/office/kmymoney/CMakeLists.txt <span style="color: grey">(1174934)</span></li>
<li>/trunk/extragear/office/kmymoney/kmymoney/CMakeLists.txt <span style="color: grey">(1174934)</span></li>
<li>/trunk/extragear/office/kmymoney/kmymoney/mymoney/CMakeLists.txt <span style="color: grey">(1174934)</span></li>
<li>/trunk/extragear/office/kmymoney/kmymoney/mymoney/mymoneymoney.h <span style="color: grey">(1174934)</span></li>
<li>/trunk/extragear/office/kmymoney/kmymoney/mymoney/mymoneymoney.cpp <span style="color: grey">(1174934)</span></li>
<li>/trunk/extragear/office/kmymoney/kmymoney/mymoney/mymoneymoneytest.h <span style="color: grey">(1174934)</span></li>
<li>/trunk/extragear/office/kmymoney/kmymoney/mymoney/mymoneymoneytest.cpp <span style="color: grey">(1174934)</span></li>
<li>/trunk/extragear/office/kmymoney/kmymoney/plugins/CMakeLists.txt <span style="color: grey">(1174934)</span></li>
<li>/trunk/extragear/office/kmymoney/kmymoney/plugins/csvimport/CMakeLists.txt <span style="color: grey">(1174934)</span></li>
<li>/trunk/extragear/office/kmymoney/kmymoney/plugins/icalendarexport/CMakeLists.txt <span style="color: grey">(1174934)</span></li>
<li>/trunk/extragear/office/kmymoney/kmymoney/plugins/ofximport/CMakeLists.txt <span style="color: grey">(1174934)</span></li>
<li>/trunk/extragear/office/kmymoney/kmymoney/plugins/printcheck/CMakeLists.txt <span style="color: grey">(1174934)</span></li>
<li>/trunk/extragear/office/kmymoney/kmymoney/plugins/reconciliationreport/CMakeLists.txt <span style="color: grey">(1174934)</span></li>
<li>/trunk/extragear/office/kmymoney/libalkimia/CMakeLists.txt <span style="color: grey">(PRE-CREATION)</span></li>
<li>/trunk/extragear/office/kmymoney/libalkimia/alkvalue.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>/trunk/extragear/office/kmymoney/libalkimia/alkvalue.cpp <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="http://svn.reviewboard.kde.org/r/5310/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>