----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114767/#review46544 -----------------------------------------------------------
My initial comments after a quick walk-through. Looks good otherwise. I am for the fourth button to get access to this option. kmymoney/wizards/newloanwizard/keditloanwizard.cpp <https://git.reviewboard.kde.org/r/114767/#comment33234> Hm, did not know that this works. I always used QDebug (without the 't' as all the other includes). This is a just a remark, not a request to change. kmymoney/wizards/newloanwizard/keditloanwizard.cpp <https://git.reviewboard.kde.org/r/114767/#comment33244> We actually try to avoid bloating the variable names by adding type information to them. Please reconsider. kmymoney/wizards/newloanwizard/keditloanwizard.cpp <https://git.reviewboard.kde.org/r/114767/#comment33236> Think about using "void institutionList(QList<MyMoneyInstitution>& list) const" here. Avoids another copy operation. kmymoney/wizards/newloanwizard/keditloanwizard.cpp <https://git.reviewboard.kde.org/r/114767/#comment33235> how about using foreach (or Q_FOREACH) here? kmymoney/wizards/newloanwizard/keditloanwizard.cpp <https://git.reviewboard.kde.org/r/114767/#comment33237> You could also break out of the loop here since you found what you looked for. kmymoney/wizards/newloanwizard/loanattributeswizardpage.h <https://git.reviewboard.kde.org/r/114767/#comment33242> Please pass parameter as "const QString&" kmymoney/wizards/newloanwizard/loanattributeswizardpage.cpp <https://git.reviewboard.kde.org/r/114767/#comment33243> Shouldn't you use double quotes here? kmymoney/wizards/newloanwizard/loanattributeswizardpage.cpp <https://git.reviewboard.kde.org/r/114767/#comment33238> Trailing spaces kmymoney/wizards/newloanwizard/loanattributeswizardpage.cpp <https://git.reviewboard.kde.org/r/114767/#comment33239> Trailing spaces kmymoney/wizards/newloanwizard/loanattributeswizardpage.cpp <https://git.reviewboard.kde.org/r/114767/#comment33241> Possibility for Q_FOREACH again kmymoney/wizards/newloanwizard/loanattributeswizardpage.cpp <https://git.reviewboard.kde.org/r/114767/#comment33240> Better use if(dlg->exec() && dlg != 0) here. See http://blogs.kde.org/node/3919 for details. - Thomas Baumgart On Dec. 31, 2013, 6:37 p.m., Jeremy Whiting wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/114767/ > ----------------------------------------------------------- > > (Updated Dec. 31, 2013, 6:37 p.m.) > > > Review request for KMymoney. > > > Bugs: 257619 > http://bugs.kde.org/show_bug.cgi?id=257619 > > > Repository: kmymoney > > > Description > ------- > > Add ability to modify loan institution attribute. > > Note this is definitely a work in progress, the QWizardPage title > "Attributes" is not best probably, and the page order doesn't make much sense > either imo. It probably should go after the radio buttons to ask what you > want to change. I'd like to add a fourth option to just modify attributes if > that makes sense to everyone, since it's a bit cumbersome to recalculate > interest, payoff, etc. just to change the institution the loan belongs to. > > > Diffs > ----- > > kmymoney/wizards/newloanwizard/CMakeLists.txt > 939d569c2bba56ccfd3c9d6b28f8b90d414b9332 > kmymoney/wizards/newloanwizard/keditloanwizard.cpp > 5590a359ffc16c92fc3dc24b6484aac1180b1507 > kmymoney/wizards/newloanwizard/knewloanwizard.h > 6b3f049912605f4f9cb0e78ee83b14e2767586b8 > kmymoney/wizards/newloanwizard/knewloanwizarddecl.ui > 831d85ce388b51a904c76cd998f54ca6a099b347 > kmymoney/wizards/newloanwizard/loanattributeswizardpage.h PRE-CREATION > kmymoney/wizards/newloanwizard/loanattributeswizardpage.cpp PRE-CREATION > kmymoney/wizards/newloanwizard/loanattributeswizardpagedecl.ui PRE-CREATION > > Diff: https://git.reviewboard.kde.org/r/114767/diff/ > > > Testing > ------- > > It works here to set and to show the current institution. > > > Thanks, > > Jeremy Whiting > >
_______________________________________________ KMyMoney-devel mailing list KMyMoney-devel@kde.org https://mail.kde.org/mailman/listinfo/kmymoney-devel
