Review Request: Merge request for telepathy-kde-contactlist

[Dario Freddi]

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


Follows a review. Protip: smaller diffs next time, I surely missed something :D

Btw, I see this removes all the bit of Nepomuk integration me and George did time ago: I don't know if you guys already discussed about that, but I'd like at least to have George chiming in and have a look.

Beware of QVariant misuse: besides being wrong, it can be dangerous!


account-button.cpp
<http://git.reviewboard.kde.org/r/100838/#comment1850>

    Use KAction instead of QAction here



account-button.cpp
<http://git.reviewboard.kde.org/r/100838/#comment1851>

    For clarity, try using QVariant::value() instead of qVariantFromValue, which was meant as a workaround for old compilers. Your code would become:
    
    a->data().value<Tp::Presence>().status()
    
    Check all the occurrences throughout the code



account-button.cpp
<http://git.reviewboard.kde.org/r/100838/#comment1852>

    It is not very clear to me what you are doing here, but this routine can be extremely slow. Can you please tell me what you are trying to achieve?
    
    I also see there is more than one occurrence of that, so this should indeed optimized if possible.



account-button.cpp
<http://git.reviewboard.kde.org/r/100838/#comment1853>

    Const'ify: const Tp::Presence &presence



account-button.cpp
<http://git.reviewboard.kde.org/r/100838/#comment1854>

    Use const Tp::Presence& here



account-filter-model.h
<http://git.reviewboard.kde.org/r/100838/#comment1855>

    Watch out for correct copyrights, unless you copied this code from somewhere else.



accounts-model-item.cpp
<http://git.reviewboard.kde.org/r/100838/#comment1858>

    Urgh - do not do that. Declare AccountsModelItem as a metatype const_cast to AccountsModelItem and do QVariant::fromValue() from there. Even though I'd try to avoid that const_cast if possible, but it seems rather unfeasible.



accounts-model.h
<http://git.reviewboard.kde.org/r/100838/#comment1856>

    You don't really need a p-impl class here as you are not exporting the model, but if you want to keep it that way, declare it as:
    
    class Private;
    Private * const d;
    
    To keep this consistent with KDE's style.



accounts-model.cpp
<http://git.reviewboard.kde.org/r/100838/#comment1857>

    I don't really see the point of that, can you please explain?



contact-delegate.h
<http://git.reviewboard.kde.org/r/100838/#comment1859>

    Copyright missing



contact-delegate.cpp
<http://git.reviewboard.kde.org/r/100838/#comment1860>

    Copyright missing
    
    P.S.: Hey, this looks like some of my old code btw :D



contact-model-item.h
<http://git.reviewboard.kde.org/r/100838/#comment1861>

    Apparently, those do not need to be virtual



contact-model-item.cpp
<http://git.reviewboard.kde.org/r/100838/#comment1862>

    Same as before



main-widget.cpp
<http://git.reviewboard.kde.org/r/100838/#comment1863>

    KMenu instead of QMenu


- Dario


On March 23, 2011, 9:32 p.m., Martin Klapetek wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/100838/
> -----------------------------------------------------------
> 
> (Updated March 23, 2011, 9:32 p.m.)
> 
> 
> Review request for Telepathy.
> 
> 
> Summary
> -------
> 
> This is the work that begun in my clone repo and has matured enough now to be merged back to 'upstream', where the work should continue now.
> 
> 
> Diffs
> -----
> 
>   CMakeLists.txt 2c62ea10556cdba38d1bca0fe63603df97414336 
>   account-button.h PRE-CREATION 
>   account-button.cpp PRE-CREATION 
>   account-filter-model.h PRE-CREATION 
>   account-filter-model.cpp PRE-CREATION 
>   accounts-model-item.h PRE-CREATION 
>   accounts-model-item.cpp PRE-CREATION 
>   accounts-model.h PRE-CREATION 
>   accounts-model.cpp PRE-CREATION 
>   contact-delegate-overlay.h PRE-CREATION 
>   contact-delegate-overlay.cpp PRE-CREATION 
>   contact-delegate.h PRE-CREATION 
>   contact-delegate.cpp PRE-CREATION 
>   contact-model-item.h PRE-CREATION 
>   contact-model-item.cpp PRE-CREATION 
>   contact-overlays.h PRE-CREATION 
>   contact-overlays.cpp PRE-CREATION 
>   contact-view-hover-button.h PRE-CREATION 
>   contact-view-hover-button.cpp PRE-CREATION 
>   filter-bar.h PRE-CREATION 
>   filter-bar.cpp PRE-CREATION 
>   main-widget.h aed6f7c8336321bc8a6ffb3b9af6b1d493f85790 
>   main-widget.cpp 6146b62eec17b54be63b594200613931673ac5fe 
>   main-widget.ui 5b0d5aaaf3a4e2f49eb030b98b15fcae3a86170c 
>   main.cpp bba0e4175e8853afe603f26ea4707f4974192d0f 
>   ontologies/CMakeLists.txt 3e0bbe8c634908f689dcd360409aeddcc6fc0d23 
>   tree-node.h PRE-CREATION 
>   tree-node.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/100838/diff
> 
> 
> Testing
> -------
> 
> We did a lot of thourough testing on #kde-telepathy, also some people emailed their reports which all has been fixed.
> 
> 
> Thanks,
> 
> Martin
> 
>

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