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

[Dominik Schmidt]

> On Feb. 7, 2011, 11:50 a.m., David Edmundson wrote:
> > 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.
> > 
> > 
> >

Okay, I get your point.

My logic was, that I didn't want to have to reimplement this in every application, see my later comment for further explanation.


At least this functionality doesn't harm anything, if you don't want to use it, don't use it: For that you don't need to build a new subclass.
To make it more clear and to don't affect the titleColor, we could provide a setter setMessagesCounted(bool) or something.


> On Feb. 7, 2011, 11:50 a.m., David Edmundson wrote:
> > app/chatwindow.cpp, line 142
> > <http://git.reviewboard.kde.org/r/100594/diff/1/?file=8608#file8608line142>
> >
> >     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();
> >     }

Okay, i'll check.


> On Feb. 7, 2011, 11:50 a.m., David Edmundson wrote:
> > app/telepathychatui.h, line 48
> > <http://git.reviewboard.kde.org/r/100594/diff/1/?file=8609#file8609line48>
> >
> >     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.
> >

What if we have two windows?

This list keeps all chatwidgets from all windows.


But you're right, they should not get out of sync, I should remove them, when a tab is closed.

I also could loop through all windows, which leads us to the same problem, so it doesn't matter probably.


> On Feb. 7, 2011, 11:50 a.m., David Edmundson wrote:
> > lib/chatwidget.h, line 68
> > <http://git.reviewboard.kde.org/r/100594/diff/1/?file=8611#file8611line68>
> >
> >     Why is this public?

Because I was playing around and forgot to move it to a sensible place ;-)


> On Feb. 7, 2011, 11:50 a.m., David Edmundson wrote:
> > lib/chatwidget.h, line 66
> > <http://git.reviewboard.kde.org/r/100594/diff/1/?file=8611#file8611line66>
> >
> >     This still seems very application specific, same with showEvent().
> >     
> >     Could these be moved to the ChatTab in app.

I thought this was very generic.

If the widget is visible (always true if there's a single widget somewhere in the implementing app) and the window is active, it's "onTop". This seems to be very much applicable to every use case I can think of.


Same with showEvent, if the widget was hidden and is shown now (this also affects window minimizing and reactivating), unread messages are reset.

This seems to me so basic that every application that would want to implement some concept of unread messages would have to basically reimplement exactly this.

We can talk this over on IRC maybe.


- Dominik


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


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/5a10bc32/attachment.htm