> On Oct. 10, 2013, 5:02 a.m., Cristian Oneț wrote: > >
Revised patch to follow. I've also attempted to fix the 'quirks' I referred to above, but I'll keep them separate from this, for clarity. > On Oct. 10, 2013, 5:02 a.m., Cristian Oneț wrote: > > kmymoney/dialogs/transactioneditor.cpp, line 722 > > <http://git.reviewboard.kde.org/r/113143/diff/1/?file=199630#file199630line722> > > > > Please declare variables at the point of their initialization, there is > > no point in declaring the number variable outside of the if statement. And > > use a const reference if you don't intend to change it. > > > > const QString &number = tr.splits().front().number(); On being passed to keepNewNumber(), 'number' was getting changed so couldn't be const. I've reworked that area anyway, and dispensed with QString number. > On Oct. 10, 2013, 5:02 a.m., Cristian Oneț wrote: > > kmymoney/mymoney/mymoneyfile.h, line 1480 > > <http://git.reviewboard.kde.org/r/113143/diff/1/?file=199631#file199631line1480> > > > > Please pass a 'const QString &' as a parameter instead of by-value. > > And why does 'strNumericPart' only forward to the 'numericPart' private > > function, why don't you make 'numericPart' public and remove > > 'strNumericPart'? > Please pass a 'const QString &' as a parameter instead of by-value. - Done. I did originally have 'numericPart' as public, but had an impression that member variables were best not public so changed it. Lack of formal tuition rears its head again. - Done. On Oct. 10, 2013, 5:02 a.m., Allan Anderson wrote: > > The above are just some plain old C++ coding style suggestions. Also should > > 'numericPart' really belong to the mymoneyfile object? If so I would add > > some testcases for it in the mymoneyfiletest suite. No, not really. I placed it there only because there were a couple of of other similar routines there, but they aren't really that similar. As it's used only by TransactionEditor, I've moved it to there. - Allan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113143/#review41476 ----------------------------------------------------------- On Oct. 7, 2013, 8:10 p.m., Allan Anderson wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/113143/ > ----------------------------------------------------------- > > (Updated Oct. 7, 2013, 8:10 p.m.) > > > Review request for KMymoney. > > > Bugs: 319801 > http://bugs.kde.org/show_bug.cgi?id=319801 > > > Repository: kmymoney > > > Description > ------- > > If a user's sequence of check numbers is broken by, say 'ATM' or an invoice > number such as 'No 123-001 ABC', the next check number produced will be '1', > entries containing alpha or punctuation characters not being saved. > The fix corrects this by saving the complete entry, and uses any entered > numeric part to calculate the next number in sequence. If an existing > numeric entry is edited, this entry will be taken into account for the next > number. > > There are some quirks. If the entry which led to the current 'next number' > is deleted, it is not possible to revert to the previous, now forgotten, > 'next number', so the produced 'next number' is likely to leave a 'gap', and > may need editing. Also, there is no check that a new 'next number' does not > already exist. For instance, if there is the erroneous sequence 23,23,24, > the 'next number' will be the expected 25. However, if the user corrects the > error by changing a 23 to 22, the new 'next number' will be 23, which also > already exists. I'm not sure if such issues, which exist also in the current > release, are worthy of fixing for a fairly unimportant area, without becoming > more involved. > > > Diffs > ----- > > kmymoney/dialogs/transactioneditor.h f07dafb > kmymoney/dialogs/transactioneditor.cpp 39049cf > kmymoney/mymoney/mymoneyfile.h af0c6fb > kmymoney/mymoney/mymoneyfile.cpp ff7302c > > Diff: http://git.reviewboard.kde.org/r/113143/diff/ > > > Testing > ------- > > Many manual entries checked, including coping with all values in the unit > tests. The unit test runs OK. > > > Thanks, > > Allan Anderson > >
_______________________________________________ KMyMoney-devel mailing list KMyMoney-devel@kde.org https://mail.kde.org/mailman/listinfo/kmymoney-devel
