<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 />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On October 7th, 2014, 12:47 p.m. UTC, <b>Cristian Oneț</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;"><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>
</blockquote>
<p>On October 7th, 2014, 6:19 p.m. UTC, <b>Allan Anderson</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Thanks Cristian. As you say, the patch is huge, although that mainly comes from several (nearly all) UI files having changes. Alvaro mentioned recently that he doesn't use QTDesigner because of its tendency to bloat the files, I suspect rather like Windows Office. If I were more confident, I too would use a text editor. In addition, some code I wanted to change was also going to need to be reused, so I extracted it into two separate routines, and the original large routine I was able to dispense with. I don't foresee any further major changes in the plugin!
This patch and the recent associated "BUG:339044 REVIEW:120260 - Fix CSV import wizard not working correctly on Windows", between them removed 431 lines, so I think I've moved in the right direction.</p></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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Committed.
remote: This commit is available for viewing at:
remote: http://commits.kde.org/kmymoney/50f7b6c47ad4f326b7cd6b2e0f8746b8391b3810</p></pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On October 7th, 2014, 12:47 p.m. UTC, <b>Cristian Oneț</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<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>
<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>
</blockquote>
<p>On October 7th, 2014, 6:19 p.m. UTC, <b>Allan Anderson</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">You did mention this before, but now I understand to what you are referring. May be this is another example of Designer bloat. I might be able to get a simpler result from producing these UI labels via code. I'll keep my eye on that.</p></pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; 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;">The UI heading labels have been trimmed. They are still cemtered, but no longer HTML-based</p></pre>
<br />
<p>- Allan</p>
<br />
<p>On October 16th, 2014, 12:15 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. 16, 2014, 12:15 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>