<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/107714/">http://git.reviewboard.kde.org/r/107714/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On January 23rd, 2013, 12:32 p.m. UTC, <b>Alessandro Russo</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;">When I added the tags I followed as example the code for memo and payee fields, so if that line of code was wrong for the memo and payee fields it will be wrong also for the tag field.</pre>
</blockquote>
<p>On January 23rd, 2013, 1:03 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;">The patch I proposed takes care of the enabling of the OK button issues, including those with TAGs, but I think you'll find that the other points I raised may still exist.
I'm just about to push this fix, so we'll soon see.</pre>
</blockquote>
<p>On January 23rd, 2013, 1:37 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;">Erm, there still is a problem with the OK button, to do with the amount and memo fields, which I thought I'd fixed as part of the patch. I'll need to look again.
So far as Tags are concerned, I've assumed that they would function in the same way as the payee field. Enter a new name, then move focus to another field. A dialog pops up to ask if a new payee should be created. With a new tag, however, when focus is moved to another field, the new name is cleared and no popup appears.
Whilst this may be what was intended, isn't it a bit inconsistent?
</pre>
</blockquote>
<p>On January 23rd, 2013, 4:31 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;">Sorry, false alarm. I had to break off, and when I returned, I found there is not still a problem with the OK button, so far as I can see. I'd looked at the code and all the calls to slotUpdateButtonState() in that area had been removed. I started to think that it must be getting called from elsewhere, but it wasn't.
I then realised that I could no longer produce that problem. So, what I had done, I don't now know, but possibly had reverted to a pre-patched version. Apologies again.
The Tag issue still is there, and there is a variation. If I create a new tag, then select that in a new schedule, then click in another field, the selection disappears, both from the line-edit and from the drop-down. Clicking the red 'x' restores it to the drop-down, which gets us back to square one.
</pre>
</blockquote>
<p>On January 23rd, 2013, 9:13 p.m. UTC, <b>Alessandro Russo</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 intended behavior of tag field is the following: After you start typing the tag in the tag field a drop down list show up with all the tag names that match the characters typed. When you stop typing and click on another field (or press the Tab key) if a tag with that name exists than it will be added after the tag field with a little red 'X' on the left and the name of the tag; the name of this tag will be removed from the combo box list (you can't insert the same tag two times), if you click on the red 'x' the tag will be removed from the transaction and it will be re-added to the combo box (so that you can re-add it if you change idea). If the name that you wrote in the tag field is different from the names of existing tags a new dialog will appear (like the payee field), there you can choose to create a new tag or to close it. If you create the tag it will be automatically added to the transaction. </pre>
</blockquote>
<p>On January 23rd, 2013, 9:51 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;">Yes, I can see it works that way in Ledger view, but not in Schedules.
I have created a new tag called 'New Tag', and in the schedule dialog,
I type a 't', and the drop-down appears showing 'New Tag'. I continue to type
'ag' and 'New Tag' still shows. If I now type a 'u', the drop-down disappears.
So far, so good. Now, and this is where it differs, I have 'tagu' in the line-edit,
and if I now click in the memo field, or any edit field in the transaction tab widget,
but not in the schedule name box, 'tagu' disappears. Clicking in the schedule name box
appears to cause the tag to be 'accepted'. In no way can I get the dialog to appear
asking if I want to accept the new tag.
I was expecting it to function as for a new payee or category, which it does in the ledger,
but not in the new schedule.
Allan</pre>
</blockquote>
<p>On January 23rd, 2013, 10:10 p.m. UTC, <b>Alessandro Russo</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;">Hum.. so maybe there is a bug .. I haven't used the schedules too much... In the weekend I'll make some tests.</pre>
</blockquote>
<p>On January 24th, 2013, 8:11 p.m. UTC, <b>Marko Käning</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;">BTW, is it a bug or a feature that the editor doesn't allow to edit overdue payments of schedules?</pre>
</blockquote>
<p>On January 26th, 2013, 1:09 p.m. UTC, <b>Alessandro Russo</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;">Ok, i found the problem. Here there is the fix, can you modify your patch or should I open a different review request?
diff --git a/kmymoney/dialogs/keditscheduledlg.cpp b/kmymoney/dialogs/keditscheduledlg.cpp
index 7ddf3c3..f3146f3 100644
--- a/kmymoney/dialogs/keditscheduledlg.cpp
+++ b/kmymoney/dialogs/keditscheduledlg.cpp
@@ -201,6 +201,7 @@ TransactionEditor* KEditScheduleDlg::startEdit(void)
connect(MyMoneyFile::instance(), SIGNAL(dataChanged()), editor, SLOT(slotReloadEditWidgets()));
// connect(editor, SIGNAL(finishEdit(KMyMoneyRegister::SelectedTransactions)), this, SLOT(slotLeaveEditMode(KMyMoneyRegister::SelectedTransactions)));
connect(editor, SIGNAL(createPayee(QString,QString&)), kmymoney, SLOT(slotPayeeNew(QString,QString&)));
+ connect(editor, SIGNAL(createTag(QString,QString&)), kmymoney, SLOT(slotTagNew(QString,QString&)));
connect(editor, SIGNAL(createCategory(MyMoneyAccount&,MyMoneyAccount)), kmymoney, SLOT(slotCategoryNew(MyMoneyAccount&,MyMoneyAccount)));
connect(editor, SIGNAL(createSecurity(MyMoneyAccount&,MyMoneyAccount)), kmymoney, SLOT(slotInvestmentNew(MyMoneyAccount&,MyMoneyAccount)));
connect(MyMoneyFile::instance(), SIGNAL(dataChanged()), editor, SLOT(slotReloadEditWidgets()));
diff --git a/kmymoney/dialogs/kenterscheduledlg.cpp b/kmymoney/dialogs/kenterscheduledlg.cpp
index 9f4432f..04752f0 100644
--- a/kmymoney/dialogs/kenterscheduledlg.cpp
+++ b/kmymoney/dialogs/kenterscheduledlg.cpp
@@ -218,6 +218,7 @@ TransactionEditor* KEnterScheduleDlg::startEdit(void)
connect(MyMoneyFile::instance(), SIGNAL(dataChanged()), editor, SLOT(slotReloadEditWidgets()));
// connect(editor, SIGNAL(finishEdit(KMyMoneyRegister::SelectedTransactions)), this, SLOT(slotLeaveEditMode(KMyMoneyRegister::SelectedTransactions)));
connect(editor, SIGNAL(createPayee(QString,QString&)), kmymoney, SLOT(slotPayeeNew(QString,QString&)));
+ connect(editor, SIGNAL(createTag(QString,QString&)), kmymoney, SLOT(slotTagNew(QString,QString&)));
connect(editor, SIGNAL(createCategory(MyMoneyAccount&,MyMoneyAccount)), kmymoney, SLOT(slotCategoryNew(MyMoneyAccount&,MyMoneyAccount)));
connect(editor, SIGNAL(createSecurity(MyMoneyAccount&,MyMoneyAccount)), kmymoney, SLOT(slotInvestmentNew(MyMoneyAccount&,MyMoneyAccount)));
connect(MyMoneyFile::instance(), SIGNAL(dataChanged()), editor, SLOT(slotReloadEditWidgets()));
You simply need to add the following line:
connect(editor, SIGNAL(createTag(QString,QString&)), kmymoney, SLOT(slotTagNew(QString,QString&)));
after the similar line with createPayee in the files dialogs/keditscheduledlg.cpp and dialogs/kenterscheduledlg.cpp
(I fixed the same bug in the Enter Scheduled dialog)</pre>
</blockquote>
<p>On January 27th, 2013, 1:14 a.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;">My patches are committed already, but it doesn't make sense really to start a new review,
having got this far, so I've added your changes here.
If I type in a new tag name and then click in one of the edit fields, the dialog now appears
asking whether to add it. It doesn't appear though if I focus on the schedule name, or widgets
not in the transaction area, or click outside the transaction widgets.
If I am creating a new schedule, and type in all the fields, the entries remain visible in the
line edit boxes. This isn't the case with a new tag. If I type in a new tag name and accept it,
it gets cleared. I have to click the red cross then select it from the combo. With the name now
showing, however, if I click the memo field it goes again. It also is possible to get more than
one red cross visible, together with the added tag name alongside.
If I have added a new tag to a schedule, and then decide to delete that tag, and it is the only
tag, I get a message saying I can't delete the last tag as there needs to be one to reassign to.
I understand that, but if I have a bunch of schedules, how do I find which is the one referencing
that tag, to allow me to remove it? Search transaction shows the tag, but selecting it doesn't
enable Find.
Apologies if I seem 'picky'. I didn't set out to undermine your good work, and it is quite a
big change.
</pre>
</blockquote>
<p>On January 27th, 2013, 9:23 a.m. UTC, <b>Alessandro Russo</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;">Hi, no, you don't seem picky, I appreciate your testing.
The first problem (clicking on the schedule name and in other places) is present also for the payee and category/account fields. So we should fix them too.
The second problem is the intended behavior. You can have multiple tags so the behavior is this: you have to write (or select in the combo) a tag (or write a new tag and click 'yes') when you press tab or change focus your tag is 'added' by removing it from the combo box and creating a new label with the same name and a 'X' on its left just after the combo box. You can add multiple tags in this way. To remove a tag from the transaction/schedule you simply have to click on the red X for that tag. I tested it also when adding a new tag (by accepting the creation clicking 'yes' in the modal dialog) and it works too.
The last problem is a TODO present somewhere in the sources, I used the same code for payee that require that every transaction has a payee, for the tags maybe isn't needed. You could simply delete the tag and remove it from all the transaction. Maybe we could show a dialog offering the possibility (not requesting it) to substitute it with another tag, if the user chooses to not do it simply remove the tag from all transactions.
About the Search transaction: I couldn't reproduce it. If I select a tag, it enables 'Find' and you can search all the transactions with that tag/tags. However it can not find any scheduled transaction (this not only for tag search but for every search). So the bug is more general.</pre>
</blockquote>
<p>On January 27th, 2013, 10:07 a.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;">> About the Search transaction: I couldn't reproduce it. If I select a tag, it enables 'Find' and you can search all the transactions with that tag/tags.
> However it can not find any scheduled transaction (this not only for tag search but for every search). So the bug is more general.
I had just created and saved a new schedule having a new tab. I hadn't got to the stage of creating a transaction with that tab. As I had completed what I was testing, I decided to delete the tab, but couldn't remember which schedule I'd added it to. The Schedules list doesn't show tabs, so that's why I tried the transaction search. Of course, I now realise there weren't any transactions with the tab to find, but couldn't get to that stage as Find was disabled.
Whether or not it is a real requirement to show, or find, which schedules use a particular tab, I'll leave to you.
One final point. If I create a new schedule and set up all the fields except tag, then select a tag and immediately click OK, the tag isn't entered.
Prior to looking at tags, I had been going over new schedule creation with a fine tooth comb looking at OK button enabling, and that's how I approached tabs, so nothing personal <BG></pre>
</blockquote>
<p>On January 27th, 2013, 10:24 a.m. UTC, <b>Alessandro Russo</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 'Find' button is not enabled because you have only 1 tag. If you have more than 1 tag you can deselect the other tags and leave selected only the tag you are searching for. There is the possibility to search for all the transaction without tag but not to search the transaction with a tag when this tag is the only one. I think we could change the search dialog as follow: remove the check 'Select transactions without tags' and put at the top of the tags list a row with the name 'None'. So to replicate the old functionality you have to deselect all the tags except for the first one ('None'). To search for a specific tag when you have only 1 you have to deselect 'None' and select the tag.
For the last fix we need to check, when the 'Ok' button is clicked, if there is a tag to be added in the tag field and if so add it.
I don't know if it's needed to add a 'tag' column in the scheduled transaction list, do you have so many of them?</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 don't know if it's needed to add a 'tag' column in the scheduled transaction list, do you have so many of them?
Nope! I have none. My requirements are pretty simple, and I don't, so far, have a need for them. But, I do have quite
a few schedules, so wouldn't fancy plodding through them all.
</pre>
<br />
<p>- Allan</p>
<br />
<p>On January 12th, 2013, 12:07 p.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 Jan. 12, 2013, 12:07 p.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;">The problem as originally reported was that in Schedules view, the OK button became enabled even though no schedule name had been entered.
It was found that the button became enabled as soon as a payee was entered. It was also found that this happened when an amount was entered.
For "payee", line 753 of transactioneditor.cpp has -
"connect(payee,SIGNAL(textChanged(QString)),this,SLOT(slotUpdateButtonState()))", and slotUpdateButtonState() has -
"emit transactionDataSufficient(isComplete(reason)",
and 'This signal is sent out whenever enough data is present to enter the transaction into the ledger.'
Similarly, for "amount", at line 826, the same line appears.
As neither of these fields is a mandatory one, I believe they should not affect the OK button status. So, as shown in the patch, I have temporarily disabled these lines. I have done numerous tests of schedule creation and editing, and manual entry and editing of transactions without any problem.
The same area of code in transactioneditor.cpp has several more of these possibly unneeded lines, although not affecting schedules. For instance, even with these two lines out and with no mandatory fields completed, if a payee is selected and the memo, tag field, next due date or status is edited, the OK button again is enabled wrongly.
I don't really see any valid reason for 'slotUpdateButtonState()' to be in this section. What do the wise men think?
</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;">Numerous tests of schedule creation and editing, and manual entry and editing of transactions without any problem.</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=311481">311481</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/transactioneditor.cpp <span style="color: grey">(8f6f06b)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/107714/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>