Review Request: Few improvements in LogViewer

[David Edmundson]

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


I'm somewhat confused by EntityModelItem representing either an Account or an Entity (if I'm reading the code right).
But if it works I guess it's fine?

If I was being pedantic I would:
 - either use an abstract tree model item with a virtual data method and subclass for contact and 
 - just use QModelItem.

However... I want to write a generic Grouping class anyway, which would be perfect here, so I can't be bothered to go through this

Also consider if you need to make pointers to entitymodelitems rather than copying them. It might be less confusing.


logviewer/entity-model.cpp
<http://git.reviewboard.kde.org/r/105450/#comment12071>

    coding style:
    
    if (foo) {
      bar
    }
    
    not 
    
    if (foo)
      bar
    
    it does the same thing, but the number of errors caused by confusing {} makes it worth always doing it.
    
    (http://techbase.kde.org/Policies/Kdelibs_Coding_Style)
    
    This exists throughout this code. 



logviewer/entity-model.cpp
<http://git.reviewboard.kde.org/r/105450/#comment12072>

    const &
    
    (edit: just noticed it's my code that was wrong. Naughty d_ed)



logviewer/log-viewer.cpp
<http://git.reviewboard.kde.org/r/105450/#comment12070>

    look up two lines :)



logviewer/log-viewer.cpp
<http://git.reviewboard.kde.org/r/105450/#comment12069>

    We should add this method to the relevant class (the ChatWidget) rather than accessing its internal HTML.. otherwise things have a lot of potential to break.


- David Edmundson


On July 5, 2012, 8:52 p.m., Dan Vratil wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105450/
> -----------------------------------------------------------
> 
> (Updated July 5, 2012, 8:52 p.m.)
> 
> 
> Review request for Telepathy.
> 
> 
> Description
> -------
> 
> - display entities as a per-account tree view
> - try to display contact's real name (or nickname) if possible. This does not work when account is offline (we need active connection), in such case it falls back to TelepathyLoggerQt4::Entity::alias()
> - use QSplitter to make width of the EntityView and MessageView adjustable
> - make the window slightly bigger by default
> 
> 
> Diffs
> -----
> 
>   logviewer/CMakeLists.txt bb22191 
>   logviewer/entity-model-item.h PRE-CREATION 
>   logviewer/entity-model-item.cpp PRE-CREATION 
>   logviewer/entity-model.h cd6649e 
>   logviewer/entity-model.cpp a661d10 
>   logviewer/entity-proxy-model.h PRE-CREATION 
>   logviewer/entity-proxy-model.cpp PRE-CREATION 
>   logviewer/entity-view.h 03c23d1 
>   logviewer/entity-view.cpp 1ef5ef4 
>   logviewer/log-viewer.h 04820fd 
>   logviewer/log-viewer.cpp 2b472fa 
>   logviewer/log-viewer.ui c5e6a09 
> 
> Diff: http://git.reviewboard.kde.org/r/105450/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Dan Vratil
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-telepathy/attachments/20120706/6a688606/attachment-0001.html>