Code review - Issue 4275048: Add asynchronous InputContext.IsEnabled and InputContext.GetEngine APIs.https://codereview.appspot.com/2011-03-21T05:13:32+00:00rietveld
Message from unknown
2011-03-14T07:05:57+00:00Yusuke Satourn:md5:1fe1e5da9a1f1f96bfe5df4685f664d0
Message from yusukes@chromium.org
2011-03-14T07:09:23+00:00Yusuke Satourn:md5:cf861d3ccd71c654eaaab0a67801a110
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.
Message from Shawn.P.Huang@gmail.com
2011-03-14T13:40:46+00:00Pengurn:md5:4cda6e4c567b429d67356304a88d12b7
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#newcode647
src/ibusinputcontext.c:647: if (error != NULL && error->message != NULL) {
Is it necessary?
http://codereview.appspot.com/4275048/diff/2001/src/ibusinputcontext.c#newcode845
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#newcode878
src/ibusinputcontext.c:878: if (error != NULL && error->message != NULL) {
Is it necessary?
http://codereview.appspot.com/4275048/diff/2001/src/ibusinputcontext.c#newcode949
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#newcode1
src/tests/ibus-inputcontext.c:1: /* -*- mode: C; c-basic-offset: 4; indent-tabs-mode: nil; -*- */
It is better to add the license header.
Message from unknown
2011-03-16T15:57:59+00:00Yusuke Satourn:md5:f5e5ad908061ee92c48c20354b8fc0f2
Message from unknown
2011-03-16T16:00:54+00:00Yusuke Satourn:md5:aa877cccea20a137132ab5f68fcd0eb2
Message from yusukes@chromium.org
2011-03-18T14:08:01+00:00Yusuke Satourn:md5:51d243e6bec2e842030cea2db251e917
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#newcode647
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#newcode845
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#newcode878
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#newcode949
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#newcode1
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.
Message from Shawn.P.Huang@gmail.com
2011-03-18T15:30:33+00:00Pengurn:md5:1d4544c5411f75c6da377cdd1955f9c0
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#newcode647
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#newcode845
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?
>
Message from yusukes@chromium.org
2011-03-21T05:13:32+00:00Yusuke Satourn:md5:cfc0cadb42f1abb3952940d4f32d73ac
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#newcode647
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#newcode845
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?
> >
>