Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cannot select candidate on GTK3 application #77

Closed
t-site opened this issue Feb 28, 2016 · 5 comments

Comments

Projects
None yet
3 participants
@t-site
Copy link

commented Feb 28, 2016

on GTK3 applications include Firefox 44 on Arch linux x86_64,
When I typed "さかたくんはさかたしにいった", I caught chnage only one word.
on GTK2 applications, no such bug. no IME engine error.
for example,
initilal candiate 阪田君は坂田氏にいった
change to 酒田クンは坂田氏にいった
then, press right allow key
阪田君は坂田氏にいった
rewind 酒田クン to 阪田君 .
I cannot change more than 2 words.
This Problem is not solved on git master.

@dai-vdr dai-vdr added bug GTK3 labels Feb 28, 2016

@NaofumiHonda

This comment has been minimized.

Copy link
Contributor

commented May 24, 2016

I also confirm this issue with the candidate windows of vertical type, however,
the problem does not occure with the one of horizontal or table types.
So I checked related source codes and found that the only essential difference
of those is "emitting or not an index-change signal in a selection change of candidates".
Although I don't understand why this signal emitting causes the problem, the following
patch resolves the issue for gtk3 case (and works well for gtk2 also).

In ./gtk2/immodule/uim-cand-win-vertical-gtk.c around line 272-278

if (cwin->index_changed) {
  cwin->index_changed = FALSE;
  /* g_signal_emit_by_name(G_OBJECT(cwin), "index-changed"); */ /* commented out! */
}
@dai-vdr

This comment has been minimized.

Copy link
Contributor

commented Jun 9, 2016

@NaofumiHonda, Thank you for your investigation.
This signal emittion is introduced by bc40465 to avoid SEGV.
So I do not know the result of your comment-out.

@NaofumiHonda

This comment has been minimized.

Copy link
Contributor

commented Jun 9, 2016

Thank you for related information.
The suggested commitment just changes a timing of the signal emitting, and hence,
my previous patch does nothing worse for this SEGV.

Now why the patch works? I did further investigation.
A point is that:
Around the line 209 in gtk2/immodule/uim-cand-win-vertical-gtk.c,
the code assumes that the conditions
path_currently_selected == false and cwin->candidate_index != idx
only hold for the case that a candidate is selected by a mouse click.
Surely this assumption is true for gkt2 case. However, in gtk3 case, the same
conditions also hold when the candidate window becomes hidden by
changing a target word, which causes a wired phenomenon.
Therefore we need to consider the latter case also and
it has been done in the attached patch.

Of course, this patch respects the commitment bc40465 for SEGV.

patch.zip

@dai-vdr

This comment has been minimized.

Copy link
Contributor

commented Jun 10, 2016

great works 👍
but how about moving up comment together?

--- a/gtk2/immodule/uim-cand-win-vertical-gtk.c
+++ b/gtk2/immodule/uim-cand-win-vertical-gtk.c
@@ -206,16 +206,19 @@ tree_selection_change(GtkTreeSelection *selection,
   g_return_val_if_fail(indicies, TRUE);
   idx = *indicies + cwin->display_limit * cwin->page_index;

+  if (path_currently_selected && cwin->candidate_index >= 0) {
+    /* if emit "index-changed" here and IM deactivates this candwin,
+     * activates new candwin and selects a candidate on new candwin
+     * from index-changed callback, SEGV occurs in gtk because gtk tries to
+     * select on old candwin after return of this tree_selection_change().
+     * To avoid SEGV, instead of emitting before selection change by gtk,
+     * emit after selection changed by gtk. */
+    cwin->index_changed = TRUE;
+  }
+
   if (!path_currently_selected && cwin->candidate_index != idx) {
     if (cwin->candidate_index >= 0) {
       cwin->candidate_index = idx;
-      /* if emit "index-changed" here and IM deactivates this candwin,
-       * activates new candwin and selects a candidate on new candwin
-       * from index-changed callback, SEGV occurs in gtk because gtk tries to
-       * select on old candwin after return of this tree_selection_change().
-       * To avoid SEGV, instead of emitting before selection change by gtk,
-       * emit after selection changed by gtk. */
-      cwin->index_changed = TRUE;
     }

     uim_cand_win_gtk_update_label(cwin);
@NaofumiHonda

This comment has been minimized.

Copy link
Contributor

commented Jun 10, 2016

You are absolutely right. Thanks!

@dai-vdr dai-vdr closed this in 811113f Jun 10, 2016

dai-vdr added a commit that referenced this issue Jun 10, 2016

Merge pull request #84 from dai-vdr/fix-candwin-vertical-gtk
fix cannot select candidate on GTK3 application, closes #77
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.