Issue 3199042: Focus out/in fake input context if the input window changed. (Closed)
|
Created:
13 years, 7 months ago by Peng Huang Modified:
13 years, 7 months ago CC:
satorux Base URL:
git@github.com:/ibus/ibus.git@chromeos Visibility:
Public. |
DescriptionFocus out/in fake input context if the input window changed.
BUG=chromium-os:8855
TEST=manual
Patch Set 1 #Patch Set 2 : Listen on the focus-{in,out}-event #
Total comments: 10
Patch Set 3 : Fix some issues found in codereview #MessagesTotal messages: 13
I'm not sure if this fix is good enough. If there is only one window when pressing alt+shift+Tab, I don't think the input window will be changed at all. We probably still need to track the focus change.
Sign in to reply to this message.
On 2010/11/19 04:38:45, suzhe wrote: > I'm not sure if this fix is good enough. If there is only one window when > pressing alt+shift+Tab, I don't think the input window will be changed at all. > We probably still need to track the focus change. You are right. But if only one window, the window focus will not be changed. So we don't have a good way to fix it.
Sign in to reply to this message.
On 2010/11/19 04:38:45, suzhe wrote: > I'm not sure if this fix is good enough. If there is only one window when > pressing alt+shift+Tab, I don't think the input window will be changed at all. > We probably still need to track the focus change. According to my test, the window will lost and regain the input focus. Please help verify.
Sign in to reply to this message.
On 2010/11/19 04:52:04, suzhe wrote: > On 2010/11/19 04:38:45, suzhe wrote: > > I'm not sure if this fix is good enough. If there is only one window when > > pressing alt+shift+Tab, I don't think the input window will be changed at all. > > We probably still need to track the focus change. > > According to my test, the window will lost and regain the input focus. Please > help verify. Do you have any idea how to get the focus changing related events from the toplevel window?
Sign in to reply to this message.
On 2010/11/19 05:10:08, Shawn.P.Huang wrote:
> On 2010/11/19 04:52:04, suzhe wrote:
> > On 2010/11/19 04:38:45, suzhe wrote:
> > > I'm not sure if this fix is good enough. If there is only one window when
> > > pressing alt+shift+Tab, I don't think the input window will be changed at
> all.
> > > We probably still need to track the focus change.
> >
> > According to my test, the window will lost and regain the input focus.
Please
> > help verify.
>
> Do you have any idea how to get the focus changing related events from the
> toplevel window?
You can get the widget of im context's client window by using code like:
GtkWidget* GetWidgetFromWindow(GdkWindow* window) {
while(window) {
gpointer data;
gdk_window_get_user_data(window, &data);
if (GTK_IS_WIDGET(data))
return GTK_WIDGET(data);
window = gdk_window_get_parent(window);
}
return NULL;
}
Then get the top level window from this widget. Then you can hook callbacks to
its "focus-in-event" and "focus-out-event" signals. Please be aware of following
issues:
1. Multiple im context may share the same top level window. So it's better to
check if the callbacks are already hooked before hooking them.
2. We only need to use this trick when there is no focused im context.
3. It's probably enough to hook only one of these two signals and focus_out/in
the global im context there.
Sign in to reply to this message.
On 2010/11/19 05:30:03, suzhe wrote:
> On 2010/11/19 05:10:08, Shawn.P.Huang wrote:
> > On 2010/11/19 04:52:04, suzhe wrote:
> > > On 2010/11/19 04:38:45, suzhe wrote:
> > > > I'm not sure if this fix is good enough. If there is only one window
when
> > > > pressing alt+shift+Tab, I don't think the input window will be changed
at
> > all.
> > > > We probably still need to track the focus change.
> > >
> > > According to my test, the window will lost and regain the input focus.
> Please
> > > help verify.
> >
> > Do you have any idea how to get the focus changing related events from the
> > toplevel window?
>
> You can get the widget of im context's client window by using code like:
>
> GtkWidget* GetWidgetFromWindow(GdkWindow* window) {
> while(window) {
> gpointer data;
> gdk_window_get_user_data(window, &data);
> if (GTK_IS_WIDGET(data))
> return GTK_WIDGET(data);
>
> window = gdk_window_get_parent(window);
> }
> return NULL;
> }
>
> Then get the top level window from this widget. Then you can hook callbacks to
> its "focus-in-event" and "focus-out-event" signals. Please be aware of
following
> issues:
> 1. Multiple im context may share the same top level window. So it's better to
> check if the callbacks are already hooked before hooking them.
> 2. We only need to use this trick when there is no focused im context.
> 3. It's probably enough to hook only one of these two signals and focus_out/in
> the global im context there.
How about use gdk_window_add_filter to get the FocusIn/Out events?
Sign in to reply to this message.
On 2010/11/19 05:42:40, Shawn.P.Huang wrote:
> On 2010/11/19 05:30:03, suzhe wrote:
> > On 2010/11/19 05:10:08, Shawn.P.Huang wrote:
> > > On 2010/11/19 04:52:04, suzhe wrote:
> > > > On 2010/11/19 04:38:45, suzhe wrote:
> > > > > I'm not sure if this fix is good enough. If there is only one window
> when
> > > > > pressing alt+shift+Tab, I don't think the input window will be changed
> at
> > > all.
> > > > > We probably still need to track the focus change.
> > > >
> > > > According to my test, the window will lost and regain the input focus.
> > Please
> > > > help verify.
> > >
> > > Do you have any idea how to get the focus changing related events from the
> > > toplevel window?
> >
> > You can get the widget of im context's client window by using code like:
> >
> > GtkWidget* GetWidgetFromWindow(GdkWindow* window) {
> > while(window) {
> > gpointer data;
> > gdk_window_get_user_data(window, &data);
> > if (GTK_IS_WIDGET(data))
> > return GTK_WIDGET(data);
> >
> > window = gdk_window_get_parent(window);
> > }
> > return NULL;
> > }
> >
> > Then get the top level window from this widget. Then you can hook callbacks
to
> > its "focus-in-event" and "focus-out-event" signals. Please be aware of
> following
> > issues:
> > 1. Multiple im context may share the same top level window. So it's better
to
> > check if the callbacks are already hooked before hooking them.
> > 2. We only need to use this trick when there is no focused im context.
> > 3. It's probably enough to hook only one of these two signals and
focus_out/in
> > the global im context there.
>
> How about use gdk_window_add_filter to get the FocusIn/Out events?
I don't know. But be aware that the event received by the filter function is
GdkXEvent, rather than GdkEvent, which is actually a XEvent. And it's a very low
level function which only works on X. So I still think handling "focus-in-event"
of the top level window is better.
Sign in to reply to this message.
On 2010/11/19 05:53:50, suzhe wrote:
> On 2010/11/19 05:42:40, Shawn.P.Huang wrote:
> > On 2010/11/19 05:30:03, suzhe wrote:
> > > On 2010/11/19 05:10:08, Shawn.P.Huang wrote:
> > > > On 2010/11/19 04:52:04, suzhe wrote:
> > > > > On 2010/11/19 04:38:45, suzhe wrote:
> > > > > > I'm not sure if this fix is good enough. If there is only one window
> > when
> > > > > > pressing alt+shift+Tab, I don't think the input window will be
changed
> > at
> > > > all.
> > > > > > We probably still need to track the focus change.
> > > > >
> > > > > According to my test, the window will lost and regain the input focus.
> > > Please
> > > > > help verify.
> > > >
> > > > Do you have any idea how to get the focus changing related events from
the
> > > > toplevel window?
> > >
> > > You can get the widget of im context's client window by using code like:
> > >
> > > GtkWidget* GetWidgetFromWindow(GdkWindow* window) {
> > > while(window) {
> > > gpointer data;
> > > gdk_window_get_user_data(window, &data);
> > > if (GTK_IS_WIDGET(data))
> > > return GTK_WIDGET(data);
> > >
> > > window = gdk_window_get_parent(window);
> > > }
> > > return NULL;
> > > }
> > >
> > > Then get the top level window from this widget. Then you can hook
callbacks
> to
> > > its "focus-in-event" and "focus-out-event" signals. Please be aware of
> > following
> > > issues:
> > > 1. Multiple im context may share the same top level window. So it's better
> to
> > > check if the callbacks are already hooked before hooking them.
> > > 2. We only need to use this trick when there is no focused im context.
> > > 3. It's probably enough to hook only one of these two signals and
> focus_out/in
> > > the global im context there.
> >
> > How about use gdk_window_add_filter to get the FocusIn/Out events?
>
> I don't know. But be aware that the event received by the filter function is
> GdkXEvent, rather than GdkEvent, which is actually a XEvent. And it's a very
low
> level function which only works on X. So I still think handling
"focus-in-event"
> of the top level window is better.
Updated to use focus-in-event/focus-out-event
Sign in to reply to this message.
And I just noticed that this im module may have some other issues, for example, it does not have a class level finalize function to do necessary clean up, which may cause some subtle issue, such as: _fake_context may not be destroyed properly when this im module gets unloaded. http://codereview.appspot.com/3199042/diff/9001/client/gtk2/ibusimcontext.c File client/gtk2/ibusimcontext.c (right): http://codereview.appspot.com/3199042/diff/9001/client/gtk2/ibusimcontext.c#n... client/gtk2/ibusimcontext.c:263: break; is _use_key_snooper set to true on ChromeOS? If not, then this block will only be executed when there is no focused input context, and above statement will always be true. So code below will never be executed. http://codereview.appspot.com/3199042/diff/9001/client/gtk2/ibusimcontext.c#n... client/gtk2/ibusimcontext.c:272: g_object_ref (event->window); If there is no more key event before unloading this im module, this window will be leaked. And I just notice that, this _key_snooper_cb will never be removed, which is not good. http://codereview.appspot.com/3199042/diff/9001/client/gtk2/ibusimcontext.c#n... client/gtk2/ibusimcontext.c:280: GtkWidget *widget = gtk_get_event_widget ((GdkEvent *)event); I'm not sure if this function returns the top level window. http://codereview.appspot.com/3199042/diff/9001/client/gtk2/ibusimcontext.c#n... client/gtk2/ibusimcontext.c:291: NULL); I think it's not necessary to disconnect the handler explicitly. You just need to avoid connecting them more than once. http://codereview.appspot.com/3199042/diff/9001/client/gtk2/ibusimcontext.c#n... client/gtk2/ibusimcontext.c:304: g_object_ref (widget); Same as above event->window case, adding ref here may cause memory leak. More seriously, this may cause problem if the application expects this widget to be destroyed within a certain time.
Sign in to reply to this message.
It is better to clean every thing when class is finalized, maybe it is not a good time to fix it. Because maybe it has some risk. BTW， currently im-ibus.so is resident module. It can not be unloaded, because it calls g_type_module_use in im_module_init (). http://codereview.appspot.com/3199042/diff/9001/client/gtk2/ibusimcontext.c File client/gtk2/ibusimcontext.c (right): http://codereview.appspot.com/3199042/diff/9001/client/gtk2/ibusimcontext.c#n... client/gtk2/ibusimcontext.c:263: break; On 2010/11/19 09:06:27, suzhe wrote: > is _use_key_snooper set to true on ChromeOS? If not, then this block will only > be executed when there is no focused input context, and above statement will > always be true. So code below will never be executed. Done. http://codereview.appspot.com/3199042/diff/9001/client/gtk2/ibusimcontext.c#n... client/gtk2/ibusimcontext.c:272: g_object_ref (event->window); On 2010/11/19 09:06:27, suzhe wrote: > If there is no more key event before unloading this im module, this window will > be leaked. And I just notice that, this _key_snooper_cb will never be removed, > which is not good. Done. http://codereview.appspot.com/3199042/diff/9001/client/gtk2/ibusimcontext.c#n... client/gtk2/ibusimcontext.c:280: GtkWidget *widget = gtk_get_event_widget ((GdkEvent *)event); On 2010/11/19 09:06:27, suzhe wrote: > I'm not sure if this function returns the top level window. It does not. But it returns the widget associated with event->window. Actually, it is the widget has input focus. I think it i we want. http://codereview.appspot.com/3199042/diff/9001/client/gtk2/ibusimcontext.c#n... client/gtk2/ibusimcontext.c:291: NULL); On 2010/11/19 09:06:27, suzhe wrote: > I think it's not necessary to disconnect the handler explicitly. You just need > to avoid connecting them more than once. Disconnecting handlers could make callback function simpler. And we don't need remember which widget has been connected to. http://codereview.appspot.com/3199042/diff/9001/client/gtk2/ibusimcontext.c#n... client/gtk2/ibusimcontext.c:304: g_object_ref (widget); On 2010/11/19 09:06:27, suzhe wrote: > Same as above event->window case, adding ref here may cause memory leak. More > seriously, this may cause problem if the application expects this widget to be > destroyed within a certain time. Done.
Sign in to reply to this message.
Update Changes. On 2010/11/19 09:57:48, Shawn.P.Huang wrote: > It is better to clean every thing when class is finalized, maybe it is not a > good time to fix it. Because maybe it has some risk. > > BTW， currently im-ibus.so is resident module. It can not be unloaded, because it > calls g_type_module_use in im_module_init (). > > http://codereview.appspot.com/3199042/diff/9001/client/gtk2/ibusimcontext.c > File client/gtk2/ibusimcontext.c (right): > > http://codereview.appspot.com/3199042/diff/9001/client/gtk2/ibusimcontext.c#n... > client/gtk2/ibusimcontext.c:263: break; > On 2010/11/19 09:06:27, suzhe wrote: > > is _use_key_snooper set to true on ChromeOS? If not, then this block will only > > be executed when there is no focused input context, and above statement will > > always be true. So code below will never be executed. > > Done. > > http://codereview.appspot.com/3199042/diff/9001/client/gtk2/ibusimcontext.c#n... > client/gtk2/ibusimcontext.c:272: g_object_ref (event->window); > On 2010/11/19 09:06:27, suzhe wrote: > > If there is no more key event before unloading this im module, this window > will > > be leaked. And I just notice that, this _key_snooper_cb will never be removed, > > which is not good. > > Done. > > http://codereview.appspot.com/3199042/diff/9001/client/gtk2/ibusimcontext.c#n... > client/gtk2/ibusimcontext.c:280: GtkWidget *widget = gtk_get_event_widget > ((GdkEvent *)event); > On 2010/11/19 09:06:27, suzhe wrote: > > I'm not sure if this function returns the top level window. > It does not. But it returns the widget associated with event->window. Actually, > it is the widget has input focus. I think it i we want. > > http://codereview.appspot.com/3199042/diff/9001/client/gtk2/ibusimcontext.c#n... > client/gtk2/ibusimcontext.c:291: NULL); > On 2010/11/19 09:06:27, suzhe wrote: > > I think it's not necessary to disconnect the handler explicitly. You just need > > to avoid connecting them more than once. > > Disconnecting handlers could make callback function simpler. And we don't need > remember which widget has been connected to. > > http://codereview.appspot.com/3199042/diff/9001/client/gtk2/ibusimcontext.c#n... > client/gtk2/ibusimcontext.c:304: g_object_ref (widget); > On 2010/11/19 09:06:27, suzhe wrote: > > Same as above event->window case, adding ref here may cause memory leak. More > > seriously, this may cause problem if the application expects this widget to be > > destroyed within a certain time. > > Done.
Sign in to reply to this message.
|
