<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://git.reviewboard.kde.org/r/112947/">http://git.reviewboard.kde.org/r/112947/</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;">Reading http://kmymoney2.sourceforge.net/kde4/online-manual/details.investments.ledger.html#idp38934400 I didn't manage to get a good idea of which widgets should be visible or not depending on the investment action so I can't say anything about the functionality (other then it looks good and is without flickering). Maybe it would be a good idea to document each action in more detail, but I'm not requesting you this. The rest of my comments are strictly with my programmer's hat on.</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="http://git.reviewboard.kde.org/r/112947/diff/1/?file=192722#file192722line394" style="color: black; font-weight: bold; text-decoration: underline;">kmymoney/dialogs/investactivities.cpp</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; ">void Div::showWidgets(void) const</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">394</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">QTimer</span><span class="o">::</span><span class="n">singleShot</span><span class="p">(</span><span class="mi">1</span><span class="p">,</span> <span class="n">w</span><span class="p">,</span> <span class="n">SLOT</span><span class="p">(</span><span class="n">hide</span><span class="p">()));</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">394</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">w</span><span class="o">-></span><span class="n">hide</span><span class="p">();</span><span class="c1">///</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">does the ending '///' mean anything? if this is a placeholder to search for maybe a textual comment would be better suited</pre>
</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="http://git.reviewboard.kde.org/r/112947/diff/1/?file=192722#file192722line701" style="color: black; font-weight: bold; text-decoration: underline;">kmymoney/dialogs/investactivities.cpp</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; ">void IntInc::showWidgets(void) const</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">701</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span> <span class="p">(</span><span class="n">w</span><span class="p">)</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">701</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span> <span class="p">(</span><span class="n">w</span><span class="p">)</span><span class="hl"> </span><span class="n"><span class="hl">w</span></span><span class="o"><span class="hl">-></span></span><span class="n"><span class="hl">hide</span></span><span class="p"><span class="hl">();</span></span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Please run astyle-kmymoney.sh before pushing the commit.</pre>
</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="http://git.reviewboard.kde.org/r/112947/diff/1/?file=192724#file192724line499" style="color: black; font-weight: bold; text-decoration: underline;">kmymoney/dialogs/investtransactioneditor.cpp</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; ">void InvestTransactionEditor::slotUpdateFeeCategory(const QString& id)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">496</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">QLabel</span><span class="o">*</span> <span class="n">l</span> <span class="o">=</span> <span class="k">dynamic_cast</span><span class="o"><</span><span class="n">QLabel</span><span class="o">*></span><span class="p">(</span><span class="n">haveWidget</span><span class="p">(</span><span class="s">"fee-amount-label"</span><span class="p">));</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">499</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span> <span class="p">((</span><span class="n">d</span><span class="o">-></span><span class="n">m_activity</span><span class="o">-></span><span class="n">type</span><span class="p">()</span> <span class="o">==</span> <span class="n">MyMoneySplit</span><span class="o">::</span><span class="n">AddShares</span><span class="p">)</span> <span class="o">||</span><span class="p">(</span><span class="n">d</span><span class="o">-></span><span class="n">m_activity</span><span class="o">-></span><span class="n">type</span><span class="p">()</span> <span class="o">==</span> <span class="n">MyMoneySplit</span><span class="o">::</span><span class="n">RemoveShares</span><span class="p">)</span> <span class="o">||</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I would go with something like this to make it more readable.
const bool hideFee = txt.isEmpty() || d->m_activity->type() == MyMoneySplit::AddShares || .. the rest of the activities which don't have a fee
if (hideFee) {
l->setText("");
feeAmount->hide();
} else {
l->setText(i18n("Fee Amount"));
feeAmount->show();
}</pre>
</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="http://git.reviewboard.kde.org/r/112947/diff/1/?file=192724#file192724line528" style="color: black; font-weight: bold; text-decoration: underline;">kmymoney/dialogs/investtransactioneditor.cpp</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; ">void InvestTransactionEditor::slotUpdateFeeVisibility(const QString& txt)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">519</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span> <span class="p">((</span><span class="n">d</span><span class="o">-></span><span class="n">m_activity</span><span class="o">-></span><span class="n">type</span><span class="p">()</span> <span class="o"><span class="hl">!</span>=</span> <span class="n">MyMoneySplit</span><span class="o">::</span><span class="n">Dividend</span><span class="p">)</span> <span class="o"><span class="hl">&&</span></span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">528</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span> <span class="p">((</span><span class="n">d</span><span class="o">-></span><span class="n">m_activity</span><span class="o">-></span><span class="n">type</span><span class="p">()</span> <span class="o"><span class="hl">=</span>=</span> <span class="n">MyMoneySplit</span><span class="o">::</span><span class="n">Dividend</span><span class="p">)</span> <span class="o"><span class="hl">||</span></span><span class="p"><span class="hl">(</span></span><span class="n"><span class="hl">d</span></span><span class="o"><span class="hl">-></span></span><span class="n"><span class="hl">m_activity</span></span><span class="o"><span class="hl">-></span></span><span class="n"><span class="hl">type</span></span><span class="p"><span class="hl">()</span></span><span class="hl"> </span><span class="o"><span class="hl">==</span></span><span class="hl"> </span><span class="n"><span class="hl">MyMoneySplit</span></span><span class="o"><span class="hl">::</span></span><span class="n"><span class="hl">InterestIncome</span></span><span class="p"><span class="hl">)</span></span><span class="hl"> </span><span class="o"><span class="hl">||</span></span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Could this:
if (cond) {} else if (cond) {}
be written in a more readable manner? I would write something like:
if (cond) {} else {}
since all activity types are either in one or the other codition.</pre>
</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="http://git.reviewboard.kde.org/r/112947/diff/1/?file=192725#file192725line462" style="color: black; font-weight: bold; text-decoration: underline;">kmymoney/dialogs/transactioneditor.cpp</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; ">void TransactionEditor::setupCategoryWidget(KMyMoneyCategory* category, const QList<MyMoneySplit>& splits, QString& categoryId, const char* splitEditSlot, bool /* allowObjectCreation */)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">462</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">categoryId</span><span class="p">.</span><span class="n">clear</span><span class="p">();</span></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">462</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">categoryId</span><span class="p">.</span><span class="n">clear</span><span class="p">();</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">If the category id is cleared here then why shouldn't the category editor widget be cleared two lines bellow?</pre>
</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="http://git.reviewboard.kde.org/r/112947/diff/1/?file=192725#file192725line464" style="color: black; font-weight: bold; text-decoration: underline;">kmymoney/dialogs/transactioneditor.cpp</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; ">void TransactionEditor::setupCategoryWidget(KMyMoneyCategory* category, const QList<MyMoneySplit>& splits, QString& categoryId, const char* splitEditSlot, bool /* allowObjectCreation */)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">464</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">category</span><span class="o">-></span><span class="n">clearEditText</span><span class="p">();</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">464</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="c1">/// category->clearEditText(); // don't clear as could be from another widget - Bug 322768</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Are you sure that this is valid since categoryId.clear() is called above? This only keeps the data in the widget.</pre>
</div>
<br />
<p>- Cristian</p>
<br />
<p>On September 26th, 2013, 12:03 a.m. UTC, Allan Anderson wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for KMymoney.</div>
<div>By Allan Anderson.</div>
<p style="color: grey;"><i>Updated Sept. 26, 2013, 12:03 a.m.</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;">- Interest category disappears -
Steps to Reproduce:
1.Open a new Dividend transaction.
2.Enter an interest category and amount.
3.Enter a new fee category.
4.On accepting the new category, the interest category and amount have been cleared.
One line fix in kmymoney/dialogs/transactioneditor.cpp.
- Fixes for KEditWidget visibility issues.
When editing an investment transaction, interest or fee edit widgets show or hide incorrectly. This is a fairly long-standing issue, and there have been attempts to fix by delaying the show() or hide() instructions. This became more pronounced during work to allow column resizing, and Cristian produced a fix for the root cause. This fix is included here.
With the above fix in place, it became necessary to revert the delayed show() and hide() calls, and this has been done.
Of course, nothing is ever as simple as that, and another couple of issues emerged. Whether or not an interest or fee amount widget is shown depends on the presence or absence of the associated category's text. It transpired that if, say, an existing Reinvest transaction was edited to be, say a Buy transaction, the text from the Reinvest interest category was seen by the new Buy entry and resulted in the interest-amount widget being visible when none should appear. Similarly, if a transaction with a fee set is edited to be a type with no fee expected, for instance, an Add or RemoveShares, then the fee-amount widget became visible when not needed.
It was necessary to rework the slotUpdateFeeVisibility() and slotUpdateInterestVisibility() functions to take account of the new transaction type.</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;">Entering and editing various transaction types to ensure only the appropriate widgets became visible or hidden when required.</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="http://bugs.kde.org/show_bug.cgi?id=322768">322768</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>kmymoney/dialogs/investactivities.cpp <span style="color: grey">(50f33ed)</span></li>
<li>kmymoney/dialogs/investtransactioneditor.h <span style="color: grey">(3e62c2a)</span></li>
<li>kmymoney/dialogs/investtransactioneditor.cpp <span style="color: grey">(e9f87fb)</span></li>
<li>kmymoney/dialogs/transactioneditor.cpp <span style="color: grey">(39049cf)</span></li>
<li>kmymoney/widgets/transactioneditorcontainer.h <span style="color: grey">(f77b195)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/112947/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>