Lost input in quickfill cells due to race

[Jim Paris]

Christian Stimming wrote:
> Am Dienstag, 24. Mai 2011 schrieb Jim Paris:
> > Jim Paris wrote:
> > > Setting the selection can't be done asynchronously like this.
> > > I'll see if I can come up with a clean fix...
> > 
> > No luck coming up with anything yet.  I can add some hacks, like
> > storing the selection region separately and having the callers always
> > do gtk_editable_select_region after gtk_editable_insert_text, but
> > that's not very clean.  Another option might be to remove the custom
> > quickfill code and switch to GtkEntryCompletion, but that's very
> > invasive and involved.  Any suggestions?
> 
> In the long run, we should probably switch to GtkEntryCompletion.
> 
> As an intermediate solution, I think it would be fine if you add some 
> intermediate data that ensures the timeout callback does not drop any 
> characters that appeared in the meantime. Like, can the callback data also 
> store the current position and verify it hasn't changed when it is run? If it 
> had changed (i.e. additional characters have been received), the callback 
> should just do nothing.

I've been looking at it some more and came up with the below fix,
which seems to work.  The keyboard input ends up in the commit_cb
which calls gtk_editable_insert_text then gtk_editable_set_position.
It's the call to _set_position that clears the selection!

I think this might be a bug in GTK+, because gtk_entry_real_set_position
does this:

  if (position != priv->current_pos ||
      position != priv->selection_bound)
    {
      _gtk_entry_reset_im_context (entry);
      gtk_entry_set_positions (entry, position, position);
    }

The test is basically always true.  I don't think that || was
intended: priv->current_pos is the start of the selection,
priv->selection_bound is the end of it.  If the test were &&, then
an attempt to position the cursor at either end of the selection
wouldn't have cleared it.

But either way, the below patch seems to work fine on gnucash for now.
You can also pull it from branch 'master' at https://git.jim.sh/jim/gnucash.git

A side note: I've just realized that this use of selection for
autocompletion is one of the big causes of input lag when running
gnucash over X11-forwarding.  I guess it's the setting of the X
primary selection causing a flurry of roundtrips every time you hit a
key.  But at least now that won't translate into lost input anymore.

-jim

From: Jim Paris <jim at jtan.com>
Date: Tue, 24 May 2011 14:45:06 -0400
Subject: [PATCH] Preserve selection around the call to gtk_editable_set_position.

This lets us drop the racy gnucash_sheet_select_data_cb, which fixes
problems with lost input.
---
 src/register/register-gnome/gnucash-sheet.c |   35 ++++++--------------------
 1 files changed, 8 insertions(+), 27 deletions(-)

diff --git a/src/register/register-gnome/gnucash-sheet.c b/src/register/register-gnome/gnucash-sheet.c
index 2d7142f..f5e8671 100644
--- a/src/register/register-gnome/gnucash-sheet.c
+++ b/src/register/register-gnome/gnucash-sheet.c
@@ -844,22 +844,6 @@ typedef struct
 
 } select_info;
 
-/*
- * This function is needed because gtk_entry_insert_text() sets the
- * cursor position after emitting the "insert+text" signal.  This
- * means that the gtk_editable_select_region() call cannot be in the
- * insert_cb function because it will just be cancelled out after
- * the function finishes running.
- */
-static gboolean
-gnucash_sheet_select_data_cb (select_info *info)
-{
-    gtk_editable_select_region (info->editable,
-                                info->start_sel, info->end_sel);
-    g_free(info);
-    return FALSE; /* This is a one shot function */
-}
-
 static void
 gnucash_sheet_insert_cb (GtkWidget *widget,
                          const gchar *insert_text,
@@ -999,17 +983,7 @@ gnucash_sheet_insert_cb (GtkWidget *widget,
         *position = g_utf8_strlen(retval, -1);
 
     if (start_sel != end_sel)
-    {
-        select_info *info;
-
-        info = g_malloc(sizeof(*info));
-        info->editable = editable;
-        info->start_sel = start_sel;
-        info->end_sel = end_sel;
-        g_timeout_add(/*ASAP*/ 1,
-                               (GSourceFunc)gnucash_sheet_select_data_cb,
-                               info);
-    }
+	gtk_editable_select_region(editable, start_sel, end_sel);
 
     g_string_free (new_text_gs, TRUE);
     g_string_free (change_text_gs, TRUE);
@@ -2030,7 +2004,14 @@ gnucash_sheet_commit_cb (GtkIMContext *context, const gchar *str,
               gtk_editable_get_position (editable)
               : sheet->preedit_start_position;
     gtk_editable_insert_text (editable, str, strlen (str), &tmp_pos);
+
+    /* insert_cb may have changed the selection, but gtk_editable_set_position
+       (erroneously?) clears it.  If a selection is set, preserve it. */
+    gtk_editable_get_selection_bounds (editable, &sel_start, &sel_end);
     gtk_editable_set_position (editable, tmp_pos);
+    if (sel_start != sel_end)
+	gtk_editable_select_region (editable, sel_start, sel_end);
+
     gnucash_sheet_im_context_reset_flags(sheet);
 }
 
-- 
1.7.4.1