Review Request: Add backlog to chat window

[Daniele Elmo Domenichelli]

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


It looks like good stuff, but I'm not going too much into the code, I'll leave the honour to someone that knows better the text ui code, just some general (mostly useless) comments


cmake/modules/FindGIO.cmake
<http://git.reviewboard.kde.org/r/102776/#comment6215>

    This file is missing the copyright notice



cmake/modules/FindTelepathyLoggerQt4.cmake
<http://git.reviewboard.kde.org/r/102776/#comment6216>

    Missing copyright



lib/CMakeLists.txt
<http://git.reviewboard.kde.org/r/102776/#comment6218>

    Please remove this white line



lib/adium-theme-message-info.cpp
<http://git.reviewboard.kde.org/r/102776/#comment6217>

    Are the comments here useful?



lib/chat-widget.cpp
<http://git.reviewboard.kde.org/r/102776/#comment6220>

    Is there a reason why don't you set "this" as parent?



lib/chat-widget.cpp
<http://git.reviewboard.kde.org/r/102776/#comment6227>

    keep the bracket in the first line



lib/chat-window-style.cpp
<http://git.reviewboard.kde.org/r/102776/#comment6228>

    bracket



lib/logmanager.h
<http://git.reviewboard.kde.org/r/102776/#comment6221>

    Fix this line :)



lib/logmanager.cpp
<http://git.reviewboard.kde.org/r/102776/#comment6222>

    Same here



lib/logmanager.cpp
<http://git.reviewboard.kde.org/r/102776/#comment6231>

    I *think* that the correct style should be to keep the "," in the previous line, check the other files in the patch



lib/logmanager.cpp
<http://git.reviewboard.kde.org/r/102776/#comment6223>

    Perhaps here it's not necessary a warning here, the warning should be at build time...



lib/logmanager.cpp
<http://git.reviewboard.kde.org/r/102776/#comment6229>

    This probably shouldn't be a warning



lib/logmanager.cpp
<http://git.reviewboard.kde.org/r/102776/#comment6230>

    I'm, not sure if we want to exit here



lib/logmanager.cpp
<http://git.reviewboard.kde.org/r/102776/#comment6226>

    bracket


A general thought... Is the logmanager useful for something if TELEPATHY_LOGGER_QT4_FOUND = false?
If not, I'd avoid completely building it from cmake, it will clean a lot the code from all the #ifdef you will have to add just a couple of #ifdef in chat-widget.

- Daniele Elmo Domenichelli


On Oct. 4, 2011, 9:54 p.m., Dominik Schmidt wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/102776/
> -----------------------------------------------------------
> 
> (Updated Oct. 4, 2011, 9:54 p.m.)
> 
> 
> Review request for Telepathy.
> 
> 
> Description
> -------
> 
> I'm sorry this patch got so massive, I spent 4 hours trying to split it up and do a proper review request with no success :-(
> 
> Anyways, you can take a look at my branch with more granular commits:
> kde:clones/telepathy-text-ui/dschmidt/telepathy-text-ui gomental
> 
> 
> Now to the patch:
> It adds backlog functionality (currently a hardcoded amount of messages) to the chat widget via telepathy-logger-qt4. I made it an optional dep cause that lib is not released yet and I want to replace it with logs retrieved from Nepomuk for 0.3.
> 
> All logic interacting with that lib is encapsulated in the LogManager class, so it will be easy to do that change.
> 
> 
> Diffs
> -----
> 
>   cmake/modules/FindGIO.cmake PRE-CREATION 
>   cmake/modules/FindGObject.cmake PRE-CREATION 
>   cmake/modules/FindQtGLib.cmake PRE-CREATION 
>   cmake/modules/FindTelepathyGlib.cmake PRE-CREATION 
>   cmake/modules/FindTelepathyLogger.cmake PRE-CREATION 
>   cmake/modules/FindTelepathyLoggerQt4.cmake PRE-CREATION 
>   config/appearance-config.cpp 895040b 
>   lib/CMakeLists.txt 801749e 
>   lib/adium-theme-message-info.h d90112c 
>   lib/adium-theme-message-info.cpp 545444d 
>   lib/adium-theme-view.cpp 89879c8 
>   lib/chat-widget.h 0fe5d86 
>   lib/chat-widget.cpp 302d097 
>   lib/chat-window-style.h 32fe4fd 
>   lib/chat-window-style.cpp 8242d90 
>   lib/logmanager.h PRE-CREATION 
>   lib/logmanager.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/102776/diff/diff
> 
> 
> Testing
> -------
> 
> Yes, compiled with -DWITH_TelepathyLoggerQt4=ON and -DWITH_TelepathyLoggerQt4=OFF and both builds work.
> 
> 
> Screenshots
> -----------
> 
> chat window with backlog
>   http://git.reviewboard.kde.org/r/102776/s/286/
> 
> 
> Thanks,
> 
> Dominik Schmidt
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-telepathy/attachments/20111004/779d4e5f/attachment-0001.html>