Issue 6988047: client: Queue events while the IBus context isn't ready (Closed)
|
Created:
11 years, 6 months ago by TiagoMatos Modified:
11 years, 5 months ago Reviewers:
shawn.p.huang, fujiwara Base URL:
git://github.com/ibus/ibus.git@master Visibility:
Public. |
Descriptionclient: Queue events while the IBus context isn't ready
There are actually 3 patches here.
---
client: Queue events while the IBus context isn't ready
We may lose events that ought to be processed while the IBus context
isn't ready or if the connection to IBus isn't fully established yet.
To avoid that, enqueue events to be processed later when the IBus
context creation finishes.
---
client: Don't cancel an ongoing create input context on another request
This would only add more delays.
---
client: Cancel any ongoing create input context request on finalize
BUG=
Patch Set 1 #
Total comments: 22
Patch Set 2 : v2 #Patch Set 3 : v3 #Patch Set 4 : v4 #
Total comments: 1
MessagesTotal messages: 11
Please review, thanks.
Sign in to reply to this message.
On 2013/01/07 09:32:01, TiagoMatos wrote: > Please review, thanks. Looks good.
Sign in to reply to this message.
https://codereview.appspot.com/6988047/diff/1/client/gtk2/ibusimcontext.c File client/gtk2/ibusimcontext.c (left): https://codereview.appspot.com/6988047/diff/1/client/gtk2/ibusimcontext.c#old... client/gtk2/ibusimcontext.c:1498: /* Cancel previous create input context request */ I remember those code is for below use case. Maybe keeping them is safer. 1. ibusimcontext issue a request for creating ibuscontext. 2. ibusimcontext is waiting for it ready. 3. ibus-daemon is restarted. 4. ibusimcontext cancels the previous request. 5. goto step 1 https://codereview.appspot.com/6988047/diff/1/client/gtk2/ibusimcontext.c File client/gtk2/ibusimcontext.c (right): https://codereview.appspot.com/6988047/diff/1/client/gtk2/ibusimcontext.c#new... client/gtk2/ibusimcontext.c:279: event->hardware_keycode - 8, For new added code, we would like to limit every line in 80 chars. So please reformat it. https://codereview.appspot.com/6988047/diff/1/client/gtk2/ibusimcontext.c#new... client/gtk2/ibusimcontext.c:290: gdk_event_copy ((GdkEvent *) event)); ditto https://codereview.appspot.com/6988047/diff/1/client/gtk2/ibusimcontext.c#new... client/gtk2/ibusimcontext.c:690: g_queue_free_full (ibusimcontext->events_queue, (GDestroyNotify)gdk_event_free); ditto https://codereview.appspot.com/6988047/diff/1/client/gtk2/ibusimcontext.c#new... client/gtk2/ibusimcontext.c:706: if (!ibusimcontext->has_focus) Is it possible ibusimcontext be NULL here? https://codereview.appspot.com/6988047/diff/1/client/gtk2/ibusimcontext.c#new... client/gtk2/ibusimcontext.c:717: gtk_im_context_set_client_window ((GtkIMContext *)ibusimcontext, event->window); ditto https://codereview.appspot.com/6988047/diff/1/client/gtk2/ibusimcontext.c#new... client/gtk2/ibusimcontext.c:734: g_return_val_if_fail (ibusimcontext->cancellable != NULL || !ibus_bus_is_connected (_bus), ditto https://codereview.appspot.com/6988047/diff/1/client/gtk2/ibusimcontext.c#new... client/gtk2/ibusimcontext.c:734: g_return_val_if_fail (ibusimcontext->cancellable != NULL || !ibus_bus_is_connected (_bus), What about the app is using the ibusimmodule, but the ibus-daemon is disabled? I think in that case, ibusimcontext should not queue the key events, right? https://codereview.appspot.com/6988047/diff/1/client/gtk2/ibusimcontext.c#new... client/gtk2/ibusimcontext.c:736: g_queue_push_head (ibusimcontext->events_queue, gdk_event_copy ((GdkEvent *)event)); ditto https://codereview.appspot.com/6988047/diff/1/client/gtk2/ibusimcontext.c#new... client/gtk2/ibusimcontext.c:736: g_queue_push_head (ibusimcontext->events_queue, gdk_event_copy ((GdkEvent *)event)); Any reason for pushing events at the head of the queue, and popping them from tail of the queue? I suggest pushing them at the tail, and popping them from the head. It is a little clearer for me. https://codereview.appspot.com/6988047/diff/1/client/gtk2/ibusimcontext.c#new... client/gtk2/ibusimcontext.c:1484: while ((event = g_queue_pop_tail (ibusimcontext->events_queue)) != NULL) { ditto
Sign in to reply to this message.
https://codereview.appspot.com/6988047/diff/1/client/gtk2/ibusimcontext.c File client/gtk2/ibusimcontext.c (left): https://codereview.appspot.com/6988047/diff/1/client/gtk2/ibusimcontext.c#old... client/gtk2/ibusimcontext.c:1498: /* Cancel previous create input context request */ On 2013/01/07 15:34:18, Peng wrote: > I remember those code is for below use case. Maybe keeping them is safer. > 1. ibusimcontext issue a request for creating ibuscontext. > 2. ibusimcontext is waiting for it ready. > 3. ibus-daemon is restarted. At this point _create_input_context_done() should be called synchronously because the underlying GDBusConnection gets closed. That will unref the cancellable and set it to NULL. Then, when the connection is connected again, _create_input_context() is called but the cancellable is already NULL so we go ahead and create a new one and request a new input context asynchronously. > 4. ibusimcontext cancels the previous request. I don't see any explicit cancellations. Am I missing something? Anyway, I don't feel strongly about this at all so it can be dropped, it's even a separate commit. https://codereview.appspot.com/6988047/diff/1/client/gtk2/ibusimcontext.c File client/gtk2/ibusimcontext.c (right): https://codereview.appspot.com/6988047/diff/1/client/gtk2/ibusimcontext.c#new... client/gtk2/ibusimcontext.c:279: event->hardware_keycode - 8, On 2013/01/07 15:34:18, Peng wrote: > For new added code, we would like to limit every line in 80 chars. So please > reformat it. Really, 80 char lines in 2013? :-) I think that none of the lines added in this patch go over 100 chars which is quite reasonable and reformatting this to keep in under 80 would look bad IMHO. https://codereview.appspot.com/6988047/diff/1/client/gtk2/ibusimcontext.c#new... client/gtk2/ibusimcontext.c:706: if (!ibusimcontext->has_focus) On 2013/01/07 15:34:18, Peng wrote: > Is it possible ibusimcontext be NULL here? That would be a bug in gtk+ I think. The code that was there previously didn't check for that either FWIW. https://codereview.appspot.com/6988047/diff/1/client/gtk2/ibusimcontext.c#new... client/gtk2/ibusimcontext.c:706: if (!ibusimcontext->has_focus) On 2013/01/07 15:34:18, Peng wrote: > Is it possible ibusimcontext be NULL here? That would be a bug in gtk+. https://codereview.appspot.com/6988047/diff/1/client/gtk2/ibusimcontext.c#new... client/gtk2/ibusimcontext.c:734: g_return_val_if_fail (ibusimcontext->cancellable != NULL || !ibus_bus_is_connected (_bus), On 2013/01/07 15:34:18, Peng wrote: > What about the app is using the ibusimmodule, but the ibus-daemon is disabled? I > think in that case, ibusimcontext should not queue the key events, right? Good point. I think that using the module and not having ibus-daemon running should be considered a bug. I now added a limit for the queue to grow and will start discarding events and print warnings in that case. https://codereview.appspot.com/6988047/diff/1/client/gtk2/ibusimcontext.c#new... client/gtk2/ibusimcontext.c:736: g_queue_push_head (ibusimcontext->events_queue, gdk_event_copy ((GdkEvent *)event)); On 2013/01/07 15:34:18, Peng wrote: > Any reason for pushing events at the head of the queue, and popping them from > tail of the queue? > I suggest pushing them at the tail, and popping them from the head. It is a > little clearer for me. Done. https://codereview.appspot.com/6988047/diff/1/client/gtk2/ibusimcontext.c#new... client/gtk2/ibusimcontext.c:1484: while ((event = g_queue_pop_tail (ibusimcontext->events_queue)) != NULL) { On 2013/01/07 15:34:18, Peng wrote: > ditto Done.
Sign in to reply to this message.
I also pushed the 3 separate patches to github: https://github.com/rtcm/ibus/commits/master .
Sign in to reply to this message.
https://codereview.appspot.com/6988047/diff/1/client/gtk2/ibusimcontext.c File client/gtk2/ibusimcontext.c (right): https://codereview.appspot.com/6988047/diff/1/client/gtk2/ibusimcontext.c#new... client/gtk2/ibusimcontext.c:279: event->hardware_keycode - 8, On 2013/01/08 14:37:53, TiagoMatos wrote: > On 2013/01/07 15:34:18, Peng wrote: > > For new added code, we would like to limit every line in 80 chars. So please > > reformat it. > > Really, 80 char lines in 2013? :-) > > I think that none of the lines added in this patch go over 100 chars which is > quite reasonable and reformatting this to keep in under 80 would look bad IMHO. I know sometimes it looks stupid. But it is good for some editors and tools. like the online review tool. That why we obey it for the new added code. So please reformat it. Thanks. https://codereview.appspot.com/6988047/diff/1/client/gtk2/ibusimcontext.c#new... client/gtk2/ibusimcontext.c:706: if (!ibusimcontext->has_focus) On 2013/01/08 14:37:53, TiagoMatos wrote: > On 2013/01/07 15:34:18, Peng wrote: > > Is it possible ibusimcontext be NULL here? > > That would be a bug in gtk+. Sorry. My mistake. https://codereview.appspot.com/6988047/diff/1/client/gtk2/ibusimcontext.c#new... client/gtk2/ibusimcontext.c:734: g_return_val_if_fail (ibusimcontext->cancellable != NULL || !ibus_bus_is_connected (_bus), On 2013/01/08 14:37:53, TiagoMatos wrote: > On 2013/01/07 15:34:18, Peng wrote: > > What about the app is using the ibusimmodule, but the ibus-daemon is disabled? > I > > think in that case, ibusimcontext should not queue the key events, right? > > Good point. I think that using the module and not having ibus-daemon running > should be considered a bug. I now added a limit for the queue to grow and will > start discarding events and print warnings in that case. I am not totally agree with you. It is a very common use case (maybe be not for gnome 3). A user would to shutdown the ibus-daemon for a while, maybe for debugging the ibus-daemon or engines. It is also possible for a kde user, he set the GTK_IMMMODULE=ibus, but he does not start the ibus-daemon. In those cases, input for gtk apps will not work. I think it is not very good. I think we should only queue events, when ibus-daemon is running, but the connection or ibuscontext is not ready.
Sign in to reply to this message.
On 2013/01/08 16:14:02, Peng wrote: > I know sometimes it looks stupid. But it is good for some editors and tools. > like the online review tool. That why we obey it for the new added code. So > please reformat it. Thanks. Ok, done. > > Good point. I think that using the module and not having ibus-daemon running > > should be considered a bug. I now added a limit for the queue to grow and will > > start discarding events and print warnings in that case. > > I am not totally agree with you. It is a very common use case (maybe be not for > gnome 3). A user would to shutdown the ibus-daemon for a while, maybe for > debugging the ibus-daemon or engines. > > It is also possible for a kde user, he set the GTK_IMMMODULE=ibus, but he does > not start the ibus-daemon. > > In those cases, input for gtk apps will not work. I think it is not very good. I > think we should only queue events, when ibus-daemon is running, but the > connection or ibuscontext is not ready. Fair enough, let me know what you think of this solution now.
Sign in to reply to this message.
lgtm with a comment https://codereview.appspot.com/6988047/diff/1/client/gtk2/ibusimcontext.c File client/gtk2/ibusimcontext.c (left): https://codereview.appspot.com/6988047/diff/1/client/gtk2/ibusimcontext.c#old... client/gtk2/ibusimcontext.c:1498: /* Cancel previous create input context request */ On 2013/01/08 14:37:53, TiagoMatos wrote: > On 2013/01/07 15:34:18, Peng wrote: > > I remember those code is for below use case. Maybe keeping them is safer. > > 1. ibusimcontext issue a request for creating ibuscontext. > > 2. ibusimcontext is waiting for it ready. > > 3. ibus-daemon is restarted. > > At this point _create_input_context_done() should be called synchronously > because the underlying GDBusConnection gets closed. That will unref the > cancellable and set it to NULL. > > Then, when the connection is connected again, _create_input_context() is called > but the cancellable is already NULL so we go ahead and create a new one and > request a new input context asynchronously. > > > 4. ibusimcontext cancels the previous request. > > I don't see any explicit cancellations. Am I missing something? > > Anyway, I don't feel strongly about this at all so it can be dropped, it's even > a separate commit. Just read the code and refresh my memory. I think this function should not be called if cancellable != NULL. So I think it is better to put g_return_if_fail() here instead of just return without any warning messages. right? So we can find some weird cases and fix them.
Sign in to reply to this message.
On 2013/01/09 02:21:23, Peng wrote: > Just read the code and refresh my memory. I think this function should not be > called if cancellable != NULL. > So I think it is better to put g_return_if_fail() here instead of just return > without any warning messages. right? So we can find some weird cases and fix > them. Yes, I agree.
Sign in to reply to this message.
On 2013/01/09 13:53:53, TiagoMatos wrote: > On 2013/01/09 02:21:23, Peng wrote: > > Just read the code and refresh my memory. I think this function should not be > > called if cancellable != NULL. > > So I think it is better to put g_return_if_fail() here instead of just return > > without any warning messages. right? So we can find some weird cases and fix > > them. > > Yes, I agree. Landed it. Please close this CL. Thanks.
Sign in to reply to this message.
Message was sent while issue was closed.
https://codereview.appspot.com/6988047/diff/9002/client/gtk2/ibusimcontext.c File client/gtk2/ibusimcontext.c (right): https://codereview.appspot.com/6988047/diff/9002/client/gtk2/ibusimcontext.c#... client/gtk2/ibusimcontext.c:736: if (!_daemon_is_running) Sorry for adding a comment to the closed CL, but the client still seems to lose the very first key press event because of the race of g_bus_watch_name callbacks and filter_keypress.
Sign in to reply to this message.
|
