Lost input in quickfill cells due to race

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

Lost input in quickfill cells due to race

Jim Paris
Hi,

If I type quickly into a quickfill cell, some of my input is lost.
It's especially pronounced when X-forwarding over a slower link, but
I've seen it on fast links and even locally.  I've tracked it down to
a race condition in src/register/register-gnome/gnucash-sheet.c:

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

This sets up a short-lived timer that will change the selection.
However, other keypresses can show up in the GTK event queue before
the timeout expires.

The attached patch adds debugging that shows the race.  I'll type
"asdf" into a cell that quickfills "asdf".  If I type very slowly,
the debugging output shows:

gnucash_sheet_insert_cb: insert_text='a', position=0
  scheduling timeout, start_sel=1, end_sel=-1
gnucash_sheet_select_data_cb: start_sel=1, end_sel=-1
gnucash_sheet_insert_cb: insert_text='s', position=1
  scheduling timeout, start_sel=2, end_sel=-1
gnucash_sheet_select_data_cb: start_sel=2, end_sel=-1
gnucash_sheet_insert_cb: insert_text='d', position=2
  scheduling timeout, start_sel=3, end_sel=-1
gnucash_sheet_select_data_cb: start_sel=3, end_sel=-1
gnucash_sheet_insert_cb: insert_text='f', position=3
  scheduling timeout, start_sel=4, end_sel=-1
gnucash_sheet_select_data_cb: start_sel=4, end_sel=-1

and the cell contains "asdf" as expected. If I instead type it quickly,

gnucash_sheet_insert_cb: insert_text='a', position=0
  scheduling timeout, start_sel=1, end_sel=-1
gnucash_sheet_insert_cb: insert_text='s', position=1
gnucash_sheet_insert_cb: insert_text='d', position=2
gnucash_sheet_select_data_cb: start_sel=1, end_sel=-1
gnucash_sheet_insert_cb: insert_text='f', position=1

Note how the 's' and 'd' arrived before the selection callback.  The
wrong text was selected, and the final 'f' overwrote the entire
selection.  The cell contains just "af" after this.

Setting the selection can't be done asynchronously like this.
I'll see if I can come up with a clean fix...

-jim


commit b94910babbe66350ad455dcd5a8074298c873dfc
Author: Jim Paris <[hidden email]>
Date:   Mon May 23 16:53:59 2011 -0400

    Add debugging for keyboard input issues

diff --git a/src/register/register-gnome/gnucash-sheet.c b/src/register/register-gnome/gnucash-sheet.c
index 3d0d6b9..1137a55 100644
--- a/src/register/register-gnome/gnucash-sheet.c
+++ b/src/register/register-gnome/gnucash-sheet.c
@@ -854,6 +854,8 @@ typedef struct
 static gboolean
 gnucash_sheet_select_data_cb (select_info *info)
 {
+    printf("gnucash_sheet_select_data_cb: start_sel=%d, end_sel=%d\n",
+   info->start_sel, info->end_sel);
     gtk_editable_select_region (info->editable,
                                 info->start_sel, info->end_sel);
     g_free(info);
@@ -889,6 +891,9 @@ gnucash_sheet_insert_cb (GtkWidget *widget,
     const char *c;
     gunichar uc;
 
+    printf("gnucash_sheet_insert_cb: insert_text='%s', position=%d\n",
+   insert_text, *position);
+
     if (sheet->input_cancelled)
     {
         g_signal_stop_emission_by_name (G_OBJECT (sheet->entry),
@@ -1006,6 +1011,8 @@ gnucash_sheet_insert_cb (GtkWidget *widget,
         info->editable = editable;
         info->start_sel = start_sel;
         info->end_sel = end_sel;
+ printf("  scheduling timeout, start_sel=%d, end_sel=%d\n",
+       info->start_sel, info->end_sel);
         g_timeout_add(/*ASAP*/ 1,
                                (GSourceFunc)gnucash_sheet_select_data_cb,
                                info);

_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: Lost input in quickfill cells due to race

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?

-jim
_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: Lost input in quickfill cells due to race

Derek Atkins
Jim Paris <[hidden email]> writes:

> 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?

Work on the "register-rewrite" branch that should be using the
GtkEntryCompletion?

> -jim

-derek
--
       Derek Atkins, SB '93 MIT EE, SM '95 MIT Media Laboratory
       Member, MIT Student Information Processing Board  (SIPB)
       URL: http://web.mit.edu/warlord/    PP-ASEL-IA     N1NWH
       [hidden email]                        PGP key available
_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: Lost input in quickfill cells due to race

Christian Stimming-4
In reply to this post by Jim Paris
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.

Regards,

Christian
_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: 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 <[hidden email]>
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

_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: Lost input in quickfill cells due to race

Christian Stimming-4
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 <[hidden email]>
> 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);
>  }

_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: Lost input in quickfill cells due to race

Christian Stimming-4
In reply to this post by Jim Paris
Dear Jim,

in May you sent us a very useful patch to get rid of lost characters during
auto-completion ("quickfill") of text input in the account register window.

However, subsequently I noticed a change in behaviour of the register auto-
completion. Namely, during typing sometimes the cursor jumps to the end of the
selection where instead it previously stayed inside the selection, or more
precisely, when typing a quick-filled word character by character, the cursor
previously stayed right after the character that was just typed, but now it
occasionally jumps to the end of the selection. In effect, when typing
"Groceries", I type "G r o", but after the "G r" the cursor jumped to the end
of the selection and I end up with the word "Grocerieso", which is annoying.

Reverting your patch from May (r20689) will make this behaviour go away again.
This is Ubuntu 10.10 with gtk-2.20.1. I don't want to revert your patch right
away as I'm sure you had some other bugs that went away by your patch, but
this particular other issue was introduced by it. (Unfortunately, it also went
into the stable 2.4.6 already, so it needs to be fixed there as well...)

Do you have any ideas here?

Regards,

Christian


Am Dienstag, 24. Mai 2011 schrieb Jim Paris:

> 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.
_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: Lost input in quickfill cells due to race

Herbert Thoma-2
On 18.07.2011 16:43, Christian Stimming wrote:
> Reverting your patch from May (r20689) will make this behaviour go away again.
> This is Ubuntu 10.10 with gtk-2.20.1. I don't want to revert your patch right
> away as I'm sure you had some other bugs that went away by your patch, but
> this particular other issue was introduced by it. (Unfortunately, it also went
> into the stable 2.4.6 already, so it needs to be fixed there as well...)

I can confirm that this bug also occurs on openSUSE 11.4 with gtk-2.22.1

> Do you have any ideas here?
>
> Regards,
>
> Christian
>
>
> Am Dienstag, 24. Mai 2011 schrieb Jim Paris:
>> 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.
> _______________________________________________
> gnucash-devel mailing list
> [hidden email]
> https://lists.gnucash.org/mailman/listinfo/gnucash-devel
>

--
Herbert Thoma
Dipl.-Ing., MBA
Head of Video Group
Multimedia Realtime Systems Department
Fraunhofer IIS
Am Wolfsmantel 33, 91058 Erlangen, Germany
Phone: +49-9131-776-6130
Fax:   +49-9131-776-6099
email: [hidden email]
www: http://www.iis.fhg.de/
_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: Lost input in quickfill cells due to race

Jim Paris
Christian Stimming wrote:

> in May you sent us a very useful patch to get rid of lost characters during
> auto-completion ("quickfill") of text input in the account register window.
>
> However, subsequently I noticed a change in behaviour of the register auto-
> completion. Namely, during typing sometimes the cursor jumps to the end of the
> selection where instead it previously stayed inside the selection, or more
> precisely, when typing a quick-filled word character by character, the cursor
> previously stayed right after the character that was just typed, but now it
> occasionally jumps to the end of the selection. In effect, when typing
> "Groceries", I type "G r o", but after the "G r" the cursor jumped to the end
> of the selection and I end up with the word "Grocerieso", which is annoying.
>
> Reverting your patch from May (r20689) will make this behaviour go away again.
> This is Ubuntu 10.10 with gtk-2.20.1. I don't want to revert your patch right
> away as I'm sure you had some other bugs that went away by your patch, but
> this particular other issue was introduced by it. (Unfortunately, it also went
> into the stable 2.4.6 already, so it needs to be fixed there as well...)
>
> Do you have any ideas here?

Hi Christian,

I'm having a hard time reproducing it here.  I installed Ubuntu 10.10,
which comes with a different version of gtk:

  $ lsb_release -d
  Description:    Ubuntu 10.10
  $ pkg-config gtk+-2.0 --modversion
  2.22.0
  $ gnucash --version
  GnuCash 2.4.7 development version
  Built 2011-07-18 from r3675a6f+

I typed "groceries" in a new register a couple dozen times, and had no
lost or misplaced characters.  I tried it both locally and x-forwarded.

Might it be triggered by some interaction with input methods?  I tried
using some, but it didn't seem to cause any problems.

Does it happen every time?  If not, how easy is it to reproduce?  This
change _should_ have gotten rid of any potential for race conditions,
but maybe there's something still there.  I don't see where that would
be happening, though.

(Herbert Thoma said it also happened on gtk-2.22.1, so I don't think
my newer gtk is the issue)

-jim
_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel