Review Request: MessageProcessor and Filters

[David Edmundson]

> On Feb. 2, 2012, 5:06 p.m., Daniele Elmo Domenichelli wrote:
> > lib/message.h, lines 36-37
> > <http://git.reviewboard.kde.org/r/103848/diff/1/?file=48499#file48499line36>
> >
> >     Is there a reason why this class cannot be a QObject and use QObject's dynamic properties?

avoid the overhead of QObject was the logic. Plus this conflicts with my other suggestion of being implicitly shared (as QObject has copy disabled)

Though this mimics the properties of QObject.


> On Feb. 2, 2012, 5:06 p.m., Daniele Elmo Domenichelli wrote:
> > lib/message.h, lines 42-46
> > <http://git.reviewboard.kde.org/r/103848/diff/1/?file=48499#file48499line42>
> >
> >     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?
> >

makes sense. +1 from me.


> On Feb. 2, 2012, 5:06 p.m., Daniele Elmo Domenichelli wrote:
> > lib/message.cpp, lines 30-34
> > <http://git.reviewboard.kde.org/r/103848/diff/1/?file=48500#file48500line30>
> >
> >     Perhaps this could be a "cleanFilter"

I considered that too, now you've said it too..lets do it.

The only twist is we need to (eventually) work out if the original data is raw text or HTML and only do this as appropriate.


> On Feb. 2, 2012, 5:06 p.m., Daniele Elmo Domenichelli wrote:
> > lib/url-filter.cpp, line 2
> > <http://git.reviewboard.kde.org/r/103848/diff/1/?file=48501#file48501line2>
> >
> >     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

Yeah, credit me bitch.


On Feb. 2, 2012, 5:06 p.m., Lasath Fernando wrote:
> > 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

We had a discussion on this at one point. (youtube being an idea being thought about)

I think putting images/text inline, has the potential to be really messy [1], in particular with theme support.
If you have large images it means text may move when loaded or you not notice other bits of text.

It also makes some sense to have our original link in case our plugin is rubbish. Also by storing in a list, QML and HTML /can/ handle the parts differently if they need to. It has a nice separation of "this person said this" "here's some extra suff that we added to make your life easier"

>From your suggestion, what if you had a regular link and a youtube link - you'd have to say not just "I'm handling this" but "I'm handling this bit", "I'm handling this bit", but how do you split a message into arbitrary chunks? We could only do this if we took URLs to be a special case and did it on a per URL basis.

[1] http://static.davidedmundson.co.uk/drdanz.html


- David


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


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/435ff91c/attachment-0001.html>