Adding more tests for ibus-gconf daemon.

Adding more tests for ibus-gconf daemon.

Support boolean_list, int_list, double_list, and string_list parameters on set_config and get_config commands.

BUG=none
TEST=none

[satorux1, 2010-03-29 04:44:55]

lgtm. please add a comment to desktopui_IBusTest.py.
you can ignore my comments in ibusclient.cc.

On 2010/03/29 03:44:18, Yusuke Sato wrote:
>

[satorux1, 2010-03-29 04:45:07]

http://codereview.chromium.org/1540001/diff/2001/3001
File client/deps/ibusclient/src/ibusclient.cc (right):

http://codereview.chromium.org/1540001/diff/2001/3001#newcode56
client/deps/ibusclient/src/ibusclient.cc:56: g_value_set_string(&gvalue,
kDummyValueString);
Not a big deal, but we could share the code a bit more by adding a function like
GetGValueTypeFromStringOrDie()? This may be useful in GetConfigAndPrintResult()
too, but this may be overkill. It's up to you.

int type = GetGValueTypeFromStringOrDie(type_string);
g_value_init(&gvalue, type);
if (type == G_TYPE_BOOLEAN) {
  g_value_set_boolean(&gvalue, kDummyValueBoolean);
} else if (...) {
  ...
} else if (type == G_TYPE_VALUE_ARRAY) {
  // Process list types.
  ...
  string element_type_string = ...  // get the prefix portion from boolean_list,
etc.
  for () {
    int element_type = GetValueTypeFromStringOrDie(sub_type_string);
    ...
  }
} else {
  g_assert_not_reached();
}

http://codereview.chromium.org/1540001/diff/2001/3002
File client/site_tests/desktopui_IBusTest/desktopui_IBusTest.py (right):

http://codereview.chromium.org/1540001/diff/2001/3002#newcode56
client/site_tests/desktopui_IBusTest/desktopui_IBusTest.py:56: # First, write a
dummy value which is not |type_name| type.
Why are we doing this? Could you add a comment?

http://codereview.chromium.org/1540001/diff/2001/3002#newcode57
client/site_tests/desktopui_IBusTest/desktopui_IBusTest.py:57: out =
self.run_ibusclient('set_config %s' % type_name_2)
wrong_type_name may be easier to read?

[Yusuke Sato, 2010-03-29 13:25:11]

http://codereview.chromium.org/1540001/diff/2001/3001
File client/deps/ibusclient/src/ibusclient.cc (right):

http://codereview.chromium.org/1540001/diff/2001/3001#newcode56
client/deps/ibusclient/src/ibusclient.cc:56: g_value_set_string(&gvalue,
kDummyValueString);
On 2010/03/29 04:45:07, satorux1 wrote:
> Not a big deal, but we could share the code a bit more by adding a function
like
> GetGValueTypeFromStringOrDie()? This may be useful in
GetConfigAndPrintResult()
> too, but this may be overkill. It's up to you.
> 
> int type = GetGValueTypeFromStringOrDie(type_string);
> g_value_init(&gvalue, type);
> if (type == G_TYPE_BOOLEAN) {
>   g_value_set_boolean(&gvalue, kDummyValueBoolean);
> } else if (...) {
>   ...
> } else if (type == G_TYPE_VALUE_ARRAY) {
>   // Process list types.
>   ...
>   string element_type_string = ...  // get the prefix portion from
boolean_list,
> etc.
>   for () {
>     int element_type = GetValueTypeFromStringOrDie(sub_type_string);
>     ...
>   }
> } else {
>   g_assert_not_reached();
> }
> 

That looks better, thanks. Fixed.

http://codereview.chromium.org/1540001/diff/2001/3002
File client/site_tests/desktopui_IBusTest/desktopui_IBusTest.py (right):

http://codereview.chromium.org/1540001/diff/2001/3002#newcode56
client/site_tests/desktopui_IBusTest/desktopui_IBusTest.py:56: # First, write a
dummy value which is not |type_name| type.
On 2010/03/29 04:45:07, satorux1 wrote:
> Why are we doing this? Could you add a comment?

To make sure the second set_config overwrites this |wrong_type_name| value. I
added this since a very first version of the ibus-memconf daemon had this
problem :(

Added a comment.

http://codereview.chromium.org/1540001/diff/2001/3002#newcode57
client/site_tests/desktopui_IBusTest/desktopui_IBusTest.py:57: out =
self.run_ibusclient('set_config %s' % type_name_2)
On 2010/03/29 04:45:07, satorux1 wrote:
> wrong_type_name may be easier to read?

Done.

[satorux1, 2010-03-30 02:32:15]

http://codereview.chromium.org/1540001/diff/12001/13001
File client/deps/ibusclient/src/ibusclient.cc (right):

http://codereview.chromium.org/1540001/diff/12001/13001#newcode30
client/deps/ibusclient/src/ibusclient.cc:30: // Convert |list_type_string| into
its element type (e.g. "int_list" to "int").
Converts

http://codereview.chromium.org/1540001/diff/12001/13001#newcode76
client/deps/ibusclient/src/ibusclient.cc:76: g_value_init(&gvalue,
G_TYPE_BOOLEAN);
Can we do this before switch?

  g_value_init(&gvalue, gtype);

http://codereview.chromium.org/1540001/diff/12001/13001#newcode97
client/deps/ibusclient/src/ibusclient.cc:97: // constant expression.
Sigh. Maybe it would be more consistent to to use 'if ... else if'?

http://codereview.chromium.org/1540001/diff/12001/13001#newcode237
client/deps/ibusclient/src/ibusclient.cc:237: }
You could shorten the code like:

match = ((element_gtype == G_TYPE_BOOLEAN &&
          g_value_get_boolean(&(array->values[i]))
              == kDummyValueBooleanArray[i])) ||
         (element_gtype == G_TYPE_INT &&
          g_value_get_int(&(array->values[i]))
              == kDummyValueIntArray[i])) ||
          ....);

[Yusuke Sato, 2010-03-30 03:52:31]

http://codereview.chromium.org/1540001/diff/12001/13001
File client/deps/ibusclient/src/ibusclient.cc (right):

http://codereview.chromium.org/1540001/diff/12001/13001#newcode30
client/deps/ibusclient/src/ibusclient.cc:30: // Convert |list_type_string| into
its element type (e.g. "int_list" to "int").
On 2010/03/30 02:32:15, satorux1 wrote:
> Converts

Done.

http://codereview.chromium.org/1540001/diff/12001/13001#newcode76
client/deps/ibusclient/src/ibusclient.cc:76: g_value_init(&gvalue,
G_TYPE_BOOLEAN);
Oops, forgot to do so. Thanks, fixed.

On 2010/03/30 02:32:15, satorux1 wrote:
> Can we do this before switch?
> 
>   g_value_init(&gvalue, gtype);

http://codereview.chromium.org/1540001/diff/12001/13001#newcode97
client/deps/ibusclient/src/ibusclient.cc:97: // constant expression.
On 2010/03/30 02:32:15, satorux1 wrote:
> Sigh. Maybe it would be more consistent to to use 'if ... else if'?

ok, i don't have strong opinion here. removed all switch-cases.

http://codereview.chromium.org/1540001/diff/12001/13001#newcode237
client/deps/ibusclient/src/ibusclient.cc:237: }
Hmm.. it's a bit hard to parse to me. Let me use if-else (instead of
switch-case) to shorten the code.

On 2010/03/30 02:32:15, satorux1 wrote:
> You could shorten the code like:
> 
> match = ((element_gtype == G_TYPE_BOOLEAN &&
>           g_value_get_boolean(&(array->values[i]))
>               == kDummyValueBooleanArray[i])) ||
>          (element_gtype == G_TYPE_INT &&
>           g_value_get_int(&(array->values[i]))
>               == kDummyValueIntArray[i])) ||
>           ....);

[satorux1, 2010-03-30 03:56:05]

LGTM