<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/120507/">https://git.reviewboard.kde.org/r/120507/</a>
</td>
</tr>
</table>
<br />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I wouldn't wait for a ship it since, as always the patch is huge, I don't want to speak for others but it's really hard for anybody to make a relevant review this way. I know we had our ups and downs so take this advice as a sign that I care enough to not leave this review request unanswered. If you find the patch good just ship it.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Also I support the effort of trying to improve and to avoid duplication but all I see is that 'CSVDialog' keeps getting new members and judging by their names it has become the most complex state machine I've ever seen.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Here are some nitpicks I came up with by looking at the request.</p></pre>
<br />
<div>
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://git.reviewboard.kde.org/r/120507/diff/1/?file=316711#file316711line302" style="color: black; font-weight: bold; text-decoration: underline;">kmymoney/plugins/csvimport/investmentwizardpage.ui</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">302</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <string><p align="center">If necessary, select column containing fee.<span class="ew"> </span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">296</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <string><p align="center">If necessary, select column containing fee.<span class="ew"> </span></pre></td>
</tr>
</tbody>
</table>
<div style="margin-left: 2em;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Do we really need to center all strings? As a translator I found it verry hard to translate the csvimporter because of the many rich text directives. Please take KMyMoney UI as an example.</p></pre>
</div>
</div>
<br />
<div>
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://git.reviewboard.kde.org/r/120507/diff/1/?file=316712#file316712line146" style="color: black; font-weight: bold; text-decoration: underline;">kmymoney/plugins/csvimport/investprocessing.h</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">public:</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">144</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="ew"> </span></pre></td>
</tr>
</tbody>
</table>
<div style="margin-left: 2em;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Whitespace errors (useless whitespace in empty lines or at the end of line) are pretty well highlighted by reviewboard.</p></pre>
</div>
</div>
<br />
<p>- Cristian Oneț</p>
<br />
<p>On October 5th, 2014, 6:34 p.m. UTC, Allan Anderson wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for KMymoney.</div>
<div>By Allan Anderson.</div>
<p style="color: grey;"><i>Updated Oct. 5, 2014, 6:34 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kmymoney
</div>
<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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Improve functionality across distros.
It was also the intention to save the windows settings, but it was realised that there was a liklihood that the next file to be imported could have quite different column width and number of rows. So, it was decided to display the whole of the file being imported instead.<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
Also,the UI has been improved to avoid displaying split rows, with all possible rows being displayed, the window height aligning with the rows, and the width displaying complete columns, as far as the screen allowed. Further, I reworked some areas of the code to avoid duplication and improve re-usability.</p></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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Tested with numerous banking and investment files from different sources, on Linux Mint KDE and Ubuntu Unity.</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>kmymoney/plugins/csvimport/lines-datewizardpage.ui <span style="color: grey">(884f3ba)</span></li>
<li>kmymoney/plugins/csvimport/separatorwizardpage.ui <span style="color: grey">(b6dba9f)</span></li>
<li>kmymoney/plugins/csvimport/bankingwizardpage.ui <span style="color: grey">(0d6a4a8)</span></li>
<li>kmymoney/plugins/csvimport/csvdialog.h <span style="color: grey">(24abd9a)</span></li>
<li>kmymoney/plugins/csvimport/csvdialog.cpp <span style="color: grey">(ab20ed4)</span></li>
<li>kmymoney/plugins/csvimport/csvdialog.ui <span style="color: grey">(61ab9ce)</span></li>
<li>kmymoney/plugins/csvimport/csvimporterplugin.h <span style="color: grey">(d2d2f6c)</span></li>
<li>kmymoney/plugins/csvimport/csvimporterplugin.cpp <span style="color: grey">(602b4d9)</span></li>
<li>kmymoney/plugins/csvimport/introwizardpage.ui <span style="color: grey">(a597d56)</span></li>
<li>kmymoney/plugins/csvimport/investmentdlg.cpp <span style="color: grey">(5e7b266)</span></li>
<li>kmymoney/plugins/csvimport/investmentwizardpage.ui <span style="color: grey">(846836e)</span></li>
<li>kmymoney/plugins/csvimport/investprocessing.h <span style="color: grey">(fa8ffa0)</span></li>
<li>kmymoney/plugins/csvimport/investprocessing.cpp <span style="color: grey">(1629e14)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/120507/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>