Add ibus_config_watch/unwatch.

Add ibus_config_watch/unwatch.

Currently IBusConfig proxy is notified every config value change.
e.g. ibus-m17n will get notified even when ibus-anthy's config values
are changed.

With this patch, IBusConfig proxy will be able to restrict
notification by calling ibus_config_watch().  For example, after:

ibus_config_watch (config, "engine/M17N/hi/inscript", NULL);

it will be notified value changes only under
"engine/M17N/hi/inscript" section.

BUG=none
TEST=manually with modified ibus-m17n

[Daiki Ueno, 2011-09-21 05:58:24]

To test, add

ibus_config_watch (config, klass->config_section, NULL);

at the end of ibus_m17n_engine_class_init in ibus-m17n.

[Peng, 2011-09-22 02:42:23]

https://codereview.appspot.com/5091045/diff/1/src/ibusconfig.c
File src/ibusconfig.c (right):

https://codereview.appspot.com/5091045/diff/1/src/ibusconfig.c#newcode55
src/ibusconfig.c:55: G_DEFINE_TYPE (IBusConfig, ibus_config, IBUS_TYPE_PROXY)
Could you implement G_TYPE_INITABLE and G_TYPE_ASYNC_INITABLE and subscribe
signal in those interfaces?

You may check the code in ibusproxy.c as references.

https://codereview.appspot.com/5091045/diff/1/src/ibusconfig.c#newcode109
src/ibusconfig.c:109: }
It is better to remove all match rules here

https://codereview.appspot.com/5091045/diff/1/src/ibusconfig.c#newcode149
src/ibusconfig.c:149: GString *str = g_string_new ("");
g_string_new ("type='signal',"
                      "interface='" IBUS_INTERFACE_CONFIG "',"
                      "path='" IBUS_PATH_CONFIG "',"
                      "member='ValueChanged'");

https://codereview.appspot.com/5091045/diff/1/src/ibusconfig.c#newcode199
src/ibusconfig.c:199: g_free (rule);
should free rule even if result != NULL

https://codereview.appspot.com/5091045/diff/1/src/ibusconfig.c#newcode237
src/ibusconfig.c:237: g_free (rule);
ditto

https://codereview.appspot.com/5091045/diff/1/src/ibusconfig.c#newcode292
src/ibusconfig.c:292: initable,
It is better to add a ref of config and use g_object_unref as
user_data_free_func?

[Daiki Ueno, 2011-09-22 23:25:40]

Thanks for the comments.  I'll update the CL soon.

BTW, this is not an urgent issue and I'll be fully occupied with house-moving
this weekend, so please go ahead to release ibus-1.4 without this.

[Daiki Ueno, 2011-09-28 02:43:48]

Sorry for the delay.  I've moved.

https://codereview.appspot.com/5091045/diff/1/src/ibusconfig.c
File src/ibusconfig.c (right):

https://codereview.appspot.com/5091045/diff/1/src/ibusconfig.c#newcode55
src/ibusconfig.c:55: G_DEFINE_TYPE (IBusConfig, ibus_config, IBUS_TYPE_PROXY)
On 2011/09/22 02:42:23, Peng wrote:
> Could you implement G_TYPE_INITABLE and G_TYPE_ASYNC_INITABLE and subscribe
> signal in those interfaces?
> 
> You may check the code in ibusproxy.c as references.

Done.  One concern I have noticed is that, when ibus-daemon
(ibusimpl.c:_dbus_name_owner_changed_cb) creates an IBusConfig instance with
g_initable_new(), name owner will never set and signal subscription always
fails.   So I added an extra check of flags in ibus_config_init_finish().

https://codereview.appspot.com/5091045/diff/1/src/ibusconfig.c#newcode109
src/ibusconfig.c:109: }
On 2011/09/22 02:42:23, Peng wrote:
> It is better to remove all match rules here

Hmm, I think it may be too much because:

- match rules requested will be automatically removed in server side (in
bus_match_rule_connection_destroy_cb) when a client connection is closed
- to remove match rules, a proxy will need to keep a list of requested match
rules

https://codereview.appspot.com/5091045/diff/1/src/ibusconfig.c#newcode149
src/ibusconfig.c:149: GString *str = g_string_new ("");
On 2011/09/22 02:42:23, Peng wrote:
> g_string_new ("type='signal',"
>                       "interface='" IBUS_INTERFACE_CONFIG "',"
>                       "path='" IBUS_PATH_CONFIG "',"
>                       "member='ValueChanged'");

Done.

https://codereview.appspot.com/5091045/diff/1/src/ibusconfig.c#newcode199
src/ibusconfig.c:199: g_free (rule);
On 2011/09/22 02:42:23, Peng wrote:
> should free rule even if result != NULL

Done.

https://codereview.appspot.com/5091045/diff/1/src/ibusconfig.c#newcode237
src/ibusconfig.c:237: g_free (rule);
On 2011/09/22 02:42:23, Peng wrote:
> ditto 

Done.

https://codereview.appspot.com/5091045/diff/1/src/ibusconfig.c#newcode292
src/ibusconfig.c:292: initable,
On 2011/09/22 02:42:23, Peng wrote:
> It is better to add a ref of config and use g_object_unref as
> user_data_free_func?

Done.

[Peng, 2011-09-28 19:17:15]

https://codereview.appspot.com/5091045/diff/1/src/ibusconfig.c
File src/ibusconfig.c (right):

https://codereview.appspot.com/5091045/diff/1/src/ibusconfig.c#newcode109
src/ibusconfig.c:109: }
On 2011/09/28 02:43:48, Daiki Ueno wrote:
> On 2011/09/22 02:42:23, Peng wrote:
> > It is better to remove all match rules here
> 
> Hmm, I think it may be too much because:
> 
> - match rules requested will be automatically removed in server side (in
> bus_match_rule_connection_destroy_cb) when a client connection is closed
> - to remove match rules, a proxy will need to keep a list of requested match
> rules

My concern is the config could be destroyed but the connection is still alive.

https://codereview.appspot.com/5091045/diff/5001/src/ibusconfig.c
File src/ibusconfig.c (right):

https://codereview.appspot.com/5091045/diff/5001/src/ibusconfig.c#newcode697
src/ibusconfig.c:697: _signal_subscribe (config);
Could we do it even if the ibus-conf is not running?

[Daiki Ueno, 2012-02-27 04:28:22]

Sorry for not updating this issue for a long time.

I have rebased it against the git master and implemented purging all match rules
on destroy.  Could you review again if you have time?

https://codereview.appspot.com/5091045/diff/1/src/ibusconfig.c
File src/ibusconfig.c (right):

https://codereview.appspot.com/5091045/diff/1/src/ibusconfig.c#newcode109
src/ibusconfig.c:109: }
On 2011/09/28 19:17:15, Peng wrote:
> On 2011/09/28 02:43:48, Daiki Ueno wrote:
> > On 2011/09/22 02:42:23, Peng wrote:
> > > It is better to remove all match rules here
> > 
> > Hmm, I think it may be too much because:
> > 
> > - match rules requested will be automatically removed in server side (in
> > bus_match_rule_connection_destroy_cb) when a client connection is closed
> > - to remove match rules, a proxy will need to keep a list of requested match
> > rules
> 
> My concern is the config could be destroyed but the connection is still alive.

Done.

https://codereview.appspot.com/5091045/diff/5001/src/ibusconfig.c
File src/ibusconfig.c (right):

https://codereview.appspot.com/5091045/diff/5001/src/ibusconfig.c#newcode697
src/ibusconfig.c:697: _signal_subscribe (config);
On 2011/09/28 19:17:15, Peng wrote:
> Could we do it even if the ibus-conf is not running?

Right.  Removed unnecessary code.

[Peng, 2012-02-27 16:42:52]

https://codereview.appspot.com/5091045/diff/8003/src/ibusconfig.c
File src/ibusconfig.c (right):

https://codereview.appspot.com/5091045/diff/8003/src/ibusconfig.c#newcode29
src/ibusconfig.c:29: #define DBUS_INTERFACE_DBUS "org.freedesktop.DBus"
Those macros are also used by ibusbus.c. Probably it is better to put them into
a header file for sharing code. (maybe ibusinternal.h?)

https://codereview.appspot.com/5091045/diff/8003/src/ibusconfig.c#newcode67
src/ibusconfig.c:67: static void      _purge_all_match_rules    
(GDBusConnection    *connection,
static void _remove_all_match_rules (IBusConfig *config) may be better?

https://codereview.appspot.com/5091045/diff/8003/src/ibusconfig.c#newcode254
src/ibusconfig.c:254: _remove_match_rule (connection, *rules);
g_free(*rule) ?

https://codereview.appspot.com/5091045/diff/8003/src/ibusconfig.c#newcode263
src/ibusconfig.c:263: g_return_val_if_fail (IBUS_IS_CONFIG (config), FALSE);
g_assert ((section != NULL) || (section == NULL && name == NULL)).

https://codereview.appspot.com/5091045/diff/8003/src/ibusconfig.c#newcode263
src/ibusconfig.c:263: g_return_val_if_fail (IBUS_IS_CONFIG (config), FALSE);
g_assert((section != NULL) || (section == NULL && name == NULL))

https://codereview.appspot.com/5091045/diff/8003/src/ibusconfig.c#newcode276
src/ibusconfig.c:276: priv->watch_rules = g_array_new (TRUE, FALSE, sizeof
(gchar *));
Maybe you could just set size of watch_rules to 0. instead of free and allocate
a new one.

https://codereview.appspot.com/5091045/diff/8003/src/ibusconfig.c#newcode297
src/ibusconfig.c:297: if (g_strcmp0 (_rule, rule) == 0) {
I think it is not necessary to check it. ibus-daemon can handle adding and
removing same rule two or more times.

Otherwise, you could track the refcount of each watch, when caller call unwatch,
you should decrease refcount of the watch, when refcount is zero to do the real
remove.

https://codereview.appspot.com/5091045/diff/8003/src/ibusconfig.c#newcode321
src/ibusconfig.c:321: g_return_val_if_fail (IBUS_IS_CONFIG (config), FALSE);
ditto

https://codereview.appspot.com/5091045/diff/8003/src/ibusconfig.h
File src/ibusconfig.h (right):

https://codereview.appspot.com/5091045/diff/8003/src/ibusconfig.h#newcode71
src/ibusconfig.h:71: struct _IBusConfig {
/*< private >*/

https://codereview.appspot.com/5091045/diff/8003/src/ibusconfig.h#newcode73
src/ibusconfig.h:73: IBusConfigPrivate *priv;
Add a new line here

https://codereview.appspot.com/5091045/diff/8003/src/ibusconfig.h#newcode77
src/ibusconfig.h:77: struct _IBusConfigClass {
/*< private >*/

https://codereview.appspot.com/5091045/diff/8003/src/ibusconfig.h#newcode78
src/ibusconfig.h:78: IBusProxyClass parent;
Add a new line here

https://codereview.appspot.com/5091045/diff/8003/src/ibusconfig.h#newcode325
src/ibusconfig.h:325: const gchar        *name);
Could you please add some unittest for those two apis?

[Peng, 2012-02-27 16:44:05]

+Yusuke, I think some IMEs in ChromeOS may want to use those two API.

[Yusuke Sato, 2012-02-28 04:09:39]

On 2012/02/27 16:44:05, Peng wrote:
> +Yusuke, I think some IMEs in ChromeOS may want to use those two API.

the interface looks good to me. thanks Peng for letting me know about this in
advance.

[Daiki Ueno, 2012-02-28 08:46:36]

https://codereview.appspot.com/5091045/diff/8003/src/ibusconfig.c
File src/ibusconfig.c (right):

https://codereview.appspot.com/5091045/diff/8003/src/ibusconfig.c#newcode29
src/ibusconfig.c:29: #define DBUS_INTERFACE_DBUS "org.freedesktop.DBus"
On 2012/02/27 16:42:52, Peng wrote:
> Those macros are also used by ibusbus.c. Probably it is better to put them
into
> a header file for sharing code. (maybe ibusinternal.h?)

Done.

https://codereview.appspot.com/5091045/diff/8003/src/ibusconfig.c#newcode67
src/ibusconfig.c:67: static void      _purge_all_match_rules    
(GDBusConnection    *connection,
On 2012/02/27 16:42:52, Peng wrote:
> static void _remove_all_match_rules (IBusConfig *config) may be better?

Done.

https://codereview.appspot.com/5091045/diff/8003/src/ibusconfig.c#newcode254
src/ibusconfig.c:254: _remove_match_rule (connection, *rules);
On 2012/02/27 16:42:52, Peng wrote:
> g_free(*rule) ?

Done.

https://codereview.appspot.com/5091045/diff/8003/src/ibusconfig.c#newcode263
src/ibusconfig.c:263: g_return_val_if_fail (IBUS_IS_CONFIG (config), FALSE);
On 2012/02/27 16:42:52, Peng wrote:
> g_assert ((section != NULL) || (section == NULL && name == NULL)).

Done.

https://codereview.appspot.com/5091045/diff/8003/src/ibusconfig.c#newcode276
src/ibusconfig.c:276: priv->watch_rules = g_array_new (TRUE, FALSE, sizeof
(gchar *));
On 2012/02/27 16:42:52, Peng wrote:
> Maybe you could just set size of watch_rules to 0. instead of free and
allocate
> a new one.

Done.

https://codereview.appspot.com/5091045/diff/8003/src/ibusconfig.c#newcode297
src/ibusconfig.c:297: if (g_strcmp0 (_rule, rule) == 0) {
On 2012/02/27 16:42:52, Peng wrote:
> I think it is not necessary to check it. ibus-daemon can handle adding and
> removing same rule two or more times.
> 
> Otherwise, you could track the refcount of each watch, when caller call
unwatch,
> you should decrease refcount of the watch, when refcount is zero to do the
real
> remove.

OK, removed.

https://codereview.appspot.com/5091045/diff/8003/src/ibusconfig.c#newcode321
src/ibusconfig.c:321: g_return_val_if_fail (IBUS_IS_CONFIG (config), FALSE);
On 2012/02/27 16:42:52, Peng wrote:
> ditto

Done.

https://codereview.appspot.com/5091045/diff/8003/src/ibusconfig.h
File src/ibusconfig.h (right):

https://codereview.appspot.com/5091045/diff/8003/src/ibusconfig.h#newcode71
src/ibusconfig.h:71: struct _IBusConfig {
On 2012/02/27 16:42:52, Peng wrote:
> /*< private >*/

Done.

https://codereview.appspot.com/5091045/diff/8003/src/ibusconfig.h#newcode73
src/ibusconfig.h:73: IBusConfigPrivate *priv;
On 2012/02/27 16:42:52, Peng wrote:
> Add a new line here

Done.

https://codereview.appspot.com/5091045/diff/8003/src/ibusconfig.h#newcode77
src/ibusconfig.h:77: struct _IBusConfigClass {
On 2012/02/27 16:42:52, Peng wrote:
> /*< private >*/

Done.

https://codereview.appspot.com/5091045/diff/8003/src/ibusconfig.h#newcode78
src/ibusconfig.h:78: IBusProxyClass parent;
On 2012/02/27 16:42:52, Peng wrote:
> Add a new line here

Done.

https://codereview.appspot.com/5091045/diff/8003/src/ibusconfig.h#newcode325
src/ibusconfig.h:325: const gchar        *name);
On 2012/02/27 16:42:52, Peng wrote:
> Could you please add some unittest for those two apis?

Added.  Since there seems no easy way to force trigger ValueChanged signal, I
added a helper program "ibus-config-set-value" in tests dir.

[Peng, 2012-02-28 16:57:59]

http://codereview.appspot.com/5091045/diff/14002/src/ibusconfig.c
File src/ibusconfig.c (right):

http://codereview.appspot.com/5091045/diff/14002/src/ibusconfig.c#newcode191
src/ibusconfig.c:191: _add_match_rule (GDBusConnection *connection, const gchar
*rule)
Is it possible to use ibus_bus_add_match() instead of creating a new function?

http://codereview.appspot.com/5091045/diff/14002/src/ibusconfig.c#newcode218
src/ibusconfig.c:218: _remove_match_rule (GDBusConnection *connection, const
gchar *rule)
ditto

http://codereview.appspot.com/5091045/diff/14002/src/ibusconfig.c#newcode246
src/ibusconfig.c:246: IBusConfigPrivate *priv = IBUS_CONFIG_GET_PRIVATE
(config);
Use config->priv directly. Not necessary to use IBUS_CONFIG_GET_PRIVATE()

http://codereview.appspot.com/5091045/diff/14002/src/ibusconfig.c#newcode267
src/ibusconfig.c:267: IBusConfigPrivate *priv = IBUS_CONFIG_GET_PRIVATE
(config);
ditto

http://codereview.appspot.com/5091045/diff/14002/src/ibusconfig.c#newcode309
src/ibusconfig.c:309: g_assert ((section != NULL) || (section == NULL && name ==
NULL));
Does it accept section == NULL && name == NULL?

http://codereview.appspot.com/5091045/diff/14002/src/ibusconfig.c#newcode311
src/ibusconfig.c:311: IBusConfigPrivate *priv = IBUS_CONFIG_GET_PRIVATE
(config);
ditto

http://codereview.appspot.com/5091045/diff/14002/src/ibusconfig.c#newcode764
src/ibusconfig.c:764: IBusConfigPrivate *priv = IBUS_CONFIG_GET_PRIVATE
(initable);
ditto

http://codereview.appspot.com/5091045/diff/14002/src/ibusinternal.h
File src/ibusinternal.h (right):

http://codereview.appspot.com/5091045/diff/14002/src/ibusinternal.h#newcode55
src/ibusinternal.h:55: #define DBUS_SERVICE_DBUS "org.freedesktop.DBus"
Could you remove those macros definition in ibusbus.c as well?

http://codereview.appspot.com/5091045/diff/14002/src/tests/ibus-config.c
File src/tests/ibus-config.c (right):

http://codereview.appspot.com/5091045/diff/14002/src/tests/ibus-config.c#newc...
src/tests/ibus-config.c:218: test_config_watch (void)
It is better to cover all functions and situations.
For example:
watch("a", null)
set("a", "b")
set("c", "b")
watch("a", "b")
set("a", "b")
set("c", "b")
set("a", "c")
ibus_config_unwatch()
etc

http://codereview.appspot.com/5091045/diff/14002/src/tests/ibus-config.c#newc...
src/tests/ibus-config.c:235: spawn_set_value (config, "test", "v1", data);
Could you just call ibus_config_set.. in here?
and then call ibus_main() and in the value_changed_cb, you could call
ibus_quit() to quit from ibus_main()?

[Daiki Ueno, 2012-02-29 02:57:49]

https://codereview.appspot.com/5091045/diff/14002/src/ibusconfig.c
File src/ibusconfig.c (right):

https://codereview.appspot.com/5091045/diff/14002/src/ibusconfig.c#newcode191
src/ibusconfig.c:191: _add_match_rule (GDBusConnection *connection, const gchar
*rule)
On 2012/02/28 16:57:59, Peng wrote:
> Is it possible to use ibus_bus_add_match() instead of creating a new function?

Done.

https://codereview.appspot.com/5091045/diff/14002/src/ibusconfig.c#newcode218
src/ibusconfig.c:218: _remove_match_rule (GDBusConnection *connection, const
gchar *rule)
On 2012/02/28 16:57:59, Peng wrote:
> ditto

Done.

https://codereview.appspot.com/5091045/diff/14002/src/ibusconfig.c#newcode246
src/ibusconfig.c:246: IBusConfigPrivate *priv = IBUS_CONFIG_GET_PRIVATE
(config);
On 2012/02/28 16:57:59, Peng wrote:
> Use config->priv directly. Not necessary to use IBUS_CONFIG_GET_PRIVATE()

Done.

https://codereview.appspot.com/5091045/diff/14002/src/ibusconfig.c#newcode267
src/ibusconfig.c:267: IBusConfigPrivate *priv = IBUS_CONFIG_GET_PRIVATE
(config);
On 2012/02/28 16:57:59, Peng wrote:
> ditto

Done.

https://codereview.appspot.com/5091045/diff/14002/src/ibusconfig.c#newcode309
src/ibusconfig.c:309: g_assert ((section != NULL) || (section == NULL && name ==
NULL));
On 2012/02/28 16:57:59, Peng wrote:
> Does it accept section == NULL && name == NULL?

Yes, it does.

https://codereview.appspot.com/5091045/diff/14002/src/ibusconfig.c#newcode311
src/ibusconfig.c:311: IBusConfigPrivate *priv = IBUS_CONFIG_GET_PRIVATE
(config);
On 2012/02/28 16:57:59, Peng wrote:
> ditto

Done.

https://codereview.appspot.com/5091045/diff/14002/src/ibusconfig.c#newcode764
src/ibusconfig.c:764: IBusConfigPrivate *priv = IBUS_CONFIG_GET_PRIVATE
(initable);
On 2012/02/28 16:57:59, Peng wrote:
> ditto

Done.

https://codereview.appspot.com/5091045/diff/14002/src/ibusinternal.h
File src/ibusinternal.h (right):

https://codereview.appspot.com/5091045/diff/14002/src/ibusinternal.h#newcode55
src/ibusinternal.h:55: #define DBUS_SERVICE_DBUS "org.freedesktop.DBus"
On 2012/02/28 16:57:59, Peng wrote:
> Could you remove those macros definition in ibusbus.c as well?

Done.

https://codereview.appspot.com/5091045/diff/14002/src/tests/ibus-config.c
File src/tests/ibus-config.c (right):

https://codereview.appspot.com/5091045/diff/14002/src/tests/ibus-config.c#new...
src/tests/ibus-config.c:218: test_config_watch (void)
On 2012/02/28 16:57:59, Peng wrote:
> It is better to cover all functions and situations.
> For example:
> watch("a", null)
> set("a", "b")
> set("c", "b")
> watch("a", "b")
> set("a", "b")
> set("c", "b")
> set("a", "c")
> ibus_config_unwatch()
> etc

Done.

https://codereview.appspot.com/5091045/diff/14002/src/tests/ibus-config.c#new...
src/tests/ibus-config.c:235: spawn_set_value (config, "test", "v1", data);
On 2012/02/28 16:57:59, Peng wrote:
> Could you just call ibus_config_set.. in here?
> and then call ibus_main() and in the value_changed_cb, you could call
> ibus_quit() to quit from ibus_main()?

Right.  I was confused with the gconf caching behavior and it turned out that
spawn was not necessary here.  Rewrote it with unset/set_value.

[Peng, 2012-02-29 03:52:15]

lgtm. with some comments.

https://codereview.appspot.com/5091045/diff/16004/src/tests/ibus-config.c
File src/tests/ibus-config.c (right):

https://codereview.appspot.com/5091045/diff/16004/src/tests/ibus-config.c#new...
src/tests/ibus-config.c:235: ibus_config_unwatch (config, "test/s1", NULL);
I would like to add some tests after ibus_config_unwatch() to make sure the
signals will not be received after unwatch.

https://codereview.appspot.com/5091045/diff/16004/src/tests/ibus-config.c#new...
src/tests/ibus-config.c:353: ibus_config_unwatch (config, "test/s1", "n1");
ditto

[Daiki Ueno, 2012-02-29 05:26:53]

On 2012/02/29 03:52:15, Peng wrote:
> lgtm. with some comments.
> 
> https://codereview.appspot.com/5091045/diff/16004/src/tests/ibus-config.c
> File src/tests/ibus-config.c (right):
> 
>
https://codereview.appspot.com/5091045/diff/16004/src/tests/ibus-config.c#new...
> src/tests/ibus-config.c:235: ibus_config_unwatch (config, "test/s1", NULL);
> I would like to add some tests after ibus_config_unwatch() to make sure the
> signals will not be received after unwatch.
> 
>
https://codereview.appspot.com/5091045/diff/16004/src/tests/ibus-config.c#new...
> src/tests/ibus-config.c:353: ibus_config_unwatch (config, "test/s1", "n1");
> ditto

I'll add them.  However, test_config_watch() is now ~150 lines.  I'm thinking of
using g_test_add() with fixtures to separate out test data from
test_config_watch().  That will simplify the code a lot.