Created attachment 195755 [details] [review]
Patch for st-im-text.c

I attached the patch.

Review of attachment 195755 [details] [review]:

I'd like to see an 'activate' signal added to StEntry signal that is internally hooked up to do the right thing rather than this.

Created attachment 198417 [details] [review]
Patch for st-im-text.c

Revised the patch.

(In reply to comment #2)
> Review of attachment 195755 [details] [review]:
> 
> I'd like to see an 'activate' signal added to StEntry signal that is internally
> hooked up to do the right thing rather than this.

OK, I added 'activate' signal but didn't use clutter_binding_pool_install_action() in StEntry against ClutterText because the previous change seems to check Control + Enter key:
http://git.gnome.org/browse/gnome-shell/commit/?id=203dedfb3ab216ee4bcd5d3a7e158cc9506410ca

I think StEntry needs st_im_text_filter_keypress() because StIMText.priv->im_context and StIMText.priv->need_im_reset are private members in StIMText.
Maybe we would need the similar API in GtkEntry in GTK3.

What about the patch?

I don't think this is the right way to patch, since it's more like hack and only take "Enter" into account, actually this should be take as the exact same issue as this: https://bugzilla.gnome.org/show_bug.cgi?id=90082 (Though with clutter there seems need to be something different.)

*** Bug 669080 has been marked as a duplicate of this bug. ***

*** Bug 676949 has been marked as a duplicate of this bug. ***

Given that native IBus integration is happening, I don't think we want this anymore.

Still needed.

So could a gnome-shell maintainer review the patch here?

Review of attachment 198417 [details] [review]:

I'm not a gnome-shell maintainer but fwiw.

This patch no longer applies cleanly to the latest git master.
I personally think it has unnecessary complicated code and should be rewritten.

::: js/ui/viewSelector.js
@@ +148,1 @@
                 return true;

"activate" signal does not have return value.

::: src/st/st-entry.c
@@ +84,3 @@
   PRIMARY_ICON_CLICKED,
   SECONDARY_ICON_CLICKED,
+  ACTIVATE,

Why do you want to extend StEntry instead of StIMText?  StEntry's "text" property is visible from JS and you can directly connect to the "activate" signal of that.

@@ +466,3 @@
+  /* We do not use clutter_binding_pool_find but check keyval directly
+   * because the receivers might want to connect 'activate' signal
+   * with/without any modifiers. */

Why not install custom binding pool and chain up to "key-press-event" handler of ClutterText?  That will help a little not to scatter IM handling code in various places.

::: src/st/st-im-text.c
@@ +447,2 @@
 /**
+ * st_im_text_filter_keypress:

I don't think this is necessary atm, as long as only Control modifier is needed.

Created attachment 222913 [details] [review]
st-im-text: install additional "activate" key bindings for search providers

Use "activate" signal instead of "key-press-event" to activate search.
Since some search providers require Control modifier to change
the behavior, install custom keybindings for "activate" in StIMText.

Review of attachment 222913 [details] [review]:

I don't like it much that this adds even more special cases.

Created attachment 223015 [details] [review]
Use connect_after() for entries' key-press-event signals

This way we don't unnecessarily override default behavior. In
particular input methods' key processing now has a chance to deal with
important keys for its operation like Return/Enter, Up, Down or Tab.

Unmodified Return/Enter needs to be treated differently though since
ClutterText processes those by default and thus they are not available
on connect_after(). But it emits the 'activate' signal which we can
use instead.

--

What about this instead?

We expect input method gets the key events at first. Currently ibus also uses key snooper.
I'd think it would be great if the application handler could use connect_after() and input method would use connect() instead.

(In reply to comment #14)
> Unmodified Return/Enter needs to be treated differently though since
> ClutterText processes those by default and thus they are not available
> on connect_after(). But it emits the 'activate' signal which we can
> use instead.

The patch, if it works, looks ok to me as a short-term workaround, but it looks a bit unfortunate that the actual handler code copied among "key-press-event" and "activate" handlers.  I'd suggest to add a comment explaining why, to avoid any confusion in the future.

(In reply to comment #13)
> Review of attachment 222913 [details] [review]:
> 
> I don't like it much that this adds even more special cases.

If you mean the code manually installing the keybindings in st-im-text.c by "special cases", I could think of moving it to CSS (like GTK3), that would be a long-term solution though.

(In reply to comment #16)
> (In reply to comment #14)
> > Unmodified Return/Enter needs to be treated differently though since
> > ClutterText processes those by default and thus they are not available
> > on connect_after(). But it emits the 'activate' signal which we can
> > use instead.
> 
> The patch, if it works, looks ok to me as a short-term workaround, but it looks
> a bit unfortunate that the actual handler code copied among "key-press-event"
> and "activate" handlers.  I'd suggest to add a comment explaining why, to avoid
> any confusion in the future.

It is unfortunate but since ClutterText already does its own processing of unmodified Return/Enter we can't do much better. Well, and this is only an issue because for these specific entries we want to handle Ctrl+Return differently so I don't thing having specific code for a specific different case is that wrong.

> (In reply to comment #13)
> > Review of attachment 222913 [details] [review] [details]:
> > 
> > I don't like it much that this adds even more special cases.
> 
> If you mean the code manually installing the keybindings in st-im-text.c by
> "special cases",

I mean that adding more keybindings because *some* users of StIMText want to use Ctrl+Return differently doesn't look right. Next we'll have to add Shift+Return etc. And what about Up, Down, Tab, etc.?

> I could think of moving it to CSS (like GTK3), that would be a
> long-term solution though.

I don't understand this part.

Created attachment 224129 [details] [review]
st-im-text: Replace key-* handler with captured-event

When using an input method like IBus, the IM is expected to process
key events before anything else. Currently this doesn't always work
as expected, as the event filtering is done in the default handlers
of the key-press and key-release events, e.g. only after other
handlers have been run.
To allow the IM to filter events earlier, move the code to a
captured-event handler instead.

We clearly haven't had enough attempts to fix this issue, so here's a fourth one :-)

I have looked at the connect_after() approach, but I don't really like it either:
 - the (moderate) code duplication makes the code harder to read:

   entry.connect('key-press-event', function() {
       if (key == RETURN) {
           if (mods & CONTROL_MASK)
               doSomething(true);
          else
              doSomething(false);
       }
   });

  is easier to read IMHO than

  entry.connect('activate', function() {
      doSomething(false);  
  });
  entry.connect('key-press-event', function() {
    if (key == RETURN && mods & CONTROL_MASK)
        doSomething(true); 
 });

 - it assumes that search providers may want to treat ctrl-enter differently,
   but not activation with any other modifier (which nothing in core does, but
   extensions may as the current code allows it)

 - probably the most important one:
   having to use connect_after() instead of connect() for entries is not exactly obvious, which
   means that we will keep sneaking broken entries in :-)
   also entries in extensions are almost guaranteed to be broken (unless the extension author
  happens to use IM)

Review of attachment 224129 [details] [review]:

This is a better approach indeed. Ok to push with the fixes below

::: src/st/st-im-text.c
@@ +357,3 @@
 static gboolean
+st_im_text_captured_event (ClutterActor    *actor,
+                           ClutterKeyEvent *event)

This should be ClutterEvent *

Created attachment 224132 [details] [review]
st-im-text: Replace key-* handler with captured-event

(Fix a compiler warning - sorry, I thought I had -Werror in my CFLAGS ...)

Review of attachment 224132 [details] [review]:

yup

Attachment 224132 [details] pushed as 49df72c - st-im-text: Replace key-* handler with captured-event