Review Request: Merge request for telepathy-kde-contactlist

[David Edmundson]

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


I've not covered everything, but it's a start. 



accountbutton.cpp
<http://git.reviewboard.kde.org/r/100838/#comment1565>

    This whole class is very weird.
    
    Never write 'magic numbers' i.e 3 or 7 just unexplained throughout the code.
    Storing a C array of every possible enum value seems a wrong approach.
    
    I would store the presence status() - the string (which you can use as an ID) as the data() field of the QAction.
    
    onlineAction->setData(Presence::Available().status());
    
    (you might even be able to just store the Tp::Presence object in there if it's a Q_METATYPE (try it and see). )
    
    Then you can use this to create a proper presence from.
    
    Also your presence lists have no profile support - this can be a future thing so I'll explain that later.



accountbutton.cpp
<http://git.reviewboard.kde.org/r/100838/#comment1563>

    This is done for you:
    
    from the docs:
    ----
    Tp::Account::iconName();
    As a last resort, "im-" + protocolName() will be returned.
    



accountfiltermodel.cpp
<http://git.reviewboard.kde.org/r/100838/#comment1564>

    It seems a bit wrong to reimplement text filtering in something that inherits from QSortFilterProxyModel. For someone using this class there are two methods to set a text filter. One that works, one that doesn't. 
    
    Your reasoning not to made sense
    (default behaviour is recursive so will not show any contacts if the account name doesn't match the string)
    
    but maybe we can get round that by calling
    QSortFilterProxyMode::filterAcceptsRow(source_row, source_parent) here, where we know it's a contact. 



contactdelegate.cpp
<http://git.reviewboard.kde.org/r/100838/#comment1567>

    hardcoded values here. At the end of this there should be very few fixed numbers.



contactdelegate.cpp
<http://git.reviewboard.kde.org/r/100838/#comment1566>

    This method is much better than it was.
    
     - this should be KGlobalSettings::smallestReadableFont();



contactdelegate.cpp
<http://git.reviewboard.kde.org/r/100838/#comment1568>

    see other font comment.



contactdelegate.cpp
<http://git.reviewboard.kde.org/r/100838/#comment1569>

    This needs to check metrics
    
    maximum of icon size + padding, font height + padding


- David


On March 11, 2011, 12:27 a.m., Martin Klapetek wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/100838/
> -----------------------------------------------------------
> 
> (Updated March 11, 2011, 12:27 a.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 
>   accountbutton.h PRE-CREATION 
>   accountbutton.cpp PRE-CREATION 
>   accountfiltermodel.h PRE-CREATION 
>   accountfiltermodel.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 
>   avatars/astronaut.jpg PRE-CREATION 
>   avatars/chess.jpg PRE-CREATION 
>   avatars/coffee.jpg PRE-CREATION 
>   avatars/dice.jpg PRE-CREATION 
>   avatars/fish.jpg PRE-CREATION 
>   avatars/lightning.jpg PRE-CREATION 
>   avatars/soccerball.png PRE-CREATION 
>   config.h.cmake PRE-CREATION 
>   contact-model-item.h PRE-CREATION 
>   contact-model-item.cpp PRE-CREATION 
>   contactdelegate.h PRE-CREATION 
>   contactdelegate.cpp PRE-CREATION 
>   contactdelegateoverlay.h PRE-CREATION 
>   contactdelegateoverlay.cpp PRE-CREATION 
>   contactoverlays.h PRE-CREATION 
>   contactoverlays.cpp PRE-CREATION 
>   contactviewhoverbutton.h PRE-CREATION 
>   contactviewhoverbutton.cpp PRE-CREATION 
>   filterbar.h PRE-CREATION 
>   filterbar.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/20110311/7233f657/attachment.htm