Review Request: Implement unread messages in the lib and show the status in the app

[David Edmundson]

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


I like what this is achieving. Very much so.

I'm still not entirely convinced by the lib API changes.
I imagine us to be the only app making a tabbed interface to have lots of chat channels, but lots of apps embedding a single chat channel into their shared whiteboard app/game/etc.

signals:
 newMessageReceived(); //maybe some info about the message in case anyone wants to use it.
 (which would be useful anyway)
and that should be all that's actually needed.

I was expecting to see a simple

All logic of whether it is visible or not can be done in the app which knows whether it's the active tab or not, which must be easier.
isActiveWindow is public anyway, so there's no _need_ for the library to be doing it.





app/chatwindow.cpp
<http://git.reviewboard.kde.org/r/100594/#comment1068>

    This isn't right.
    If you have unread messages, then they start/stop typing, you reset the text colour.
    
    If should be 
    
    if (sender) {
     setTabTextColor(tabIndex, sender->titleColor();
    }



app/telepathychatui.h
<http://git.reviewboard.kde.org/r/100594/#comment1069>

    Our tab widget already contains a list of widgets.
    
    Keeping two lists is bad, as they could get out of sync. A better policy is to add a 
    
    textChannel() getter to the (chatTab?) widget, and loop through the list of tabs from the tab widget.
    



lib/chatwidget.h
<http://git.reviewboard.kde.org/r/100594/#comment1071>

    This still seems very application specific, same with showEvent().
    
    Could these be moved to the ChatTab in app.



lib/chatwidget.h
<http://git.reviewboard.kde.org/r/100594/#comment1070>

    Why is this public?


- David


On Feb. 7, 2011, 1:09 a.m., Dominik Schmidt wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/100594/
> -----------------------------------------------------------
> 
> (Updated Feb. 7, 2011, 1:09 a.m.)
> 
> 
> Review request for Telepathy.
> 
> 
> Summary
> -------
> 
> Hopefully you like this one better :P
> 
> Now the chatWidget doesn't know anything about a tab which it could be, only about the unread messages based on isActiveWindow() and isVisible() this behaviour can be overriden (or completely disabled) in a subclass.
> 
> 
> Diffs
> -----
> 
>   app/CMakeLists.txt 738a3b6 
>   app/chattab.h PRE-CREATION 
>   app/chattab.cpp PRE-CREATION 
>   app/chatwindow.h e6e2ffd 
>   app/chatwindow.cpp 5cf2ea0 
>   app/telepathychatui.h 1cfe76c 
>   app/telepathychatui.cpp 4415c82 
>   lib/chatwidget.h c4b3945 
>   lib/chatwidget.cpp a559adc 
> 
> Diff: http://git.reviewboard.kde.org/r/100594/diff
> 
> 
> Testing
> -------
> 
> Works fine for me.
> 
> 
> Thanks,
> 
> Dominik
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/kde-telepathy/attachments/20110207/55a840ab/attachment.htm