Add asynchronous InputContext.IsEnabled and InputContext.GetEngine APIs.

Add asynchronous InputContext.IsEnabled and InputContext.GetEngine APIs.

* Added asynchronous InputContext.IsEnabled and InputContext.GetEngine APIs. Now all InputContext IPCs can be async.
* Added comments to src/ibusinputcontext.h.
* Moved input context tests from ibus-bus.c to ibus-inputcontext.c (new file), and fixed flaky tests.
* Fixed typos in bus/.

BUG=http://code.google.com/p/ibus/issues/detail?id=1215
TEST=ran the new test

[Yusuke Sato, 2011-03-14 07:09:23]

If you think it's better to change all APIs (i.e. add timeout, cancellable, ...
parameters to all async APIs) for consistency, please let me know. I'll do it.

[Peng, 2011-03-14 13:40:46]

http://codereview.appspot.com/4275048/diff/2001/src/ibusinputcontext.c
File src/ibusinputcontext.c (right):

http://codereview.appspot.com/4275048/diff/2001/src/ibusinputcontext.c#newcod...
src/ibusinputcontext.c:647: if (error != NULL && error->message != NULL) {
Is it necessary?

http://codereview.appspot.com/4275048/diff/2001/src/ibusinputcontext.c#newcod...
src/ibusinputcontext.c:845: GError            **error)
The return type is boolean value. I think the caller may be confused by the
return value. Maybe we should define the finish function like:
gboolean xxx_finish(IBusInputContext *context, GAsyncResult *res, gboolean
*retval, GError **error);

Return true, if it is successful, otherwise returns false, and use retval to get
the real return value from ibus-daemon.

And I think for integer, it also has similar problem. Maybe it is better to
define _finish function like it too.

What do you think?

http://codereview.appspot.com/4275048/diff/2001/src/ibusinputcontext.c#newcod...
src/ibusinputcontext.c:878: if (error != NULL && error->message != NULL) {
Is it necessary?

http://codereview.appspot.com/4275048/diff/2001/src/ibusinputcontext.c#newcod...
src/ibusinputcontext.c:949: if (error != NULL && error->message != NULL) {
Is it necessary?

http://codereview.appspot.com/4275048/diff/2001/src/tests/ibus-inputcontext.c
File src/tests/ibus-inputcontext.c (right):

http://codereview.appspot.com/4275048/diff/2001/src/tests/ibus-inputcontext.c...
src/tests/ibus-inputcontext.c:1: /* -*- mode: C; c-basic-offset: 4;
indent-tabs-mode: nil; -*- */
It is better to add the license header.

[Yusuke Sato, 2011-03-18 14:08:01]

all done. But it seems that something weird happened to this rietveld review.

So could you review http://codereview.appspot.com/4298049/ instead? I've
uploaded the latest CL there.

http://codereview.appspot.com/4275048/diff/2001/src/ibusinputcontext.c
File src/ibusinputcontext.c (right):

http://codereview.appspot.com/4275048/diff/2001/src/ibusinputcontext.c#newcod...
src/ibusinputcontext.c:647: if (error != NULL && error->message != NULL) {
I think GDBus functions sometimes return 'fail' without initializing GError. For
example, g_initable_new_valist might return immediately without initializing
GError when g_return_val_if_fail(G_TYPE_IS_INITABLE (object_type), NULL) fails.

I tend to be very conservative since I experienced some SEGVs due to the issue
in the past.

What do you think?

On 2011/03/14 13:40:46, Peng wrote:
> Is it necessary?

http://codereview.appspot.com/4275048/diff/2001/src/ibusinputcontext.c#newcod...
src/ibusinputcontext.c:845: GError            **error)
Sounds reasonable to me. Fixed the signature of
ibus_input_context_is_enabled_async_finish.

BTW, what do you mean by 'for integer'? Do you mean the async functions in
ibusbus.h?

On 2011/03/14 13:40:46, Peng wrote:
> The return type is boolean value. I think the caller may be confused by the
> return value. Maybe we should define the finish function like:
> gboolean xxx_finish(IBusInputContext *context, GAsyncResult *res, gboolean
> *retval, GError **error);
> 
> Return true, if it is successful, otherwise returns false, and use retval to
get
> the real return value from ibus-daemon.
> 
> And I think for integer, it also has similar problem. Maybe it is better to
> define _finish function like it too.
> 
> What do you think?

http://codereview.appspot.com/4275048/diff/2001/src/ibusinputcontext.c#newcod...
src/ibusinputcontext.c:878: if (error != NULL && error->message != NULL) {
On 2011/03/14 13:40:46, Peng wrote:
> Is it necessary?

ditto.

http://codereview.appspot.com/4275048/diff/2001/src/ibusinputcontext.c#newcod...
src/ibusinputcontext.c:949: if (error != NULL && error->message != NULL) {
On 2011/03/14 13:40:46, Peng wrote:
> Is it necessary?

ditto.

http://codereview.appspot.com/4275048/diff/2001/src/tests/ibus-inputcontext.c
File src/tests/ibus-inputcontext.c (right):

http://codereview.appspot.com/4275048/diff/2001/src/tests/ibus-inputcontext.c...
src/tests/ibus-inputcontext.c:1: /* -*- mode: C; c-basic-offset: 4;
indent-tabs-mode: nil; -*- */
On 2011/03/14 13:40:46, Peng wrote:
> It is better to add the license header.

Done.

[Peng, 2011-03-18 15:30:33]

http://codereview.appspot.com/4275048/diff/2001/src/ibusinputcontext.c
File src/ibusinputcontext.c (right):

http://codereview.appspot.com/4275048/diff/2001/src/ibusinputcontext.c#newcod...
src/ibusinputcontext.c:647: if (error != NULL && error->message != NULL) {
IMHO, error must be set, if function call is failed.

For the case you mentioned, it is a wrong using the API. The passed arguments
are invalidate. That must be fixed. I think it is not too reasonable to recovery
from that kind of faults instead of segment fault. That makes things much worse.
right?

For other cases, if some APIs do not set error, it should be a bug of those
APIs. We need fix them.

If we want to add some checking code here, maybe it is better to use g_assert
(error != NULL).


On 2011/03/18 14:08:01, Yusuke Sato wrote:
> I think GDBus functions sometimes return 'fail' without initializing GError.
For
> example, g_initable_new_valist might return immediately without initializing
> GError when g_return_val_if_fail(G_TYPE_IS_INITABLE (object_type), NULL)
fails.
> 
> I tend to be very conservative since I experienced some SEGVs due to the issue
> in the past.
> 
> What do you think?
> 
> On 2011/03/14 13:40:46, Peng wrote:
> > Is it necessary?
>

http://codereview.appspot.com/4275048/diff/2001/src/ibusinputcontext.c#newcod...
src/ibusinputcontext.c:845: GError            **error)
On 2011/03/18 14:08:01, Yusuke Sato wrote:
> Sounds reasonable to me. Fixed the signature of
> ibus_input_context_is_enabled_async_finish.
> 
> BTW, what do you mean by 'for integer'? Do you mean the async functions in
> ibusbus.h?

Yeah. Some functions return gint. It is difficult to define which value is
failed. 0 -1, or other.

> 
> On 2011/03/14 13:40:46, Peng wrote:
> > The return type is boolean value. I think the caller may be confused by the
> > return value. Maybe we should define the finish function like:
> > gboolean xxx_finish(IBusInputContext *context, GAsyncResult *res, gboolean
> > *retval, GError **error);
> > 
> > Return true, if it is successful, otherwise returns false, and use retval to
> get
> > the real return value from ibus-daemon.
> > 
> > And I think for integer, it also has similar problem. Maybe it is better to
> > define _finish function like it too.
> > 
> > What do you think?
>

[Yusuke Sato, 2011-03-21 05:13:32]

http://codereview.appspot.com/4275048/diff/2001/src/ibusinputcontext.c
File src/ibusinputcontext.c (right):

http://codereview.appspot.com/4275048/diff/2001/src/ibusinputcontext.c#newcod...
src/ibusinputcontext.c:647: if (error != NULL && error->message != NULL) {
OK, I'm convinced. I'll remove the redundant check.
Please take a look at the new review URL.

On 2011/03/18 15:30:33, Peng wrote:
> IMHO, error must be set, if function call is failed.
> 
> For the case you mentioned, it is a wrong using the API. The passed arguments
> are invalidate. That must be fixed. I think it is not too reasonable to
recovery
> from that kind of faults instead of segment fault. That makes things much
worse.
> right?
> 
> For other cases, if some APIs do not set error, it should be a bug of those
> APIs. We need fix them.
> 
> If we want to add some checking code here, maybe it is better to use g_assert
> (error != NULL).
> 
> 
> On 2011/03/18 14:08:01, Yusuke Sato wrote:
> > I think GDBus functions sometimes return 'fail' without initializing GError.
> For
> > example, g_initable_new_valist might return immediately without initializing
> > GError when g_return_val_if_fail(G_TYPE_IS_INITABLE (object_type), NULL)
> fails.
> > 
> > I tend to be very conservative since I experienced some SEGVs due to the
issue
> > in the past.
> > 
> > What do you think?
> > 
> > On 2011/03/14 13:40:46, Peng wrote:
> > > Is it necessary?
> > 
>

http://codereview.appspot.com/4275048/diff/2001/src/ibusinputcontext.c#newcod...
src/ibusinputcontext.c:845: GError            **error)
ok, I'll file a bug on code.google.com/p/ibus.


On 2011/03/18 15:30:33, Peng wrote:
> On 2011/03/18 14:08:01, Yusuke Sato wrote:
> > Sounds reasonable to me. Fixed the signature of
> > ibus_input_context_is_enabled_async_finish.
> > 
> > BTW, what do you mean by 'for integer'? Do you mean the async functions in
> > ibusbus.h?
> 
> Yeah. Some functions return gint. It is difficult to define which value is
> failed. 0 -1, or other.
> 
> > 
> > On 2011/03/14 13:40:46, Peng wrote:
> > > The return type is boolean value. I think the caller may be confused by
the
> > > return value. Maybe we should define the finish function like:
> > > gboolean xxx_finish(IBusInputContext *context, GAsyncResult *res, gboolean
> > > *retval, GError **error);
> > > 
> > > Return true, if it is successful, otherwise returns false, and use retval
to
> > get
> > > the real return value from ibus-daemon.
> > > 
> > > And I think for integer, it also has similar problem. Maybe it is better
to
> > > define _finish function like it too.
> > > 
> > > What do you think?
> > 
>