Hide Property Panel when users type keys except for shortcut keys.

Hide Property Panel when input cursor or preedit text is changed.


BUG=http://code.google.com/p/ibus/issues/detail?id=1659
TEST=ui/gtk3/ibus-ui-gtk3

[Peng, 2013-12-25 22:05:15]

https://codereview.appspot.com/45510043/diff/1/bus/panelproxy.c
File bus/panelproxy.c (right):

https://codereview.appspot.com/45510043/diff/1/bus/panelproxy.c#newcode414
bus/panelproxy.c:414: NULL, NULL, NULL);
I can see this change will send every key events to panel. I think it is a kind
of wast. I am not sure is it is really necessary. In most case, pressing keys,
will cause cursor position changes. Can we hide panel, when the cursor position
changed instead?

[fujiwara, 2013-12-26 07:12:26]

https://codereview.appspot.com/45510043/diff/1/bus/panelproxy.c
File bus/panelproxy.c (right):

https://codereview.appspot.com/45510043/diff/1/bus/panelproxy.c#newcode414
bus/panelproxy.c:414: NULL, NULL, NULL);
On 2013/12/25 22:05:15, Peng wrote:
> I can see this change will send every key events to panel. I think it is a
kind
> of wast. I am not sure is it is really necessary. In most case, pressing keys,
> will cause cursor position changes. Can we hide panel, when the cursor
position
> changed instead?

OK, updated the patch.
Now the panel is hid when the cursor position or preedit is changed.

[Peng, 2013-12-26 15:57:06]

https://codereview.appspot.com/45510043/diff/40001/bus/inputcontext.c
File bus/inputcontext.c (right):

https://codereview.appspot.com/45510043/diff/40001/bus/inputcontext.c#newcode127
bus/inputcontext.c:127: UPDATE_PREEDIT_TEXT_BY_CONTEXT,
Don't understand why need a new signal. Could you please explain it? Why
existing signals do not meet our requirements?

[fujiwara, 2013-12-27 03:39:16]

https://codereview.appspot.com/45510043/diff/40001/bus/inputcontext.c
File bus/inputcontext.c (right):

https://codereview.appspot.com/45510043/diff/40001/bus/inputcontext.c#newcode127
bus/inputcontext.c:127: UPDATE_PREEDIT_TEXT_BY_CONTEXT,
On 2013/12/26 15:57:06, Peng wrote:
> Don't understand why need a new signal. Could you please explain it? Why
> existing signals do not meet our requirements?

Because candidatepanel.vala needs to receive the UPDATE_PREEDIT_TEXT signal only
when embed-preedit-text gsetting value is false.

OK, now I updated the patch and candidatepanel.vala ignores the preedit signal
when embed-preedit-text value is true.

[Peng, 2013-12-30 22:54:17]

https://codereview.appspot.com/45510043/diff/100001/data/ibus.schemas.in
File data/ibus.schemas.in (right):

https://codereview.appspot.com/45510043/diff/100001/data/ibus.schemas.in#newc...
data/ibus.schemas.in:204: <key>/schemas/desktop/ibus/panel/timeout</key>
auto_hide_timeout?

https://codereview.appspot.com/45510043/diff/100001/ui/gtk3/propertypanel.vala
File ui/gtk3/propertypanel.vala (right):

https://codereview.appspot.com/45510043/diff/100001/ui/gtk3/propertypanel.val...
ui/gtk3/propertypanel.vala:36: private int m_show = PanelShow.AUTO_HIDE;
How about reuse m_auto_hide_timeout? If m_auto_hide_timeout is 0, it means auto
hide is disabled.

maybe add some comment about those variables to help reviewer understand the
code?

https://codereview.appspot.com/45510043/diff/100001/ui/gtk3/propertypanel.val...
ui/gtk3/propertypanel.vala:37: private uint m_timeout = 10000;
m_auto_hide_timeout?

https://codereview.appspot.com/45510043/diff/100001/ui/gtk3/propertypanel.val...
ui/gtk3/propertypanel.vala:38: private uint m_timeout_id = 0;
m_auto_hide_timeout_id?

https://codereview.appspot.com/45510043/diff/100001/ui/gtk3/propertypanel.val...
ui/gtk3/propertypanel.vala:39: private bool m_done_auto_hide = false;
This variable name is not clear. Probably you don't need this variable.
You could set m_cursor_location to an invalid value in focus_in(), and then
set_cursor_location() may test m_cursor_location to decide if need show the
panel.

Thought?

https://codereview.appspot.com/45510043/diff/100001/ui/gtk3/propertypanel.val...
ui/gtk3/propertypanel.vala:95: if (m_cursor_location != location) {
I am thinking when the (m_cursor_location == location) is true. Maybe most apps
will update cursor location when preedit text is changed. If it true, probably
we don't need send preedit text to panel, if embedded preedit text is enabled.
We just need hide the property panel when set_cursor_location is received.

https://codereview.appspot.com/45510043/diff/100001/ui/gtk3/propertypanel.val...
ui/gtk3/propertypanel.vala:104: if (m_show == PanelShow.AUTO_HIDE &&
m_timeout_id != 0) {
It is better to hide the property panel first if necessary, and then
adjust_window_position() if necessary.

call usnet_show_timer()?

https://codereview.appspot.com/45510043/diff/100001/ui/gtk3/propertypanel.val...
ui/gtk3/propertypanel.vala:256: private void set_show_timer() {
rename it to show_with_auto_hide_timer()?

https://codereview.appspot.com/45510043/diff/100001/ui/gtk3/propertypanel.val...
ui/gtk3/propertypanel.vala:283: private void unset_show_timer() {
maybe rename it to hide_if_necessary()?

[fujiwara, 2014-01-07 05:14:41]

https://codereview.appspot.com/45510043/diff/100001/data/ibus.schemas.in
File data/ibus.schemas.in (right):

https://codereview.appspot.com/45510043/diff/100001/data/ibus.schemas.in#newc...
data/ibus.schemas.in:204: <key>/schemas/desktop/ibus/panel/timeout</key>
On 2013/12/30 22:54:18, Peng wrote:
> auto_hide_timeout?

Done.

https://codereview.appspot.com/45510043/diff/100001/ui/gtk3/propertypanel.vala
File ui/gtk3/propertypanel.vala (right):

https://codereview.appspot.com/45510043/diff/100001/ui/gtk3/propertypanel.val...
ui/gtk3/propertypanel.vala:36: private int m_show = PanelShow.AUTO_HIDE;
On 2013/12/30 22:54:18, Peng wrote:
> How about reuse m_auto_hide_timeout? If m_auto_hide_timeout is 0, it means
auto
> hide is disabled.
> 
> maybe add some comment about those variables to help reviewer understand the
> code?

m_show value is taken by ibus-setup list.

self.__settings_panel.bind('show',
                           self.__combobox_panel_show,
                           'active',
                           Gio.SettingsBindFlags.DEFAULT)

and the UI value and gsettings value are mutual binded.
Probably I think reuse m_auto_hide_timeout could complex ibus-setup so it would
be good to keep the current code.

https://codereview.appspot.com/45510043/diff/100001/ui/gtk3/propertypanel.val...
ui/gtk3/propertypanel.vala:37: private uint m_timeout = 10000;
On 2013/12/30 22:54:18, Peng wrote:
> m_auto_hide_timeout?

Done.

https://codereview.appspot.com/45510043/diff/100001/ui/gtk3/propertypanel.val...
ui/gtk3/propertypanel.vala:38: private uint m_timeout_id = 0;
On 2013/12/30 22:54:18, Peng wrote:
> m_auto_hide_timeout_id?

Done.

https://codereview.appspot.com/45510043/diff/100001/ui/gtk3/propertypanel.val...
ui/gtk3/propertypanel.vala:39: private bool m_done_auto_hide = false;
On 2013/12/30 22:54:18, Peng wrote:
> This variable name is not clear. Probably you don't need this variable.
> You could set m_cursor_location to an invalid value in focus_in(), and then
> set_cursor_location() may test m_cursor_location to decide if need show the
> panel.
> 
> Thought?

OK, I understood the idea and deleted m_done_auto_hide .

https://codereview.appspot.com/45510043/diff/100001/ui/gtk3/propertypanel.val...
ui/gtk3/propertypanel.vala:95: if (m_cursor_location != location) {
On 2013/12/30 22:54:18, Peng wrote:
> I am thinking when the (m_cursor_location == location) is true. Maybe most
apps
> will update cursor location when preedit text is changed. If it true, probably
> we don't need send preedit text to panel, if embedded preedit text is enabled.
> We just need hide the property panel when set_cursor_location is received.

You're right. I didn't notice the behavior is different by applications in
embedded preedit mode.
At the moment, I removed the patch in bus/inputcontext.c and added a comment
here.
I will investigate VTE later.
I'm not sure if we also can change xterm.

https://codereview.appspot.com/45510043/diff/100001/ui/gtk3/propertypanel.val...
ui/gtk3/propertypanel.vala:104: if (m_show == PanelShow.AUTO_HIDE &&
m_timeout_id != 0) {
On 2013/12/30 22:54:18, Peng wrote:
> It is better to hide the property panel first if necessary, and then
> adjust_window_position() if necessary.
> 
> call usnet_show_timer()?

Done.

https://codereview.appspot.com/45510043/diff/100001/ui/gtk3/propertypanel.val...
ui/gtk3/propertypanel.vala:256: private void set_show_timer() {
On 2013/12/30 22:54:18, Peng wrote:
> rename it to show_with_auto_hide_timer()?

Done.

https://codereview.appspot.com/45510043/diff/100001/ui/gtk3/propertypanel.val...
ui/gtk3/propertypanel.vala:283: private void unset_show_timer() {
On 2013/12/30 22:54:18, Peng wrote:
> maybe rename it to hide_if_necessary()?

Done.

[Peng, 2014-01-07 21:47:31]

lgtm

https://codereview.appspot.com/45510043/diff/100001/ui/gtk3/propertypanel.vala
File ui/gtk3/propertypanel.vala (right):

https://codereview.appspot.com/45510043/diff/100001/ui/gtk3/propertypanel.val...
ui/gtk3/propertypanel.vala:95: if (m_cursor_location != location) {
On 2014/01/07 05:14:42, fujiwara wrote:
> On 2013/12/30 22:54:18, Peng wrote:
> > I am thinking when the (m_cursor_location == location) is true. Maybe most
> apps
> > will update cursor location when preedit text is changed. If it true,
probably
> > we don't need send preedit text to panel, if embedded preedit text is
enabled.
> > We just need hide the property panel when set_cursor_location is received.
> 
> You're right. I didn't notice the behavior is different by applications in
> embedded preedit mode.
> At the moment, I removed the patch in bus/inputcontext.c and added a comment
> here.
> I will investigate VTE later.
> I'm not sure if we also can change xterm.

Thanks. I think probable it is OK for some apps the panel is not hide
automatically when preedit is changed. It is not a big issue. if you could place
the properties panel carefully, to avoid it covers the preedit text.  right?

[fujiwara, 2014-01-08 07:34:47]

https://codereview.appspot.com/45510043/diff/100001/ui/gtk3/propertypanel.vala
File ui/gtk3/propertypanel.vala (right):

https://codereview.appspot.com/45510043/diff/100001/ui/gtk3/propertypanel.val...
ui/gtk3/propertypanel.vala:95: if (m_cursor_location != location) {
On 2014/01/07 21:47:31, Peng wrote:
> On 2014/01/07 05:14:42, fujiwara wrote:
> > On 2013/12/30 22:54:18, Peng wrote:
> > > I am thinking when the (m_cursor_location == location) is true. Maybe most
> > apps
> > > will update cursor location when preedit text is changed. If it true,
> probably
> > > we don't need send preedit text to panel, if embedded preedit text is
> enabled.
> > > We just need hide the property panel when set_cursor_location is received.
> > 
> > You're right. I didn't notice the behavior is different by applications in
> > embedded preedit mode.
> > At the moment, I removed the patch in bus/inputcontext.c and added a comment
> > here.
> > I will investigate VTE later.
> > I'm not sure if we also can change xterm.
> 
> Thanks. I think probable it is OK for some apps the panel is not hide
> automatically when preedit is changed. It is not a big issue. if you could
place
> the properties panel carefully, to avoid it covers the preedit text.  right?

I think the original request (issue #1659) is to hide the panel with a key
whether it covers the preedit or not. Also the disabled-embedded preedit is
always superior to property panel so maybe your issue is no problem but probably
I need to investigate some applications about get_preedit_string().

I think property panel does not hide embedded preedit.