Bug 54033

Created attachment 66089 [details] [review]
Proposed patch

Slightly better patch without wrong files and leftover changes...sorry, it's quite late here :)

I'll do proper review next week, meanwhile take note that you'll have to provide unit test for your new feature. About that feature, is there any use of it that you would like ? Is there any proposal for integration into Empathy ?

Comment on attachment 66089 [details] [review]
Proposed patch

Review of attachment 66089 [details] [review]:
-----------------------------------------------------------------

Why do you convert the string arrays into GSList ? Also, you could enable regex, this would make your filter more flexible.

::: telepathy-logger/conf.c
@@ +261,5 @@
> +_tpl_conf_set_accounts_ignorelist (TplConf *self,
> +    GSList *newlist)
> +{
> +    GSList *iter;
> +    gint ii, len;

Any reason for ii, why not just i ?

(In reply to comment #2)
> I'll do proper review next week, meanwhile take note that you'll have to
> provide unit test for your new feature. 

I'll provide the unit tests during the week (must learn how to do it first :)

> About that feature, is there any use of
> it that you would like ? Is there any proposal for integration into Empathy ?

We would like to have it in KDE Telepathy

(In reply to comment #3)
> Why do you convert the string arrays into GSList ? 

No particular reason, I just think it's more programmer-friendly. But now I realize the TplConf API is not public so it makes no sense really....Which rises a question in my mind - how will users change the values - is direct changing of the GSettings property OK?

> Also, you could enable regex, this would make your filter more flexible.

OK.

> 
> ::: telepathy-logger/conf.c
> @@ +261,5 @@
> > +_tpl_conf_set_accounts_ignorelist (TplConf *self,
> > +    GSList *newlist)
> > +{
> > +    GSList *iter;
> > +    gint ii, len;
> 
> Any reason for ii, why not just i ?

No, just a habit :)

(In reply to comment #4)
> No particular reason, I just think it's more programmer-friendly. But now I
> realize the TplConf API is not public so it makes no sense really....Which
> rises a question in my mind - how will users change the values - is direct
> changing of the GSettings property OK?

I think so, the enable/disable feature have been there for quite a while and it seems it always been accessed through the (originally GConf) GSettings API. My only concern in this case is that one can slighly corrupt the list. I'd say it's your call, if it's direct access, then we should find a way to document that very well.

Created attachment 67260 [details] [review]
Updated patch

Sorry for the delay, too much work :(

Updated patch against latest master

 - implements _tpl_conf_is_account_ignored() 
 - _tpl_conf_{get,set}_accounts_ignorelist() don't use GSLists anymore
 - added tests (a bit clumsy I'd say, but work :-)
 - coding style fixes

Uhm, ping? :-)

Created attachment 76995 [details] [review]
Updated patch

Hi, I'm back again :-)

I've added a high-level API to TplLogManager to manage the ignorelist. I believe this way is better than exposing the gconf values to users. Tests have been updated accordingly.

Will be in next development release, 0.9.0.

commit c0a2104a590e9e68226f836d22ff6e645f3e33d3
Author: Dan Vrátil <dvratil@redhat.com>
Date:   Thu Apr 4 11:24:21 2013 -0400

    Add ignore list capability
    
    This allow disabling logging for a specific contact.
    
    Fix bug https://bugs.freedesktop.org/show_bug.cgi?id=54033