Review Request 112005: KTp Logger

Wed Aug 14 02:28:31 UTC 2013

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

All in all, damn awesome!

Comments below.

KTp/Logger/log-entity.h
<http://git.reviewboard.kde.org/r/112005/#comment27949>

    I'm a bit confused by this class as we appear to be using it for two things:

    1) Get logs for this LogEntity

    2) This message was from this entity

    In the first case "alias" is pointless, in the second case "entityType" is pointless as it will always be a contact. For something with only 3 properties, having 2 useless half the time seems wrong?

    I would consider only using this for saying who a message is from and when requesting use targetHandle, Tp::HandleType targetHandleType

KTp/Logger/log-entity.h
<http://git.reviewboard.kde.org/r/112005/#comment27950>

    root -> room? *3

KTp/Logger/log-manager.cpp
<http://git.reviewboard.kde.org/r/112005/#comment27945>

    KGLOBAL_STATIC is pretty awesome, it can take args, and I think it even deletes itself on app closedown.

    You don't have to change it here, but it's worth learning for future stuff.

KTp/Logger/log-message.h
<http://git.reviewboard.kde.org/r/112005/#comment27948>

    We've broken ABI already anyway..so feel free to hack whatever.
    Also if you ever see me make a private constructor again, shout at me.

KTp/Logger/pending-logger-operation.cpp
<http://git.reviewboard.kde.org/r/112005/#comment27951>

    Do you need the timer here?

    deleteLater() will only delete the object when it hits the event loop. This ensures the same thing.

    Unless the motive is to make sure we're always in the same thread? in which case you probably want to add a Qt::QueuedConnection in there - but I don't think it's needed.

KTp/Logger/plugins/winlogger/logger/db.cpp
<http://git.reviewboard.kde.org/r/112005/#comment27952>

    I'm confused on how this will work.

    Say I'm on ktp.conference.jabber.org and Martin sends me a message.

    Is that contactId going to be ktp.conference.jabber.org or is it going to be Martin. 

    Either way it's wrong. (I think)

KTp/Logger/plugins/winlogger/logger/observer.cpp
<http://git.reviewboard.kde.org/r/112005/#comment27953>

    handleChannels can be called multiple times for the same channel even if we're already handling it.

    If the same is true for observeChannels we can accidentally log the same things twice.

KTp/message-processor.h
<http://git.reviewboard.kde.org/r/112005/#comment27944>

    I'm confused, what will replace this?

KTp/message.h
<http://git.reviewboard.kde.org/r/112005/#comment27943>

    I think we need

    messageType
     - you can't assume "Normal". we'll break all "/me" message

    isHistory
     - Everything in your use case is obviously history, but from an API POV there's no reason why a message created directly should be.

    You can use default args for both of those to match the common case.

You've broken review board, all these comments appear in a random order. 

- David Edmundson

On Aug. 13, 2013, 7:49 p.m., Dan Vrátil wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112005/
> -----------------------------------------------------------
> 
> (Updated Aug. 13, 2013, 7:49 p.m.)
> 
> 
> Review request for Telepathy.
> 
> 
> Description
> -------
> 
> Hi guys,
> 
> on Akademy we discussed having our own logger that would provide abstraction 
> over TpLoggerQt (and possibly other backends).
> 
> I've just pushed tp-logger branch in ktp-common-internals, which introduces 
> KTp Logger - a library with API similar to what TpLoggerQt has and a plugin-
> based design. There's already tplogger plugin which provides integration with 
> Telepathy Logger via TpLoggerQt. TpLoggerQt is still just an optional 
> dependency - without it the plugin will be disabled at build time.
> 
> For Windows, there also ktp-winlogger utility, which is an extremely super 
> simple logger which stores logs into SQLite database and of course there's a 
> plugin that provides integration with KTp Logger. It's automatically enabled 
> on Windows, on Linux pass -DENABLE_WINLOGGER=TRUE to CMake. It works on Linux 
> too :-)
> 
> In ktp-text-ui there is a ktplogger branch with logviewer completely ported to 
> KTp Logger, so you can even test that it works :-) chat-ui and other 
> components still have to be ported.
> 
> 
> Diffs
> -----
> 
>   KTp/Logger/log-manager-private.h PRE-CREATION 
>   KTp/Logger/log-entity.cpp PRE-CREATION 
>   KTp/Logger/log-entity.h PRE-CREATION 
>   KTp/Logger/ktp_logger_plugin.desktop PRE-CREATION 
>   KTp/Logger/abstract-logger-plugin.cpp PRE-CREATION 
>   KTp/Logger/abstract-logger-plugin.h PRE-CREATION 
>   KTp/Logger/CMakeLists.txt PRE-CREATION 
>   KTp/CMakeLists.txt b514418db0da4ba41c13cbaea3591cba6b21ad5d 
>   KTp/Logger/log-manager.h PRE-CREATION 
>   KTp/Logger/log-manager.cpp PRE-CREATION 
>   KTp/Logger/log-message.h PRE-CREATION 
>   KTp/Logger/log-message.cpp PRE-CREATION 
>   KTp/Logger/log-search-hit.h PRE-CREATION 
>   KTp/Logger/log-search-hit.cpp PRE-CREATION 
>   KTp/Logger/pending-logger-dates-impl.h PRE-CREATION 
>   KTp/Logger/pending-logger-dates-impl.cpp PRE-CREATION 
>   KTp/Logger/pending-logger-dates.h PRE-CREATION 
>   KTp/Logger/pending-logger-dates.cpp PRE-CREATION 
>   KTp/Logger/pending-logger-entities-impl.h PRE-CREATION 
>   KTp/Logger/pending-logger-entities-impl.cpp PRE-CREATION 
>   KTp/Logger/pending-logger-entities.h PRE-CREATION 
>   KTp/Logger/pending-logger-entities.cpp PRE-CREATION 
>   KTp/Logger/pending-logger-logs-impl.h PRE-CREATION 
>   KTp/Logger/pending-logger-logs-impl.cpp PRE-CREATION 
>   KTp/Logger/pending-logger-logs.h PRE-CREATION 
>   KTp/Logger/pending-logger-logs.cpp PRE-CREATION 
>   KTp/Logger/pending-logger-operation.h PRE-CREATION 
>   KTp/Logger/pending-logger-operation.cpp PRE-CREATION 
>   KTp/Logger/pending-logger-search-impl.h PRE-CREATION 
>   KTp/Logger/pending-logger-search-impl.cpp PRE-CREATION 
>   KTp/Logger/pending-logger-search.h PRE-CREATION 
>   KTp/Logger/pending-logger-search.cpp PRE-CREATION 
>   KTp/Logger/plugins/CMakeLists.txt PRE-CREATION 
>   KTp/Logger/plugins/tplogger/CMakeLists.txt PRE-CREATION 
>   KTp/Logger/plugins/tplogger/ktploggerplugin_tplogger.desktop.cmake PRE-CREATION 
>   KTp/Logger/plugins/tplogger/pending-tp-logger-dates.h PRE-CREATION 
>   KTp/Logger/plugins/tplogger/pending-tp-logger-dates.cpp PRE-CREATION 
>   KTp/Logger/plugins/tplogger/pending-tp-logger-entities.h PRE-CREATION 
>   KTp/Logger/plugins/tplogger/pending-tp-logger-entities.cpp PRE-CREATION 
>   KTp/Logger/plugins/tplogger/pending-tp-logger-logs.h PRE-CREATION 
>   KTp/Logger/plugins/tplogger/pending-tp-logger-logs.cpp PRE-CREATION 
>   KTp/Logger/plugins/tplogger/pending-tp-logger-search.h PRE-CREATION 
>   KTp/Logger/plugins/tplogger/pending-tp-logger-search.cpp PRE-CREATION 
>   KTp/Logger/plugins/tplogger/tp-logger-plugin.h PRE-CREATION 
>   KTp/Logger/plugins/tplogger/tp-logger-plugin.cpp PRE-CREATION 
>   KTp/Logger/plugins/tplogger/utils.h PRE-CREATION 
>   KTp/Logger/plugins/tplogger/utils.cpp PRE-CREATION 
>   KTp/Logger/plugins/winlogger/CMakeLists.txt PRE-CREATION 
>   KTp/Logger/plugins/winlogger/logger/CMakeLists.txt PRE-CREATION 
>   KTp/Logger/plugins/winlogger/logger/KTp.WinLogger.client PRE-CREATION 
>   KTp/Logger/plugins/winlogger/logger/db.h PRE-CREATION 
>   KTp/Logger/plugins/winlogger/logger/db.cpp PRE-CREATION 
>   KTp/Logger/plugins/winlogger/logger/logger.h PRE-CREATION 
>   KTp/Logger/plugins/winlogger/logger/logger.cpp PRE-CREATION 
>   KTp/Logger/plugins/winlogger/logger/main.cpp PRE-CREATION 
>   KTp/Logger/plugins/winlogger/logger/observer.h PRE-CREATION 
>   KTp/Logger/plugins/winlogger/logger/observer.cpp PRE-CREATION 
>   KTp/Logger/plugins/winlogger/logger/org.freedesktop.Telepathy.Client.KTp.WinLogger.service.in PRE-CREATION 
>   KTp/Logger/plugins/winlogger/logger/org.freedesktop.Telepathy.KTp.WinLogger.service.in PRE-CREATION 
>   KTp/Logger/plugins/winlogger/plugin/CMakeLists.txt PRE-CREATION 
>   KTp/Logger/plugins/winlogger/plugin/ktploggerplugin_winlogger.desktop.cmake PRE-CREATION 
>   KTp/Logger/plugins/winlogger/plugin/pending-win-logger-dates.h PRE-CREATION 
>   KTp/Logger/plugins/winlogger/plugin/pending-win-logger-dates.cpp PRE-CREATION 
>   KTp/Logger/plugins/winlogger/plugin/pending-win-logger-entities.h PRE-CREATION 
>   KTp/Logger/plugins/winlogger/plugin/pending-win-logger-entities.cpp PRE-CREATION 
>   KTp/Logger/plugins/winlogger/plugin/pending-win-logger-logs.h PRE-CREATION 
>   KTp/Logger/plugins/winlogger/plugin/pending-win-logger-logs.cpp PRE-CREATION 
>   KTp/Logger/plugins/winlogger/plugin/pending-win-logger-search.h PRE-CREATION 
>   KTp/Logger/plugins/winlogger/plugin/pending-win-logger-search.cpp PRE-CREATION 
>   KTp/Logger/plugins/winlogger/plugin/win-logger-plugin.h PRE-CREATION 
>   KTp/Logger/plugins/winlogger/plugin/win-logger-plugin.cpp PRE-CREATION 
>   KTp/message-processor.h 8ce92b2a91c2839167ea19885166cb4e878333f0 
>   KTp/message-processor.cpp e045d7bf335bdf9b8ec9b975e147f13ef8575acb 
>   KTp/message.h 29536168c5e140f02c67c16fd705c66e9753fe0e 
>   KTp/message.cpp 36ea23479034b48e8e1f580625de855af64faf34 
> 
> Diff: http://git.reviewboard.kde.org/r/112005/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Dan Vrátil
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-telepathy/attachments/20130814/00970c15/attachment-0001.html>