<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&#39;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&#39;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&#39;t, maybe that&#39;s the problem

I&#39;m building my build environment, couldn&#39;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 &quot;1,123.&quot;. It seems that the problem is caused by the &quot;,&quot; 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) &gt; 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&amp;r2=1174787 for the details of the fix which is now in SVN.</pre>
 </blockquote>





 <p>On September 16th, 2010, 9:25 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;">I just found two possible issues, both related with the String constructor of libalkimia.

The first one is a problem when receiving &quot;.&quot; as input.
Following the old constructor, it should create an MyMoneyMoney object containing &quot;0/1&quot;, but it tries to create an mpq_class object using &quot;/1&quot; 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 &quot;.&quot; as input
136  if (res.length() == 0 )
137    res += &quot;0&quot;;
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: &quot;x1.234,567- EUR&quot;.
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 &#39;-&#39; as well as &#39;()&#39; for negative entry
    // we would have to replase &#39;=&#39; with &#39;+=&#39; in the next line
    // Also, the logic in kMyMoneyEdit::theTextChanged has to be
    // adjusted to allow both methods at the same time.
    negChars = QString(&quot;()&quot;);
  }
  validChars += negChars;

Has this been considered in the alkvalue string constructor?</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 modified the AlkValue ctor to handle both of them. Now available in SVN.</pre>
<br />








<p>- Thomas</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>