[Kmymoney-devel] Review Request 114767: Add ability to modify loan institution attribute.

[Thomas Baumgart]

-----------------------------------------------------------
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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kmymoney-devel/attachments/20140101/abffefbb/attachment-0001.html>