Support switching keyboard by hotkey without input focus

Support switching keyboard by hotkey without input focus
Add fake input context in im module. The fake input context will hold the focus if no other input context has focus. This change for fixing input method switch issue if no focus input context.

BUG=http://crosbug.com/4381

[Peng, 2010-07-21 12:30:09]

Hi James and Satoru,

Please review it. Thanks.

[satorux1, 2010-07-21 14:57:43]

The change looks smaller than James's. I confirmed it worked on the netbook.

Would be nice if you could reduce the number of #ifdefs. I have some ideas:

http://codereview.chromium.org/3052003/diff/1/4
File client/gtk2/ibusimcontext.c (right):

http://codereview.chromium.org/3052003/diff/1/4#newcode211
client/gtk2/ibusimcontext.c:211: #ifdef OS_CHROMEOS
I think we don't need this #ifdef here. See below.

http://codereview.chromium.org/3052003/diff/1/4#newcode220
client/gtk2/ibusimcontext.c:220: return FALSE;
Then, we can have:

  ibuscontext = ibusimcontext->ibuscontext;

http://codereview.chromium.org/3052003/diff/1/4#newcode248
client/gtk2/ibusimcontext.c:248: #ifndef OS_CHROMEOS
By having ibuscontext for non Chrome OS, we can get rid of the #ifdef here.

http://codereview.chromium.org/3052003/diff/1/4#newcode257
client/gtk2/ibusimcontext.c:257: case GDK_KEY_PRESS:
And Here.

http://codereview.chromium.org/3052003/diff/1/4#newcode576
client/gtk2/ibusimcontext.c:576: #endif
The three lines should be unnecessary.

http://codereview.chromium.org/3052003/diff/1/4#newcode775
client/gtk2/ibusimcontext.c:775: g_assert (IBUS_IS_IM_CONTEXT (ibusimcontext));
Can we move g_assert this below to reduce a #ifndef?

http://codereview.chromium.org/3052003/diff/1/4#newcode782
client/gtk2/ibusimcontext.c:782: #ifndef OS_CHROMEOS
  g_assert (IBUS_IS_IM_CONTEXT (ibusimcontext));

[James Su, 2010-07-21 17:41:32]

Though this CL looks simpler, it has several issues to be addressed:

1. This approach looks hacky. I don't think it's suitable for upstream.
2. It enables key_snooper and processes key events in it unconditionally, which
is wrong and breaks chrome! key_snooper should only be used to deliver key
events to ibus-daemon when there is no focused input context, and return false
for non-hotkey events.
3. I'm not sure if it's ok to use this feature without enabling global engine.
4. All operations related to panel should be blocked for fake input context.
5. I did not see where the fake input context gets destroyed.
6. (nit) It creates two extra objects for each application.

Overall, comparing to the previous one:
1. This approach is more risky.
2. This approach looks more hacky to me. The semantics of the previous one is
more straightforward than this one.

So I'd still prefer the previous solution.

[Peng, 2010-07-22 04:05:35]

I updated this patch set. Please review it.

On 2010/07/21 17:41:32, James Su wrote:
> Though this CL looks simpler, it has several issues to be addressed:
> 
> 1. This approach looks hacky. I don't think it's suitable for upstream.
> 2. It enables key_snooper and processes key events in it unconditionally,
which
> is wrong and breaks chrome! key_snooper should only be used to deliver key
> events to ibus-daemon when there is no focused input context, and return false
> for non-hotkey events.
   Fixed in new patch

> 3. I'm not sure if it's ok to use this feature without enabling global engine.
   I think this patch is for ChromeOS only by build option OS_CHROMEOS. It will
not effect Linux desktop.

> 4. All operations related to panel should be blocked for fake input context.
   With fake ic, user could change properties when no IC has focus. I think it
is better than before behavior.

> 5. I did not see where the fake input context gets destroyed.
   It will be destroyed automatically, when the connection is lost.

> 6. (nit) It creates two extra objects for each application.
   Just one extra object. It should not be a problem.
> 
> Overall, comparing to the previous one:
> 1. This approach is more risky.
> 2. This approach looks more hacky to me. The semantics of the previous one is
> more straightforward than this one.
> 
> So I'd still prefer the previous solution.

[James Su, 2010-07-22 04:25:39]

Some issues to be clarified:
1. Are you going to merge this change into upstream's master? IMHO, it's not a
good solution at all, though it looks simple.
2. It takes effect even if global engine is disabled, which will be very
strange. Though it's not a problem for ChromeOS, which will always enable global
engine.

http://codereview.chromium.org/3052003/diff/8001/1006
File client/gtk2/ibusimcontext.c (right):

http://codereview.chromium.org/3052003/diff/8001/1006#newcode540
client/gtk2/ibusimcontext.c:540: ibus_input_context_focus_in (_fake_context);
Seems that it never gets focused out. Is it ok?
And can we defer the focus in until we get a key event without focus?

http://codereview.chromium.org/3052003/diff/8001/1006#newcode1056
client/gtk2/ibusimcontext.c:1056: #ifdef OS_CHROMEOS
As I understand, you are never going to merge this change into upstream's
master, right? Then I don't think we need this ifdef.

http://codereview.chromium.org/3052003/diff/8001/1006#newcode1076
client/gtk2/ibusimcontext.c:1076: G_CALLBACK
(_ibus_context_forward_key_event_cb),
We don't need it.

[satorux1, 2010-07-22 05:53:22]

On 2010/07/22 04:25:39, James Su wrote:
> Some issues to be clarified:
> 1. Are you going to merge this change into upstream's master? IMHO, it's not a
> good solution at all, though it looks simple.

I think it's a good idea to have this change in the upstream master, as long as
this does not pollute the code too much. This makes it easier for us to use the
upstream version. By making changes like this in the upstream, we'll soon be
able to use the upstream version as-is (no more local patches!). This will
reduce the maintenance cost significantly.


> 2. It takes effect even if global engine is disabled, which will be very
> strange. Though it's not a problem for ChromeOS, which will always enable
global
> engine.
> 
> http://codereview.chromium.org/3052003/diff/8001/1006
> File client/gtk2/ibusimcontext.c (right):
> 
> http://codereview.chromium.org/3052003/diff/8001/1006#newcode540
> client/gtk2/ibusimcontext.c:540: ibus_input_context_focus_in (_fake_context);
> Seems that it never gets focused out. Is it ok?
> And can we defer the focus in until we get a key event without focus?
> 
> http://codereview.chromium.org/3052003/diff/8001/1006#newcode1056
> client/gtk2/ibusimcontext.c:1056: #ifdef OS_CHROMEOS
> As I understand, you are never going to merge this change into upstream's
> master, right? Then I don't think we need this ifdef.
> 
> http://codereview.chromium.org/3052003/diff/8001/1006#newcode1076
> client/gtk2/ibusimcontext.c:1076: G_CALLBACK
> (_ibus_context_forward_key_event_cb),
> We don't need it.

[Peng, 2010-07-22 06:39:26]

On 2010/07/22 04:25:39, James Su wrote:
> Some issues to be clarified:
> 1. Are you going to merge this change into upstream's master? IMHO, it's not a
> good solution at all, though it looks simple.

Although it is not perfect, but it almost not change the ibus-daemon. I think it
could be upstream. In future, propably we will adjust it, and this change could
be removed easily.

> 2. It takes effect even if global engine is disabled, which will be very
> strange. Though it's not a problem for ChromeOS, which will always enable
global
> engine.

Sorry. I don't know your meaning exactly. In Linux desktop, it will be disabled
by compile condition.

> 
> http://codereview.chromium.org/3052003/diff/8001/1006
> File client/gtk2/ibusimcontext.c (right):
> 
> http://codereview.chromium.org/3052003/diff/8001/1006#newcode540
> client/gtk2/ibusimcontext.c:540: ibus_input_context_focus_in (_fake_context);
> Seems that it never gets focused out. Is it ok?
> And can we defer the focus in until we get a key event without focus?

The fake IC will be focus out, if another IC grab the focus. Or another IC send
some key events to daemon, it will grab focus automatically.

I set focus to fake IC in focus_out callback of other IC, because I want to ibus
always has a focused IC, and then panel can always show currently activated IME
and properties of the IME. Whit it, we could remove some logic of global engine
in panel, to make things simple.

> 
> http://codereview.chromium.org/3052003/diff/8001/1006#newcode1056
> client/gtk2/ibusimcontext.c:1056: #ifdef OS_CHROMEOS
> As I understand, you are never going to merge this change into upstream's
> master, right? Then I don't think we need this ifdef.

I agree it is useless for upstream. So


> 
> http://codereview.chromium.org/3052003/diff/8001/1006#newcode1076
> client/gtk2/ibusimcontext.c:1076: G_CALLBACK
> (_ibus_context_forward_key_event_cb),
> We don't need it.

Currently it is useless. But in the latest ibus, it is necessary. The latest
ibus works in async mode. It means ibus_input_context_process_keyevent will
always consumes the keyevents and returns TRUE immediately. But if the events
can not really consumed by IME or ibus self, it will be forward back to
application. The async mode can avoid block. So it is necessary, if we rebase to
latest ibus.

[James Su, 2010-07-22 07:14:07]

(you may reply the comments inline in the review page, rather than reply the
email).

On 2010/07/22 06:39:26, Peng wrote:
> On 2010/07/22 04:25:39, James Su wrote:
> > Some issues to be clarified:
> > 1. Are you going to merge this change into upstream's master? IMHO, it's not
a
> > good solution at all, though it looks simple.
> 
> Although it is not perfect, but it almost not change the ibus-daemon. I think
it
> could be upstream. In future, propably we will adjust it, and this change
could
> be removed easily.
> 
> > 2. It takes effect even if global engine is disabled, which will be very
> > strange. Though it's not a problem for ChromeOS, which will always enable
> global
> > engine.
> 
> Sorry. I don't know your meaning exactly. In Linux desktop, it will be
disabled
> by compile condition.
If this CL will be upstreamed, then just remove the ifdef below. We shouldn't
use such compile switch for upstreamed code.

> 
> > 
> > http://codereview.chromium.org/3052003/diff/8001/1006
> > File client/gtk2/ibusimcontext.c (right):
> > 
> > http://codereview.chromium.org/3052003/diff/8001/1006#newcode540
> > client/gtk2/ibusimcontext.c:540: ibus_input_context_focus_in
(_fake_context);
> > Seems that it never gets focused out. Is it ok?
> > And can we defer the focus in until we get a key event without focus?
> 
> The fake IC will be focus out, if another IC grab the focus. Or another IC
send
> some key events to daemon, it will grab focus automatically.
> 
> I set focus to fake IC in focus_out callback of other IC, because I want to
ibus
> always has a focused IC, and then panel can always show currently activated
IME
> and properties of the IME. Whit it, we could remove some logic of global
engine
> in panel, to make things simple.
> 
> > 
> > http://codereview.chromium.org/3052003/diff/8001/1006#newcode1056
> > client/gtk2/ibusimcontext.c:1056: #ifdef OS_CHROMEOS
> > As I understand, you are never going to merge this change into upstream's
> > master, right? Then I don't think we need this ifdef.
> 
> I agree it is useless for upstream. So
Please just remove this ifdef, no matter whether you are going to merge it into
upstream or not. It's harmful for maintenance.

> 
> 
> > 
> > http://codereview.chromium.org/3052003/diff/8001/1006#newcode1076
> > client/gtk2/ibusimcontext.c:1076: G_CALLBACK
> > (_ibus_context_forward_key_event_cb),
> > We don't need it.
> 
> Currently it is useless. But in the latest ibus, it is necessary. The latest
> ibus works in async mode. It means ibus_input_context_process_keyevent will
> always consumes the keyevents and returns TRUE immediately. But if the events
> can not really consumed by IME or ibus self, it will be forward back to
> application. The async mode can avoid block. So it is necessary, if we rebase
to
> latest ibus.
Do NOT use async keyboard event. It will BREAK chrome and probably other
applications.

[Peng, 2010-07-22 07:40:22]

On 2010/07/22 07:14:07, James Su wrote:
> (you may reply the comments inline in the review page, rather than reply the
> email).
> > > 
> > > http://codereview.chromium.org/3052003/diff/8001/1006#newcode1076
> > > client/gtk2/ibusimcontext.c:1076: G_CALLBACK
> > > (_ibus_context_forward_key_event_cb),
> > > We don't need it.
> > 
> > Currently it is useless. But in the latest ibus, it is necessary. The latest
> > ibus works in async mode. It means ibus_input_context_process_keyevent will
> > always consumes the keyevents and returns TRUE immediately. But if the
events
> > can not really consumed by IME or ibus self, it will be forward back to
> > application. The async mode can avoid block. So it is necessary, if we
rebase
> to
> > latest ibus.
> Do NOT use async keyboard event. It will BREAK chrome and probably other
> applications.

In sync mode, we have some critical issues can not be resolved. 
Some time the daemon or the IME will not reply the key event, it will block
application totally. It is not acceptable.

[satorux1, 2010-07-22 07:46:23]

On 2010/07/22 07:14:07, James Su wrote:
> (you may reply the comments inline in the review page, rather than reply the
> email).
> 
> On 2010/07/22 06:39:26, Peng wrote:
> > On 2010/07/22 04:25:39, James Su wrote:
> > > Some issues to be clarified:
> > > 1. Are you going to merge this change into upstream's master? IMHO, it's
not
> a
> > > good solution at all, though it looks simple.
> > 
> > Although it is not perfect, but it almost not change the ibus-daemon. I
think
> it
> > could be upstream. In future, propably we will adjust it, and this change
> could
> > be removed easily.
> > 
> > > 2. It takes effect even if global engine is disabled, which will be very
> > > strange. Though it's not a problem for ChromeOS, which will always enable
> > global
> > > engine.
> > 
> > Sorry. I don't know your meaning exactly. In Linux desktop, it will be
> disabled
> > by compile condition.
> If this CL will be upstreamed, then just remove the ifdef below. We shouldn't
> use such compile switch for upstreamed code.
> 
> > 
> > > 
> > > http://codereview.chromium.org/3052003/diff/8001/1006
> > > File client/gtk2/ibusimcontext.c (right):
> > > 
> > > http://codereview.chromium.org/3052003/diff/8001/1006#newcode540
> > > client/gtk2/ibusimcontext.c:540: ibus_input_context_focus_in
> (_fake_context);
> > > Seems that it never gets focused out. Is it ok?
> > > And can we defer the focus in until we get a key event without focus?
> > 
> > The fake IC will be focus out, if another IC grab the focus. Or another IC
> send
> > some key events to daemon, it will grab focus automatically.
> > 
> > I set focus to fake IC in focus_out callback of other IC, because I want to
> ibus
> > always has a focused IC, and then panel can always show currently activated
> IME
> > and properties of the IME. Whit it, we could remove some logic of global
> engine
> > in panel, to make things simple.
> > 
> > > 
> > > http://codereview.chromium.org/3052003/diff/8001/1006#newcode1056
> > > client/gtk2/ibusimcontext.c:1056: #ifdef OS_CHROMEOS
> > > As I understand, you are never going to merge this change into upstream's
> > > master, right? Then I don't think we need this ifdef.
> > 
> > I agree it is useless for upstream. So
> Please just remove this ifdef, no matter whether you are going to merge it
into
> upstream or not. It's harmful for maintenance.

I think there is room for discussion here. IMHO, use of #ifdef is acceptable as
long as we don't overuse it. The question is which is more costly for
maintenance:

1. having #ifdef'ed code in the master branch
2. having local changes in a branch and merge the master branch from time to
time

1. would be less costly as long as we use #ifdef sparingly.


> 
> > 
> > 
> > > 
> > > http://codereview.chromium.org/3052003/diff/8001/1006#newcode1076
> > > client/gtk2/ibusimcontext.c:1076: G_CALLBACK
> > > (_ibus_context_forward_key_event_cb),
> > > We don't need it.
> > 
> > Currently it is useless. But in the latest ibus, it is necessary. The latest
> > ibus works in async mode. It means ibus_input_context_process_keyevent will
> > always consumes the keyevents and returns TRUE immediately. But if the
events
> > can not really consumed by IME or ibus self, it will be forward back to
> > application. The async mode can avoid block. So it is necessary, if we
rebase
> to
> > latest ibus.
> Do NOT use async keyboard event. It will BREAK chrome and probably other
> applications.

[James Su, 2010-07-22 17:18:55]

1. For the ifdef thing:
  The right thing we should do here is to turn on/off this fake IC feature based
on the configuration of global engine. If you insist to use the ifdef and imply
that global engine will always be used in chrome os, then you need to state it
clearly in comment. As long as you comment everything clearly, I have no other
objection to this CL.

2. For the async keyboard events
  Please remember: NEVER switch to async model, at least not for chrome os.
Please refer to http://www.w3.org/TR/DOM-Level-3-Events/#events-textevents for
the reason. Especially section 5.2.6-5.2.9 and section 6. No matter what you do,
we need to keep chrome comply with the specification strictly.
  The right solution is to have a timeout for any sync call and terminate the
call with a failure return value if the daemon fails to response within the
timeout. And never forward key event back.

[Peng, 2010-07-23 06:13:15]

On 2010/07/22 17:18:55, James Su wrote:
> 1. For the ifdef thing:
>   The right thing we should do here is to turn on/off this fake IC feature
based
> on the configuration of global engine. If you insist to use the ifdef and
imply
> that global engine will always be used in chrome os, then you need to state it
> clearly in comment. As long as you comment everything clearly, I have no other
> objection to this CL.

Update patches, added some comments to explain the code. Please check it again.
Thanks.

> 
> 2. For the async keyboard events
>   Please remember: NEVER switch to async model, at least not for chrome os.
> Please refer to http://www.w3.org/TR/DOM-Level-3-Events/#events-textevents for
> the reason. Especially section 5.2.6-5.2.9 and section 6. No matter what you
do,
> we need to keep chrome comply with the specification strictly.
>   The right solution is to have a timeout for any sync call and terminate the
> call with a failure return value if the daemon fails to response within the
> timeout. And never forward key event back.

In current version, it is in sync mode. And we could do more investigation and
discussion in another issue or mail list.

[James Su, 2010-07-23 07:04:39]

Please add comment about the relationship between fake input context and global
engine. And why we don't need to care about it on ChromeOS.

[James Su, 2010-07-23 07:12:54]

As a temporary solution only for Chrome OS, I'm ok with this CL.

http://codereview.chromium.org/3052003/diff/8002/19003
File client/gtk2/ibusimcontext.c (right):

http://codereview.chromium.org/3052003/diff/8002/19003#newcode1075
client/gtk2/ibusimcontext.c:1075: */
Oh, you added comment here.

http://codereview.chromium.org/3052003/diff/8002/19003#newcode1082
client/gtk2/ibusimcontext.c:1082: NULL);
I'd prefer not to connect this signal.

[Peng, 2010-07-23 07:52:57]

On 2010/07/23 07:12:54, James Su wrote:
> As a temporary solution only for Chrome OS, I'm ok with this CL.
> 
> http://codereview.chromium.org/3052003/diff/8002/19003
> File client/gtk2/ibusimcontext.c (right):
> 
> http://codereview.chromium.org/3052003/diff/8002/19003#newcode1075
> client/gtk2/ibusimcontext.c:1075: */
> Oh, you added comment here.
> 
> http://codereview.chromium.org/3052003/diff/8002/19003#newcode1082
> client/gtk2/ibusimcontext.c:1082: NULL);
> I'd prefer not to connect this signal.

Do not connect this signal in last update. Please check it. Thanks.