Change signatures of _async_finish functions so the caller of them can handle errors correctly

Change signatures of _async_finish functions so the caller of them can handle errors correctly.

BUG=ibus:1220
TEST=ran src/test/ibus-bus.

[Yusuke Sato, 2011-06-04 16:34:45]

Fixed ibusbus.h following your comment at
http://codereview.appspot.com/4275048/diff/2001/src/ibusinputcontext.c#newcod...
.

Sorry for the delay.

[Yusuke Sato, 2011-06-04 16:36:58]

+takao.fujiwara1

[Peng, 2011-06-05 03:41:52]

http://codereview.appspot.com/4538114/diff/1/src/ibusbus.c
File src/ibusbus.c (right):

http://codereview.appspot.com/4538114/diff/1/src/ibusbus.c#newcode891
src/ibusbus.c:891: ibus_bus_request_name_async));
It is better add g_assert (retval != NULL) here and other functions.

[Yusuke Sato, 2011-06-05 03:47:48]

http://codereview.appspot.com/4538114/diff/1/src/ibusbus.c
File src/ibusbus.c (right):

http://codereview.appspot.com/4538114/diff/1/src/ibusbus.c#newcode891
src/ibusbus.c:891: ibus_bus_request_name_async));
On 2011/06/05 03:41:52, Peng wrote:
> It is better add g_assert (retval != NULL) here and other functions.

Done.

[Peng, 2011-06-05 03:49:25]

LGTM

[Yusuke Sato, 2011-06-05 03:52:57]

Fujiwara-san, this patch changes the signatures of some functions in
src/ibusbus.h. Please let me know if you have any concerns.

[Peng, 2011-06-05 20:58:03]

On 2011/06/05 03:52:57, Yusuke Sato wrote:
> Fujiwara-san, this patch changes the signatures of some functions in
> src/ibusbus.h. Please let me know if you have any concerns.

It is better to make sure gobject-introspect can works with this kind of
functions.

[Yusuke Sato, 2011-06-05 22:47:13]

Could you elaborate a little more? I don't know much about gobject-introspect.

On 2011/06/05 20:58:03, Peng wrote:
> On 2011/06/05 03:52:57, Yusuke Sato wrote:
> > Fujiwara-san, this patch changes the signatures of some functions in
> > src/ibusbus.h. Please let me know if you have any concerns.
> 
> It is better to make sure gobject-introspect can works with this kind of
> functions.

[fujiwara, 2011-06-06 05:57:14]

Yes, probably I also think it's nice to work with g-i.
I added a comment.
If you have g-i, you could compare the result:

# diff IBus-1.0.gir.orig IBus-1.0.gir

http://codereview.appspot.com/4538114/diff/5001/src/ibusbus.h
File src/ibusbus.h (right):

http://codereview.appspot.com/4538114/diff/5001/src/ibusbus.h#newcode162
src/ibusbus.h:162: * @retval: A guint value returned from ibus-daemon.
Could you please add "(out):" below for gobject-introspection?
g-i doesn't have the pointer for non-classes so the "out" is needed to change
the arguments to return values.

+ * @retval: (out): A guint value returned from ibus-daemon.

http://codereview.appspot.com/4538114/diff/5001/src/ibusbus.h#newcode211
src/ibusbus.h:211: * @retval: A guint value returned from ibus-daemon.
+ * @retval: (out): A guint value returned from ibus-daemon.

http://codereview.appspot.com/4538114/diff/5001/src/ibusbus.h#newcode275
src/ibusbus.h:275: * @retval: %TRUE if the name has owner, %FALSE otherwise.
Same.

http://codereview.appspot.com/4538114/diff/5001/src/ibusbus.h#newcode753
src/ibusbus.h:753: * @retval: TRUE if "use_sys_layout" option is enabled.
Same.

http://codereview.appspot.com/4538114/diff/5001/src/ibusbus.h#newcode799
src/ibusbus.h:799: * @retval: %TRUE if "use_global_engine" option is enabled.
Same.

http://codereview.appspot.com/4538114/diff/5001/src/ibusbus.h#newcode845
src/ibusbus.h:845: * @retval: %TRUE if the current global engine is enabled.
Same.

[Peng, 2011-06-06 13:51:16]

http://live.gnome.org/GObjectIntrospection/Annotations
It is a document about g-i. Please check it.

On 2011/06/05 22:47:13, Yusuke Sato wrote:
> Could you elaborate a little more? I don't know much about gobject-introspect.
> 
> On 2011/06/05 20:58:03, Peng wrote:
> > On 2011/06/05 03:52:57, Yusuke Sato wrote:
> > > Fujiwara-san, this patch changes the signatures of some functions in
> > > src/ibusbus.h. Please let me know if you have any concerns.
> > 
> > It is better to make sure gobject-introspect can works with this kind of
> > functions.

[Peng, 2011-06-06 15:17:29]

Hi Yusuke,

I just checked some document of gio
(http://developer.gnome.org/gio/2.26/GDataInputStream.html#g-data-input-stream...).
Those functions have similar problems. So they need caller check &error to make
sure the function call is successful. Do you think we could use this way to
resolve the problem?

http://git.gnome.org/browse/glib/tree/gio/gdbusmessage.c#n1027
It is an sample code to handle the error. 


On 2011/06/06 13:51:16, Peng wrote:
> http://live.gnome.org/GObjectIntrospection/Annotations
> It is a document about g-i. Please check it.
> 
> On 2011/06/05 22:47:13, Yusuke Sato wrote:
> > Could you elaborate a little more? I don't know much about
gobject-introspect.
> > 
> > On 2011/06/05 20:58:03, Peng wrote:
> > > On 2011/06/05 03:52:57, Yusuke Sato wrote:
> > > > Fujiwara-san, this patch changes the signatures of some functions in
> > > > src/ibusbus.h. Please let me know if you have any concerns.
> > > 
> > > It is better to make sure gobject-introspect can works with this kind of
> > > functions.

[Yusuke Sato, 2011-06-07 06:38:31]

Fixed the header following Fujiwara-san's comments.

> Do you think we could use this way to resolve the problem?

I think we could, but I'd rather prefer the current way (i.e. the way I'm
proposing in this CL) because the GIO's way looks more error-prone. I think a
caller can easily forget to check the error object especially when calling a
boolean function such as ibus_bus_get_use_sys_layout_async_finish.

http://codereview.appspot.com/4538114/diff/5001/src/ibusbus.h
File src/ibusbus.h (right):

http://codereview.appspot.com/4538114/diff/5001/src/ibusbus.h#newcode162
src/ibusbus.h:162: * @retval: A guint value returned from ibus-daemon.
On 2011/06/06 05:57:14, fujiwara wrote:
> Could you please add "(out):" below for gobject-introspection?
> g-i doesn't have the pointer for non-classes so the "out" is needed to change
> the arguments to return values.
> 
> + * @retval: (out): A guint value returned from ibus-daemon.

Done.

http://codereview.appspot.com/4538114/diff/5001/src/ibusbus.h#newcode211
src/ibusbus.h:211: * @retval: A guint value returned from ibus-daemon.
On 2011/06/06 05:57:14, fujiwara wrote:
> + * @retval: (out): A guint value returned from ibus-daemon.

Done.

http://codereview.appspot.com/4538114/diff/5001/src/ibusbus.h#newcode275
src/ibusbus.h:275: * @retval: %TRUE if the name has owner, %FALSE otherwise.
On 2011/06/06 05:57:14, fujiwara wrote:
> Same.

Done.

http://codereview.appspot.com/4538114/diff/5001/src/ibusbus.h#newcode753
src/ibusbus.h:753: * @retval: TRUE if "use_sys_layout" option is enabled.
On 2011/06/06 05:57:14, fujiwara wrote:
> Same.

Done.

http://codereview.appspot.com/4538114/diff/5001/src/ibusbus.h#newcode799
src/ibusbus.h:799: * @retval: %TRUE if "use_global_engine" option is enabled.
On 2011/06/06 05:57:14, fujiwara wrote:
> Same.

Done.

http://codereview.appspot.com/4538114/diff/5001/src/ibusbus.h#newcode845
src/ibusbus.h:845: * @retval: %TRUE if the current global engine is enabled.
On 2011/06/06 05:57:14, fujiwara wrote:
> Same.

Done.

[fujiwara, 2011-06-07 07:39:41]

Thank you
LGTM

[Peng, 2011-06-07 14:41:24]

Hi Yusuke,

I wrote a simple python code to test gobject-introspection
(https://github.com/phuang/ibus/blob/test-gi/src/tests/ibus-gi.py
).

And I got a following outputs from the script:
===== With this CL =====
LD_LIBRARY_PATH=../.libs/ GI_TYPELIB_PATH=.. ./ibus-gi.py
Failure: Operation was cancelled
Success: (True, False)

===== Without this CL =====
$ LD_LIBRARY_PATH=../.libs/ GI_TYPELIB_PATH=.. ./ibus-gi.py
Failure: Operation was cancelled
Success: False

From the test script, you may find the function will return two values or raise
an Exception. I think it is not script languages friendly. The g-i always check
the GError returned from functions. If the GError is returned, g-i will raise an
Exception instead of returning values. And real return value is not used
totally. With this CL, script languages will get two return values (one of them
is totally useless, it is always true), if the function is success. That may
confuse script developers.

I suggest keeping the current signature, and improving the API document. So it
will not be a problem for c/c++ developers who should know low level things
well.

On 2011/06/07 06:38:31, Yusuke Sato wrote:
> Fixed the header following Fujiwara-san's comments.
> 
> > Do you think we could use this way to resolve the problem?
> 
> I think we could, but I'd rather prefer the current way (i.e. the way I'm
> proposing in this CL) because the GIO's way looks more error-prone. I think a
> caller can easily forget to check the error object especially when calling a
> boolean function such as ibus_bus_get_use_sys_layout_async_finish.
> 
> http://codereview.appspot.com/4538114/diff/5001/src/ibusbus.h
> File src/ibusbus.h (right):
> 
> http://codereview.appspot.com/4538114/diff/5001/src/ibusbus.h#newcode162
> src/ibusbus.h:162: * @retval: A guint value returned from ibus-daemon.
> On 2011/06/06 05:57:14, fujiwara wrote:
> > Could you please add "(out):" below for gobject-introspection?
> > g-i doesn't have the pointer for non-classes so the "out" is needed to
change
> > the arguments to return values.
> > 
> > + * @retval: (out): A guint value returned from ibus-daemon.
> 
> Done.
> 
> http://codereview.appspot.com/4538114/diff/5001/src/ibusbus.h#newcode211
> src/ibusbus.h:211: * @retval: A guint value returned from ibus-daemon.
> On 2011/06/06 05:57:14, fujiwara wrote:
> > + * @retval: (out): A guint value returned from ibus-daemon.
> 
> Done.
> 
> http://codereview.appspot.com/4538114/diff/5001/src/ibusbus.h#newcode275
> src/ibusbus.h:275: * @retval: %TRUE if the name has owner, %FALSE otherwise.
> On 2011/06/06 05:57:14, fujiwara wrote:
> > Same.
> 
> Done.
> 
> http://codereview.appspot.com/4538114/diff/5001/src/ibusbus.h#newcode753
> src/ibusbus.h:753: * @retval: TRUE if "use_sys_layout" option is enabled.
> On 2011/06/06 05:57:14, fujiwara wrote:
> > Same.
> 
> Done.
> 
> http://codereview.appspot.com/4538114/diff/5001/src/ibusbus.h#newcode799
> src/ibusbus.h:799: * @retval: %TRUE if "use_global_engine" option is enabled.
> On 2011/06/06 05:57:14, fujiwara wrote:
> > Same.
> 
> Done.
> 
> http://codereview.appspot.com/4538114/diff/5001/src/ibusbus.h#newcode845
> src/ibusbus.h:845: * @retval: %TRUE if the current global engine is enabled.
> On 2011/06/06 05:57:14, fujiwara wrote:
> > Same.
> 
> Done.

[Yusuke Sato, 2011-06-07 14:51:10]

Ok, then I'll abandon the CL.

Probably we should fix ibusinputcontext APIs as well? At least the following two
functions should have the same problem:

ibus_input_context_process_key_event_async_finish
ibus_input_context_is_enabled_async_finish



On 2011/06/07 14:41:24, Peng wrote:
> Hi Yusuke,
> 
> I wrote a simple python code to test gobject-introspection
> (https://github.com/phuang/ibus/blob/test-gi/src/tests/ibus-gi.py
> ).
> 
> And I got a following outputs from the script:
> ===== With this CL =====
> LD_LIBRARY_PATH=../.libs/ GI_TYPELIB_PATH=.. ./ibus-gi.py
> Failure: Operation was cancelled
> Success: (True, False)
> 
> ===== Without this CL =====
> $ LD_LIBRARY_PATH=../.libs/ GI_TYPELIB_PATH=.. ./ibus-gi.py
> Failure: Operation was cancelled
> Success: False
> 
> From the test script, you may find the function will return two values or
raise
> an Exception. I think it is not script languages friendly. The g-i always
check
> the GError returned from functions. If the GError is returned, g-i will raise
an
> Exception instead of returning values. And real return value is not used
> totally. With this CL, script languages will get two return values (one of
them
> is totally useless, it is always true), if the function is success. That may
> confuse script developers.
> 
> I suggest keeping the current signature, and improving the API document. So it
> will not be a problem for c/c++ developers who should know low level things
> well.
> 
> On 2011/06/07 06:38:31, Yusuke Sato wrote:
> > Fixed the header following Fujiwara-san's comments.
> > 
> > > Do you think we could use this way to resolve the problem?
> > 
> > I think we could, but I'd rather prefer the current way (i.e. the way I'm
> > proposing in this CL) because the GIO's way looks more error-prone. I think
a
> > caller can easily forget to check the error object especially when calling a
> > boolean function such as ibus_bus_get_use_sys_layout_async_finish.
> > 
> > http://codereview.appspot.com/4538114/diff/5001/src/ibusbus.h
> > File src/ibusbus.h (right):
> > 
> > http://codereview.appspot.com/4538114/diff/5001/src/ibusbus.h#newcode162
> > src/ibusbus.h:162: * @retval: A guint value returned from ibus-daemon.
> > On 2011/06/06 05:57:14, fujiwara wrote:
> > > Could you please add "(out):" below for gobject-introspection?
> > > g-i doesn't have the pointer for non-classes so the "out" is needed to
> change
> > > the arguments to return values.
> > > 
> > > + * @retval: (out): A guint value returned from ibus-daemon.
> > 
> > Done.
> > 
> > http://codereview.appspot.com/4538114/diff/5001/src/ibusbus.h#newcode211
> > src/ibusbus.h:211: * @retval: A guint value returned from ibus-daemon.
> > On 2011/06/06 05:57:14, fujiwara wrote:
> > > + * @retval: (out): A guint value returned from ibus-daemon.
> > 
> > Done.
> > 
> > http://codereview.appspot.com/4538114/diff/5001/src/ibusbus.h#newcode275
> > src/ibusbus.h:275: * @retval: %TRUE if the name has owner, %FALSE otherwise.
> > On 2011/06/06 05:57:14, fujiwara wrote:
> > > Same.
> > 
> > Done.
> > 
> > http://codereview.appspot.com/4538114/diff/5001/src/ibusbus.h#newcode753
> > src/ibusbus.h:753: * @retval: TRUE if "use_sys_layout" option is enabled.
> > On 2011/06/06 05:57:14, fujiwara wrote:
> > > Same.
> > 
> > Done.
> > 
> > http://codereview.appspot.com/4538114/diff/5001/src/ibusbus.h#newcode799
> > src/ibusbus.h:799: * @retval: %TRUE if "use_global_engine" option is
enabled.
> > On 2011/06/06 05:57:14, fujiwara wrote:
> > > Same.
> > 
> > Done.
> > 
> > http://codereview.appspot.com/4538114/diff/5001/src/ibusbus.h#newcode845
> > src/ibusbus.h:845: * @retval: %TRUE if the current global engine is enabled.
> > On 2011/06/06 05:57:14, fujiwara wrote:
> > > Same.
> > 
> > Done.

[Peng, 2011-06-07 15:42:06]

On 2011/06/07 14:51:10, Yusuke Sato wrote:
> Ok, then I'll abandon the CL.
> 
> Probably we should fix ibusinputcontext APIs as well? At least the following
two
> functions should have the same problem:
> 
> ibus_input_context_process_key_event_async_finish
> ibus_input_context_is_enabled_async_finish
> 

Right. I could take care of those functions. Thanks.

> 
> 
> On 2011/06/07 14:41:24, Peng wrote:
> > Hi Yusuke,
> > 
> > I wrote a simple python code to test gobject-introspection
> > (https://github.com/phuang/ibus/blob/test-gi/src/tests/ibus-gi.py
> > ).
> > 
> > And I got a following outputs from the script:
> > ===== With this CL =====
> > LD_LIBRARY_PATH=../.libs/ GI_TYPELIB_PATH=.. ./ibus-gi.py
> > Failure: Operation was cancelled
> > Success: (True, False)
> > 
> > ===== Without this CL =====
> > $ LD_LIBRARY_PATH=../.libs/ GI_TYPELIB_PATH=.. ./ibus-gi.py
> > Failure: Operation was cancelled
> > Success: False
> > 
> > From the test script, you may find the function will return two values or
> raise
> > an Exception. I think it is not script languages friendly. The g-i always
> check
> > the GError returned from functions. If the GError is returned, g-i will
raise
> an
> > Exception instead of returning values. And real return value is not used
> > totally. With this CL, script languages will get two return values (one of
> them
> > is totally useless, it is always true), if the function is success. That may
> > confuse script developers.
> > 
> > I suggest keeping the current signature, and improving the API document. So
it
> > will not be a problem for c/c++ developers who should know low level things
> > well.
> > 
> > On 2011/06/07 06:38:31, Yusuke Sato wrote:
> > > Fixed the header following Fujiwara-san's comments.
> > > 
> > > > Do you think we could use this way to resolve the problem?
> > > 
> > > I think we could, but I'd rather prefer the current way (i.e. the way I'm
> > > proposing in this CL) because the GIO's way looks more error-prone. I
think
> a
> > > caller can easily forget to check the error object especially when calling
a
> > > boolean function such as ibus_bus_get_use_sys_layout_async_finish.
> > > 
> > > http://codereview.appspot.com/4538114/diff/5001/src/ibusbus.h
> > > File src/ibusbus.h (right):
> > > 
> > > http://codereview.appspot.com/4538114/diff/5001/src/ibusbus.h#newcode162
> > > src/ibusbus.h:162: * @retval: A guint value returned from ibus-daemon.
> > > On 2011/06/06 05:57:14, fujiwara wrote:
> > > > Could you please add "(out):" below for gobject-introspection?
> > > > g-i doesn't have the pointer for non-classes so the "out" is needed to
> > change
> > > > the arguments to return values.
> > > > 
> > > > + * @retval: (out): A guint value returned from ibus-daemon.
> > > 
> > > Done.
> > > 
> > > http://codereview.appspot.com/4538114/diff/5001/src/ibusbus.h#newcode211
> > > src/ibusbus.h:211: * @retval: A guint value returned from ibus-daemon.
> > > On 2011/06/06 05:57:14, fujiwara wrote:
> > > > + * @retval: (out): A guint value returned from ibus-daemon.
> > > 
> > > Done.
> > > 
> > > http://codereview.appspot.com/4538114/diff/5001/src/ibusbus.h#newcode275
> > > src/ibusbus.h:275: * @retval: %TRUE if the name has owner, %FALSE
otherwise.
> > > On 2011/06/06 05:57:14, fujiwara wrote:
> > > > Same.
> > > 
> > > Done.
> > > 
> > > http://codereview.appspot.com/4538114/diff/5001/src/ibusbus.h#newcode753
> > > src/ibusbus.h:753: * @retval: TRUE if "use_sys_layout" option is enabled.
> > > On 2011/06/06 05:57:14, fujiwara wrote:
> > > > Same.
> > > 
> > > Done.
> > > 
> > > http://codereview.appspot.com/4538114/diff/5001/src/ibusbus.h#newcode799
> > > src/ibusbus.h:799: * @retval: %TRUE if "use_global_engine" option is
> enabled.
> > > On 2011/06/06 05:57:14, fujiwara wrote:
> > > > Same.
> > > 
> > > Done.
> > > 
> > > http://codereview.appspot.com/4538114/diff/5001/src/ibusbus.h#newcode845
> > > src/ibusbus.h:845: * @retval: %TRUE if the current global engine is
enabled.
> > > On 2011/06/06 05:57:14, fujiwara wrote:
> > > > Same.
> > > 
> > > Done.