[Kmymoney-devel] Review Request 112947: BUG:322768 - Interest category and amount disappear when new fee entered in Dividend. Also, fixes for KEditWidget visibility issues.
Cristian Oneț
onet.cristian at gmail.com
Thu Sep 26 12:48:51 UTC 2013
> On Sept. 26, 2013, 10 a.m., Cristian Oneț wrote:
> > kmymoney/dialogs/investactivities.cpp, line 394
> > <http://git.reviewboard.kde.org/r/112947/diff/1/?file=192722#file192722line394>
> >
> > does the ending '///' mean anything? if this is a placeholder to search for maybe a textual comment would be better suited
>
> Allan Anderson wrote:
> Oops. Yes, it's a placeholder, that I use to search for changes I've made. I tend not to use it much nowadays. I should have removed it, but it was getting very late and I slipped up. There may be one or two others as well.
Yes there are other I only signaled this one, no need to rush with the patch :).
> On Sept. 26, 2013, 10 a.m., Cristian Oneț wrote:
> > kmymoney/dialogs/investtransactioneditor.cpp, line 499
> > <http://git.reviewboard.kde.org/r/112947/diff/1/?file=192724#file192724line499>
> >
> > 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();
> > }
>
> Allan Anderson wrote:
> I like that. I had to add the declaration for label 'l', and add tests for it to avoid crashing when not using the form.
> Perhaps I should try similar for the interest method.
Yes, the label must be tested for it's existence.
> On Sept. 26, 2013, 10 a.m., Cristian Oneț wrote:
> > kmymoney/dialogs/investtransactioneditor.cpp, line 528
> > <http://git.reviewboard.kde.org/r/112947/diff/1/?file=192724#file192724line528>
> >
> > 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.
>
> Allan Anderson wrote:
> In general, then that is what I would do. I decided not to do it here because it provided immediate information on what was dealt with where, with there being so many different activity types.
> However, I could make it a comment instead, but I would like to leave the information there, for clarity.
> Perhaps also remove the existing comment above which doesn't seem to apply any more?
Please feel free to remove any comments which are not valid anymore (the same applies for commented code).
> On Sept. 26, 2013, 10 a.m., Cristian Oneț wrote:
> > kmymoney/dialogs/transactioneditor.cpp, line 462
> > <http://git.reviewboard.kde.org/r/112947/diff/1/?file=192725#file192725line462>
> >
> > If the category id is cleared here then why shouldn't the category editor widget be cleared two lines bellow?
>
> Allan Anderson wrote:
> Here, I deliberately left in the '///' as I felt a comment was needed, but I'll reposition them slightly.
>
> This was the cause of the problem. Here, the text was the category name from the existing interest category, but we are now here because a fee category has just been added. If cleared here, the name of the interest category gets removed and has to be reentered.
OK, I understand that but my dilemma is this: Why is it OK to clear the 'categoryId' and not clear the category name held by the edit widget? I'll look into this a bit more later.
> On Sept. 26, 2013, 10 a.m., Cristian Oneț wrote:
> > kmymoney/dialogs/transactioneditor.cpp, line 464
> > <http://git.reviewboard.kde.org/r/112947/diff/1/?file=192725#file192725line464>
> >
> > Are you sure that this is valid since categoryId.clear() is called above? This only keeps the data in the widget.
>
> Allan Anderson wrote:
> Yes it is. As explained above, the data in the widget is the name of the interest category, but we're dealing now with a new fee category. If we clear this text, the interest category has lost its name and it needs to be added again once the fee category dialog closes.
> It definitely fixes the issue, but whether there is something deeper, I don't know.
See my question above, when accepting the transaction what will be the value of 'categoryId' if it has been cleared and only the name was kept? Is it being recomputed based on the name which was kept?
- Cristian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/112947/#review40849
-----------------------------------------------------------
On Sept. 26, 2013, 12:03 a.m., Allan Anderson wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112947/
> -----------------------------------------------------------
>
> (Updated Sept. 26, 2013, 12:03 a.m.)
>
>
> Review request for KMymoney.
>
>
> Description
> -------
>
> - 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.
>
>
> This addresses bug 322768.
> http://bugs.kde.org/show_bug.cgi?id=322768
>
>
> Diffs
> -----
>
> kmymoney/dialogs/investactivities.cpp 50f33ed
> kmymoney/dialogs/investtransactioneditor.h 3e62c2a
> kmymoney/dialogs/investtransactioneditor.cpp e9f87fb
> kmymoney/dialogs/transactioneditor.cpp 39049cf
> kmymoney/widgets/transactioneditorcontainer.h f77b195
>
> Diff: http://git.reviewboard.kde.org/r/112947/diff/
>
>
> Testing
> -------
>
> Entering and editing various transaction types to ensure only the appropriate widgets became visible or hidden when required.
>
>
> Thanks,
>
> Allan Anderson
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kmymoney-devel/attachments/20130926/9f9405c4/attachment-0001.html>
More information about the KMyMoney-devel
mailing list