Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(153)

Issue 4538114: Change signatures of _async_finish functions so the caller of them can handle errors correctly (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 10 months ago by Yusuke Sato
Modified:
2 years, 10 months ago
Reviewers:
shawn.p.huang, Peng Huang, fujiwara
Base URL:
git@github.com:ibus/ibus.git@master
Visibility:
Public.

Description

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

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

Patch Set 1 #

Total comments: 2

Patch Set 2 : review fix #

Total comments: 12

Patch Set 3 : addressed fujiwara's comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+92 lines, -49 lines) Patch
M src/ibusbus.h View 1 2 6 chunks +20 lines, -8 lines 0 comments Download
M src/ibusbus.c View 1 8 chunks +31 lines, -18 lines 0 comments Download
M src/ibusinputcontext.h View 3 chunks +5 lines, -5 lines 0 comments Download
M src/tests/ibus-bus.c View 6 chunks +36 lines, -18 lines 0 comments Download

Messages

Total messages: 16
Yusuke Sato
Fixed ibusbus.h following your comment at http://codereview.appspot.com/4275048/diff/2001/src/ibusinputcontext.c#newcode845 . Sorry for the delay.
2 years, 10 months ago #1
Yusuke Sato
+takao.fujiwara1
2 years, 10 months ago #2
Peng
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) ...
2 years, 10 months ago #3
Yusuke Sato
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 ...
2 years, 10 months ago #4
Peng
LGTM
2 years, 10 months ago #5
Yusuke Sato
Fujiwara-san, this patch changes the signatures of some functions in src/ibusbus.h. Please let me know ...
2 years, 10 months ago #6
Peng
On 2011/06/05 03:52:57, Yusuke Sato wrote: > Fujiwara-san, this patch changes the signatures of some ...
2 years, 10 months ago #7
Yusuke Sato
Could you elaborate a little more? I don't know much about gobject-introspect. On 2011/06/05 20:58:03, ...
2 years, 10 months ago #8
fujiwara
Yes, probably I also think it's nice to work with g-i. I added a comment. ...
2 years, 10 months ago #9
Peng
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 ...
2 years, 10 months ago #10
Peng
Hi Yusuke, I just checked some document of gio (http://developer.gnome.org/gio/2.26/GDataInputStream.html#g-data-input-stream-read-byte). Those functions have similar problems. ...
2 years, 10 months ago #11
Yusuke Sato
Fixed the header following Fujiwara-san's comments. > Do you think we could use this way ...
2 years, 10 months ago #12
fujiwara
Thank you LGTM
2 years, 10 months ago #13
Peng
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 ...
2 years, 10 months ago #14
Yusuke Sato
Ok, then I'll abandon the CL. Probably we should fix ibusinputcontext APIs as well? At ...
2 years, 10 months ago #15
Peng
2 years, 10 months ago #16
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1278:e6ce13d99bf5