Issue 4185041: Implement async version of ibus_config_set_value. (Closed)
|
Created:
13 years, 9 months ago by Yusuke Sato Modified:
13 years, 9 months ago Reviewers:
shawn.p.huang, Peng Huang CC:
zork, satorux Base URL:
git@github.com:ibus/ibus.git@master Visibility:
Public. |
DescriptionImplement async version of ibus_config_set_value.
BUG=crosbug.com/11903
Patch Set 1 #
Total comments: 6
Patch Set 2 : review fix #Patch Set 3 : review fix #MessagesTotal messages: 9
(src/ibusconfig.h already has function prototypes for these APIs.)
Sign in to reply to this message.
http://codereview.appspot.com/4185041/diff/1/src/ibusconfig.c File src/ibusconfig.c (right): http://codereview.appspot.com/4185041/diff/1/src/ibusconfig.c#newcode276 src/ibusconfig.c:276: ibus_config_set_value_async (IBusConfig *config, Personally, I would like to provide async version functions for all APIs, and unified them like: ibus_xxx(...) ibus_xxx_finish(...) ibus_xxx_sync(...) But it may break some APIs. What do you think? http://codereview.appspot.com/4185041/diff/1/src/ibusconfig.c#newcode294 src/ibusconfig.c:294: -1, /* timeout */ It is better to add an argument to allow caller specify a timeout value.
Sign in to reply to this message.
http://codereview.appspot.com/4185041/diff/1/src/ibusconfig.c File src/ibusconfig.c (right): http://codereview.appspot.com/4185041/diff/1/src/ibusconfig.c#newcode276 src/ibusconfig.c:276: ibus_config_set_value_async (IBusConfig *config, short answer: I'm fine. Will upload a new patch set shortly. I think most APIs in ibusinputcontext.c and ibusbus.c (for example) are already asynchronous, but they don't use callbacks. Do you mean we should change all the async functions' signatures in ibusinputcontext.c, ibusbus.c, ... so that they could take timeout/GCancellable/GAsyncReadyCallback/user_data parameters? I'm fine with your proposal since its impact for Chrome OS should be very limited, but it looks like a big API change. When it'll be implemented (and who will do it)? On 2011/02/10 20:35:21, Shawn.P.Huang wrote: > Personally, I would like to provide async version functions for all APIs, and > unified them like: > > ibus_xxx(...) > ibus_xxx_finish(...) > ibus_xxx_sync(...) > > But it may break some APIs. What do you think?
Sign in to reply to this message.
Done. Please take another look. On 2011/02/14 01:31:21, Yusuke Sato wrote: > http://codereview.appspot.com/4185041/diff/1/src/ibusconfig.c > File src/ibusconfig.c (right): > > http://codereview.appspot.com/4185041/diff/1/src/ibusconfig.c#newcode276 > src/ibusconfig.c:276: ibus_config_set_value_async (IBusConfig *config, > short answer: I'm fine. Will upload a new patch set shortly. > > I think most APIs in ibusinputcontext.c and ibusbus.c (for example) are already > asynchronous, but they don't use callbacks. Do you mean we should change all the > async functions' signatures in ibusinputcontext.c, ibusbus.c, ... so that they > could take timeout/GCancellable/GAsyncReadyCallback/user_data parameters? I'm > fine with your proposal since its impact for Chrome OS should be very limited, > but it looks like a big API change. When it'll be implemented (and who will do > it)? > > > On 2011/02/10 20:35:21, Shawn.P.Huang wrote: > > Personally, I would like to provide async version functions for all APIs, and > > unified them like: > > > > ibus_xxx(...) > > ibus_xxx_finish(...) > > ibus_xxx_sync(...) > > > > But it may break some APIs. What do you think?
Sign in to reply to this message.
ah maybe it's better to use g_simple_async_result_new explicitly? reworking... On 2011/02/14 04:35:47, Yusuke Sato wrote: > Done. Please take another look. > > On 2011/02/14 01:31:21, Yusuke Sato wrote: > > http://codereview.appspot.com/4185041/diff/1/src/ibusconfig.c > > File src/ibusconfig.c (right): > > > > http://codereview.appspot.com/4185041/diff/1/src/ibusconfig.c#newcode276 > > src/ibusconfig.c:276: ibus_config_set_value_async (IBusConfig *config, > > short answer: I'm fine. Will upload a new patch set shortly. > > > > I think most APIs in ibusinputcontext.c and ibusbus.c (for example) are > already > > asynchronous, but they don't use callbacks. Do you mean we should change all > the > > async functions' signatures in ibusinputcontext.c, ibusbus.c, ... so that they > > could take timeout/GCancellable/GAsyncReadyCallback/user_data parameters? I'm > > fine with your proposal since its impact for Chrome OS should be very limited, > > but it looks like a big API change. When it'll be implemented (and who will do > > it)? > > > > > > On 2011/02/10 20:35:21, Shawn.P.Huang wrote: > > > Personally, I would like to provide async version functions for all APIs, > and > > > unified them like: > > > > > > ibus_xxx(...) > > > ibus_xxx_finish(...) > > > ibus_xxx_sync(...) > > > > > > But it may break some APIs. What do you think?
Sign in to reply to this message.
http://codereview.appspot.com/4185041/diff/1/src/ibusconfig.c File src/ibusconfig.c (right): http://codereview.appspot.com/4185041/diff/1/src/ibusconfig.c#newcode276 src/ibusconfig.c:276: ibus_config_set_value_async (IBusConfig *config, On 2011/02/14 01:31:21, Yusuke Sato wrote: > short answer: I'm fine. Will upload a new patch set shortly. > > I think most APIs in ibusinputcontext.c and ibusbus.c (for example) are already > asynchronous, but they don't use callbacks. Do you mean we should change all the > async functions' signatures in ibusinputcontext.c, ibusbus.c, ... so that they > could take timeout/GCancellable/GAsyncReadyCallback/user_data parameters? I'm > fine with your proposal since its impact for Chrome OS should be very limited, > but it looks like a big API change. When it'll be implemented (and who will do > it)? > > > On 2011/02/10 20:35:21, Shawn.P.Huang wrote: > > Personally, I would like to provide async version functions for all APIs, and > > unified them like: > > > > ibus_xxx(...) > > ibus_xxx_finish(...) > > ibus_xxx_sync(...) > > > > But it may break some APIs. What do you think? > Maybe breaking API is not a good idea. It need modify most IMEs. I think we have to use functions like: ibus_xxx ibus_xxx_async ibus_xxx_async_finish Sorry for the previous comment. The first patch set is OK for me. except the timeout argument.
Sign in to reply to this message.
PTAL. http://codereview.appspot.com/4185041/diff/1/src/ibusconfig.c File src/ibusconfig.c (right): http://codereview.appspot.com/4185041/diff/1/src/ibusconfig.c#newcode276 src/ibusconfig.c:276: ibus_config_set_value_async (IBusConfig *config, np. Please review the patchset #3. On 2011/02/15 04:14:29, Shawn.P.Huang wrote: > On 2011/02/14 01:31:21, Yusuke Sato wrote: > > short answer: I'm fine. Will upload a new patch set shortly. > > > > I think most APIs in ibusinputcontext.c and ibusbus.c (for example) are > already > > asynchronous, but they don't use callbacks. Do you mean we should change all > the > > async functions' signatures in ibusinputcontext.c, ibusbus.c, ... so that they > > could take timeout/GCancellable/GAsyncReadyCallback/user_data parameters? I'm > > fine with your proposal since its impact for Chrome OS should be very limited, > > but it looks like a big API change. When it'll be implemented (and who will do > > it)? > > > > > > On 2011/02/10 20:35:21, Shawn.P.Huang wrote: > > > Personally, I would like to provide async version functions for all APIs, > and > > > unified them like: > > > > > > ibus_xxx(...) > > > ibus_xxx_finish(...) > > > ibus_xxx_sync(...) > > > > > > But it may break some APIs. What do you think? > > > > Maybe breaking API is not a good idea. It need modify most IMEs. > > I think we have to use functions like: > ibus_xxx > ibus_xxx_async > ibus_xxx_async_finish > > Sorry for the previous comment. The first patch set is OK for me. except the > timeout argument. http://codereview.appspot.com/4185041/diff/1/src/ibusconfig.c#newcode294 src/ibusconfig.c:294: -1, /* timeout */ On 2011/02/10 20:35:21, Shawn.P.Huang wrote: > It is better to add an argument to allow caller specify a timeout value. Done.
Sign in to reply to this message.
Satoru: I've added comments about ownership of floating GVariables. On 2011/02/15 09:11:41, Yusuke Sato wrote: > PTAL. > > http://codereview.appspot.com/4185041/diff/1/src/ibusconfig.c > File src/ibusconfig.c (right): > > http://codereview.appspot.com/4185041/diff/1/src/ibusconfig.c#newcode276 > src/ibusconfig.c:276: ibus_config_set_value_async (IBusConfig *config, > np. Please review the patchset #3. > > On 2011/02/15 04:14:29, Shawn.P.Huang wrote: > > On 2011/02/14 01:31:21, Yusuke Sato wrote: > > > short answer: I'm fine. Will upload a new patch set shortly. > > > > > > I think most APIs in ibusinputcontext.c and ibusbus.c (for example) are > > already > > > asynchronous, but they don't use callbacks. Do you mean we should change all > > the > > > async functions' signatures in ibusinputcontext.c, ibusbus.c, ... so that > they > > > could take timeout/GCancellable/GAsyncReadyCallback/user_data parameters? > I'm > > > fine with your proposal since its impact for Chrome OS should be very > limited, > > > but it looks like a big API change. When it'll be implemented (and who will > do > > > it)? > > > > > > > > > On 2011/02/10 20:35:21, Shawn.P.Huang wrote: > > > > Personally, I would like to provide async version functions for all APIs, > > and > > > > unified them like: > > > > > > > > ibus_xxx(...) > > > > ibus_xxx_finish(...) > > > > ibus_xxx_sync(...) > > > > > > > > But it may break some APIs. What do you think? > > > > > > > Maybe breaking API is not a good idea. It need modify most IMEs. > > > > I think we have to use functions like: > > ibus_xxx > > ibus_xxx_async > > ibus_xxx_async_finish > > > > Sorry for the previous comment. The first patch set is OK for me. except the > > timeout argument. > > http://codereview.appspot.com/4185041/diff/1/src/ibusconfig.c#newcode294 > src/ibusconfig.c:294: -1, /* timeout */ > On 2011/02/10 20:35:21, Shawn.P.Huang wrote: > > It is better to add an argument to allow caller specify a timeout value. > > Done.
Sign in to reply to this message.
|
