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

[George Goldberg]

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


Looks nice now. I've only got minor points mentioned below. Assuming it all works and is tested and stuff, once those are addressed I think we are good to go. Would be awesome if you could deal with the issues I've raised and then update the diff and let someone else have a quick glance over it before merging.


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

    #include ordering.
    
    Start with "" ones
    
    Then do <> ones after them.



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

    const Tp::ProtocolParameter &parameter



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

    Use KDElibs style #include ordering. These two should be:
    
    #include "protocol-parameter-value.h"
    
    #include <QtCore/QAbstractListModel>
    
    laid out like that.
    



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

    const Tp::ProtocolParameter &parameter ?



src/KCMTelepathyAccounts/protocol-parameter-value.h
<http://git.reviewboard.kde.org/r/100293/#comment559>

    License header: If you don't have any particular ideas for it, just copy it from any of the other files, but replace the "Copyright Collabora" lines with "Copyright You" and you can remove the @author lines altogether since you are personally identified in the copyright statement.



src/KCMTelepathyAccounts/protocol-parameter-value.h
<http://git.reviewboard.kde.org/r/100293/#comment561>

    this should be a <> include.
    
    Also, it's nice to put them in vaguely alphabetical order, and in blocks depending on what lib they come from, although really that's just my personal preference (see other files for how I'd do it).



src/KCMTelepathyAccounts/protocol-parameter-value.h
<http://git.reviewboard.kde.org/r/100293/#comment562>

    SHIT we use KDE_EXPORT that's BAD.
    
    Probably my fault though, if it's done in other headers in this repo :(
    
    Something to fix in another patch.



src/KCMTelepathyAccounts/protocol-parameter-value.cpp
<http://git.reviewboard.kde.org/r/100293/#comment560>

    License header, as previous file.



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

    Is using [] safe in the case that there is no item in that map with key "account"? This may be the case, so need to be sure it's safe.


- George


On 2011-01-05 21:38:20, David Edmundson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/100293/
> -----------------------------------------------------------
> 
> (Updated 2011-01-05 21:38:20)
> 
> 
> 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/CMakeLists.txt a74ae27c89e1ad9e0ad6948bf959f4c93bd1ab7c 
>   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/KCMTelepathyAccounts/protocol-parameter-value.h PRE-CREATION 
>   src/KCMTelepathyAccounts/protocol-parameter-value.cpp PRE-CREATION 
>   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/20110105/ea4462fc/attachment-0001.htm