Review Request: MessageProcessor and Filters

[Daniele Elmo Domenichelli]

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



lib/image-filter.cpp
<http://git.reviewboard.kde.org/r/103848/#comment8476>

    Perhaps you should resize the image if it is bigger than a certain size, or it will make the chat unreadable



lib/message-processor.cpp
<http://git.reviewboard.kde.org/r/103848/#comment8469>

    These are leaked, you should delete them in distructor



lib/message.h
<http://git.reviewboard.kde.org/r/103848/#comment8471>

    Is there a reason why this class cannot be a QObject and use QObject's dynamic properties?



lib/message.h
<http://git.reviewboard.kde.org/r/103848/#comment8470>

    I'm not really sure if I understand why you are using a list with this enum here...
    So, you have a message, some filters can make some replacement to the main message and some other filters can append some parts.
    Why don't you use a QString for the main message and a QStingList for the appended parts?
    



lib/message.cpp
<http://git.reviewboard.kde.org/r/103848/#comment8472>

    Perhaps this could be a "cleanFilter"



lib/url-filter.cpp
<http://git.reviewboard.kde.org/r/103848/#comment8468>

    Since a large part this file seems to be taken from adium-theme-view.cpp you should probably add the copyright for the original file



lib/url-filter.cpp
<http://git.reviewboard.kde.org/r/103848/#comment8475>

    If you use QList<KUrl> instead of this it will be easier later, for example when you search for the extention you should be sure to remove query items and fragments


There is something that in my opinion is not fully working, but I'm not sure if this is a real use case:
Imagine that you want to embed a youtube video in the chat message when someone sends you the link: first of all you run the url-filter that finds the urls and replaces them and sets the URLs property.
Then it is ok if you want to add the video at the end as a new one, because you can check the URLs property, but if you want to embed it "in place" you have to examine the _replaced_ part (i.e. "<a href=...") and then replace it again.
I'd rather do it in 2 steps... First each filter can check which parts will handle and when they all finished. In this way you can parse for urls and mark them as handled by the url filter, then the youtube-filter can check only the urls and say "no I will handle this", and finally apply the filters and render the message

But perhaps it's just not useful, adding a part at the end might be enough

- Daniele Elmo Domenichelli


On Feb. 2, 2012, 2:42 p.m., Lasath Fernando wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/103848/
> -----------------------------------------------------------
> 
> (Updated Feb. 2, 2012, 2:42 p.m.)
> 
> 
> Review request for Telepathy and David Edmundson.
> 
> 
> Description
> -------
> 
> Create an extensible framework for dealing with incomming messages in text-ui. This patch implements a MessageProcessor and 3 MessageFilters: Url and emoticon parsers, as well as an image embedding plugin (for funsizes :).
> Eventually these will be turned into actual plugins that are loaded at runtime rather than being hardcoded. But for now, they just make David's code cleaner.
> 
> 
> Diffs
> -----
> 
>   lib/CMakeLists.txt 31dadd2 
>   lib/adium-theme-message-info.cpp c5e0dfc 
>   lib/adium-theme-view.cpp a0e7ac1 
>   lib/chat-widget.cpp 75acef0 
>   lib/filters.h PRE-CREATION 
>   lib/image-filter.cpp PRE-CREATION 
>   lib/message-processor.h PRE-CREATION 
>   lib/message-processor.cpp PRE-CREATION 
>   lib/message.h PRE-CREATION 
>   lib/message.cpp PRE-CREATION 
>   lib/url-filter.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/103848/diff/diff
> 
> 
> Testing
> -------
> 
> Spammed meme images at david. He seemed to be happy and replied with lots of smiley faces.
> 
> 
> Thanks,
> 
> Lasath Fernando
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-telepathy/attachments/20120202/3fe627ba/attachment-0001.html>