Lost input in quickfill cells due to race

[Christian Stimming]

Am Dienstag, 24. Mai 2011 schrieb Jim Paris:
> > 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.

Thanks a lot! I've committed this to SVN and we'll see whether anyone sees new 
problems, but the code looks very good to me. Thanks again!

Christian


> 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);
>  }