Review Request: Compile TP Account KCM against Tp-Qt4 0.5

[Dario Freddi]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/100293/#review712
-----------------------------------------------------------


Good start, here's some catches. Look out for adding constness when changing from pointers to references!


src/KCMTelepathyAccounts/account-edit-widget.cpp
<http://git.reviewboard.kde.org/r/100293/#comment524>

    const Tp::ProtocolParameter &parameter is preferred in this case.



src/KCMTelepathyAccounts/account-edit-widget.cpp
<http://git.reviewboard.kde.org/r/100293/#comment525>

    Code style: if () { (always use brackets)



src/KCMTelepathyAccounts/account-edit-widget.cpp
<http://git.reviewboard.kde.org/r/100293/#comment526>

    This part of the code is rather unclear to me: why are you merging the Maps with an iterator instead of an unite as you have done before? Precisely, what is broken here?



src/KCMTelepathyAccounts/parameter-edit-model.h
<http://git.reviewboard.kde.org/r/100293/#comment527>

    constify Tp::ProtocolParameter if possible



src/KCMTelepathyAccounts/parameter-edit-model.cpp
<http://git.reviewboard.kde.org/r/100293/#comment528>

    It is possible indeed, so const Tp::ProtocolParameter& please :)



src/KCMTelepathyAccounts/parameter-edit-widget.cpp
<http://git.reviewboard.kde.org/r/100293/#comment529>

    Constify here as well



src/KCMTelepathyAccounts/parameter-item.h
<http://git.reviewboard.kde.org/r/100293/#comment530>

    Constify



src/KCMTelepathyAccounts/parameter-item.h
<http://git.reviewboard.kde.org/r/100293/#comment531>

    Constify



src/KCMTelepathyAccounts/protocol-item.cpp
<http://git.reviewboard.kde.org/r/100293/#comment532>

    Constify



src/add-account-assistant.cpp
<http://git.reviewboard.kde.org/r/100293/#comment520>

    Code style: for () {



src/add-account-assistant.cpp
<http://git.reviewboard.kde.org/r/100293/#comment522>

    This code is potentially dangerous, as you are removing from an "alive" iterator some values. You should instead try with a while cycle, something like:
    
    while (i != parameterValues.end()) {
    if (i.value().isNull) {
    i = parameterValues.erase(i);
    } else {
    ++i;
    }
    }
    
    This code is actually safer and fail-proof



src/edit-account-dialog.cpp
<http://git.reviewboard.kde.org/r/100293/#comment523>

    See previous comment, it applies here as well.


- Dario


On 2011-01-03 20:17:01, David Edmundson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/100293/
> -----------------------------------------------------------
> 
> (Updated 2011-01-03 20:17:01)
> 
> 
> Review request for Telepathy.
> 
> 
> Summary
> -------
> 
>     Compile against Telepathy-Qt4 0.5
>     Changed account abstract-account-parameters-widget.h to return
>     QVariantMap of parameterValues() instead of trying to
>     QMap<ParameterItem,QString> which didn't compile.
>     
>     There didn't appear to be a reason for doing this as it was just copied
>     to a QVariantMap directly after anyway.
>     
>     Note that the code now doesn't remove any parameters which have the same
>     value as the default. This didn't seem to be a step required by
>     Telepathy.
>     
>     Also updated parameter-edit-model to always return the value in the type
>     requested by the ProtocolParameter.
> 
> 
> Diffs
> -----
> 
>   src/KCMTelepathyAccounts/abstract-account-parameters-widget.h 6357b4eaff3b5579b44d9040ffe33314bbf09d80 
>   src/KCMTelepathyAccounts/account-edit-widget.h 4c78338603dc0b88230b52190ca9d373dff2628d 
>   src/KCMTelepathyAccounts/account-edit-widget.cpp b554db71a17a4c9664d04f781561ce62cbc17562 
>   src/KCMTelepathyAccounts/parameter-edit-model.h 0c762ea5a30253cd1c4b772f3859b681f4fa8dd4 
>   src/KCMTelepathyAccounts/parameter-edit-model.cpp 0fe52a10798b03dff5491d78c1b16ad72e2f0533 
>   src/KCMTelepathyAccounts/parameter-edit-widget.h 4d3b5a62735bd519ce01f39037dad151e6b34cb3 
>   src/KCMTelepathyAccounts/parameter-edit-widget.cpp 187c39fbf990b610b8985d1869d48bc2be7c9c6d 
>   src/KCMTelepathyAccounts/parameter-item.h 14b11160204a5b76ee3b0fed65861d0ceec87a7f 
>   src/KCMTelepathyAccounts/parameter-item.cpp 318f175ec60b0dc31edadb72eda769b791ff9827 
>   src/KCMTelepathyAccounts/protocol-item.cpp 03ac0bceff0f32673a706329f698a66323842c69 
>   src/account-item.cpp 50afaa54c96ad51b6775f954cc180d1e12bbe96b 
>   src/add-account-assistant.cpp 3933763957b1448aacf9e6c7efbc6b296a5e3664 
>   src/edit-account-dialog.cpp 73462f2e2a93e22e6d5d67297c575f4e10d44c5c 
> 
> Diff: http://git.reviewboard.kde.org/r/100293/diff
> 
> 
> Testing
> -------
> 
> Checked it now compiled
> Added an account, edited an account and removed an account. Account adding and editing done using both the UI plugins and the generic configuration.
> 
> 
> Thanks,
> 
> David
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/kde-telepathy/attachments/20110103/e1806fd3/attachment-0001.htm