Review Request: Move contact list view to a separate class, easily embeddable

[Dario Freddi]

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


Close enough. There is just one big issue and some annoyances (but hey that's me so you'd better expect that)


contact-list-widget.h
<http://git.reviewboard.kde.org/r/103481/#comment7547>

    Code style: declare after slots



contact-list-widget.h
<http://git.reviewboard.kde.org/r/103481/#comment7548>

    Code style: put right before Q_OBJECT (and include a Q_DISABLE_COPY while you're at it)



contact-list-widget.cpp
<http://git.reviewboard.kde.org/r/103481/#comment7549>

    NNNOOOOOOOO. When you use Q_DECLARE_PRIVATE, *never* use d_ptr directly. Instead, there is a handy macro for you: Q_D. Use it like that:
    
    Q_D(ContactListWidget);
    
    at the beginning of each function where you need to access the d pointer. In fact, this macro generates a "d" variable which is your dpointer with the correct type. This allows you to subclass private classes. So, once you use Q_D, you can switch all of your d_ptr-> to d->



contact-list-widget.cpp
<http://git.reviewboard.kde.org/r/103481/#comment7550>

    Hmm, wouldn't KMessageBox be a better fit here?



contact-list-widget.cpp
<http://git.reviewboard.kde.org/r/103481/#comment7551>

    constify and watch for the style: Q_FOREACH (const QString &filename, filenames)



contact-list-widget_p.h
<http://git.reviewboard.kde.org/r/103481/#comment7552>

    A decent code style here wouldn't hurt. align all of the first letters, your choice whether to put the comma under the colon for each variable or not.


- Dario Freddi


On Dec. 20, 2011, 7:54 p.m., Martin Klapetek wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/103481/
> -----------------------------------------------------------
> 
> (Updated Dec. 20, 2011, 7:54 p.m.)
> 
> 
> Review request for Telepathy.
> 
> 
> Description
> -------
> 
> This second part moves the actual view out of MainWidget to a separate ContactListWidget class. The separate private class is mainly for the context menu, which needs access to the private variables. The widget should be fully self contained with public slots to change its behaviour/look (switch between groups/accounts, show offline users etc). All error messages are sent out as signals and picked by MainWidget, which displays the actual notifications. All Tp stuff was also moved there.
> 
> 
> This addresses bug 279107.
>     http://bugs.kde.org/show_bug.cgi?id=279107
> 
> 
> Diffs
> -----
> 
>   CMakeLists.txt e6d7cea 
>   contact-list-widget.h PRE-CREATION 
>   contact-list-widget.cpp PRE-CREATION 
>   contact-list-widget_p.h PRE-CREATION 
>   context-menu.h 178cdf0 
>   context-menu.cpp fff8c7a 
>   main-widget.h f01b377 
>   main-widget.cpp 1d3e10c 
>   main-widget.ui d71a276 
> 
> Diff: http://git.reviewboard.kde.org/r/103481/diff/diff
> 
> 
> Testing
> -------
> 
> Tested all actions. Started chat etc. All works.
> 
> 
> Thanks,
> 
> Martin Klapetek
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-telepathy/attachments/20111220/63b1d40d/attachment-0001.html>