Review Request: Porting MessageFilters

Lasath Fernando kde at lasath.org
Thu May 3 11:41:16 UTC 2012 


> On May 1, 2012, 2:39 p.m., David Edmundson wrote:
> > I'm very impressed! Especially with documentation and testing, this isn't the Lasath I know! 
> > 
> > Few comments, though generally I'm happy with it.
> > However, do _NOT_ merge this into master until 0.4 is branched. I will talk to mck182 about this.
> > 
> > (so a suspended ship-it! from me)

My lecturer has got it into our heads (via his favourite method of teaching - pain and suffering) that when crunched for time, you can always scale back features, but crappy code is still crappy code. So you're better of having less features well written and tested than having a whole bunch of haxx that's hard to maintain and/or are broken.

Thus, the tests and documentation.

And yeah, this wasn't meant to be merged to master - it's already on my scratch repo but quickgit.kde.org hadn't realized it existed at the time.
http://quickgit.kde.org/index.php?p=clones%2Fktp-text-ui%2Ffernando%2Fgsoc.git&a=shortlog&h=refs/heads/first_attempt


> On May 1, 2012, 2:39 p.m., David Edmundson wrote:
> > filters/emoticons/emoticon-filter.h, line 35
> > <http://git.reviewboard.kde.org/r/104803/diff/1/?file=59836#file59836line35>
> >
> >     You don't /need/ to use d-pointers in the filters.
> >     
> >     If you prefer to use them, go ahead - but you don't need to maintain binary compatibility as it's not a library people will link against.

It's actually a habit now - so I'm probably going to keep them there or else it'll feel weird to me :S


> On May 1, 2012, 2:39 p.m., David Edmundson wrote:
> > lib/ktptxtui_message_filter.desktop, line 1
> > <http://git.reviewboard.kde.org/r/104803/diff/1/?file=59850#file59850line1>
> >
> >     What is this file for?
> >

I assumed I needed this to let KDE know that 'KTpTextUi/MessageFilter' is a valid ServiceType.


> On May 1, 2012, 2:39 p.m., David Edmundson wrote:
> > lib/message-processor.cpp, line 95
> > <http://git.reviewboard.kde.org/r/104803/diff/1/?file=59852#file59852line95>
> >
> >     This is just in any random order.
> >     
> >     Needs fixing long term. If you write code you know needs fixing add a "//FIXME" or "//TODO" so it's not forgotten.

Oh yeah, I was actually going to ask you during our weekly meeting if I can have my own component in bugzilla. That way I could use it to keep track of everything that needs to be done. (And eventually for bugs after it's been released)


> On May 1, 2012, 2:39 p.m., David Edmundson wrote:
> > lib/message.h, line 37
> > <http://git.reviewboard.kde.org/r/104803/diff/1/?file=59853#file59853line37>
> >
> >     Don't say it's thread safe unless it actually is.
> >     
> >     Which I'm not sure this is.

It //will// be :P

As soon as I get around to building the async version (and therefore accept that there is such a thing as concurrency in this world).


> On May 1, 2012, 2:39 p.m., David Edmundson wrote:
> > tests/message-processor-basic-tests.h, line 31
> > <http://git.reviewboard.kde.org/r/104803/diff/1/?file=59856#file59856line31>
> >
> >     try and call member variables m_something.
> >     
> >     
> >     it will be make it clearer.
> >     
> >     also don't write "this->" everywhere.

Okay, it's now called actualProcessor, but it's in a d->pointer.
I have to have one or the other now, or I feel my head will explode. 

PS: I blame Java


- Lasath


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


On May 1, 2012, 8:43 a.m., Lasath Fernando wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/104803/
> -----------------------------------------------------------
> 
> (Updated May 1, 2012, 8:43 a.m.)
> 
> 
> Review request for Telepathy and David Edmundson.
> 
> 
> Description
> -------
> 
> I'm starting do make some headway on my project (despite being swamped by Uni work at the moment), so I thought I may as well review what I've done so far.
> 
> Firstly, I gave MessageProcessor a KPluginLoader. I then cleaned it up, removed unneeded #includes, gave AbstractMessageFilter a camel case header etc.
> 
> Before I started porting the three existing filters to be KPlugins, I decided I should write unit tests for them. I'm not really sure on what the conventions are for tests because, well I haven't seen any on this project. So I made a few QTests and stuck them in a directory called tests.
> 
> I've ported EscapeFilter and EmoticonFilter. I just wrote tests for UrlFilter, but haven't got to port it yet. 
> 
> I also started documenting my work, in the hope that it'll make things easier to maintain. Currently, the Message class is more or less documented. 
> If at all possible, I'd like someone who isn't familiar with how these work internally to read it and tell me if that documentation is clear.
> 
> 
> Diffs
> -----
> 
>   CMakeLists.txt e5dc102 
>   filters/CMakeLists.txt PRE-CREATION 
>   filters/emoticons/CMakeLists.txt PRE-CREATION 
>   filters/emoticons/emoticon-filter.h PRE-CREATION 
>   filters/emoticons/emoticon-filter.cpp PRE-CREATION 
>   filters/emoticons/ktptextui_message_filter_emoticons.desktop PRE-CREATION 
>   filters/escape/CMakeLists.txt PRE-CREATION 
>   filters/escape/escape-filter.h PRE-CREATION 
>   filters/escape/escape-filter.cpp PRE-CREATION 
>   filters/escape/ktptextui_message_filter_escape.desktop PRE-CREATION 
>   lib/CMakeLists.txt e94a432 
>   lib/KTp/AbstractMessageFilter PRE-CREATION 
>   lib/abstract-message-filter.h PRE-CREATION 
>   lib/abstract-message-filter.cpp PRE-CREATION 
>   lib/emoticon-filter.cpp 0e37aab 
>   lib/escape-filter.cpp c366410 
>   lib/filters.h 6059ea2 
>   lib/ktptxtui_message_filter.desktop PRE-CREATION 
>   lib/message-processor.h d6228b5 
>   lib/message-processor.cpp a9b409e 
>   lib/message.h c9d4340 
>   lib/message.cpp ae947d2 
>   tests/CMakeLists.txt PRE-CREATION 
>   tests/message-processor-basic-tests.h PRE-CREATION 
>   tests/message-processor-basic-tests.cpp PRE-CREATION 
>   tests/sync-processor.h PRE-CREATION 
>   tests/sync-processor.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/104803/diff/
> 
> 
> Testing
> -------
> 
> THERE ARE UNIT TESTS!! :D
> 
> 
> Thanks,
> 
> Lasath Fernando
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-telepathy/attachments/20120503/77aee6bb/attachment-0001.html>

More information about the KDE-Telepathy mailing list