Race condition with name widget in the activity toolbar

[bernie]

How to reproduce:

1. open any activity (for example, Write)
2. switch to the Activity toolbar
3. click on the name widget
4. quickly type a few keystrokes
5. after 1-2 seconds, some of your keystrokes will disappear and the cursor will be reset to the beginning of the edit box

Even without looking at the code, this smells like a simple race condition between the user and the callback function that updates the datastore on every change. If so, we could simply work around this bug by hooking the update to a different gtk event, such as when the widget looses focus.

[bernie]

Does not affect 0.88.

[bernie]

Proposed fix attached.

[bernie]

I'll work on a patch for mainline too, even though the bug can't be triggered there.

[sayamindu]

Looks good. Any ideas why this is not appearing in newer releases ?

[bernie]

Replying to sayamindu:

Looks good. Any ideas why this is not appearing in newer releases ?

The code in 0.88 has been completely restructured and as a result the bug has become latent or at least harder to trigger.

I'll leave this bug open as a reminder that we have to port this patch forward to master.

[alsroot]

Last patch is for 0.86+ code base

[erikos]

As it was even backported to 0.84...

[erikos]

Aleksey, thanks for your patch. It does fix the behavior that Bernie describes indeed (even with stress testing I was only able to reproduce once). And it is not invasive, too. 2 points out of 2 -> Please push.

[erikos]

Bernie, there is an issue with your patch. Using the focus out event is critical when the kid does the following: open activity, change title, close activity. The focus out event will not be triggered.

Why not use the patch from Aleksey in 0.84, too? Did t land already?

[alsroot]

trunk commit
 http://git.sugarlabs.org/projects/sugar-toolkit/repos/mainline/commits/845d2534e670366a92fd93be92a078a4a188ff9f

closing but setting olpc-0.84 to let forget about it for 0.84

[alsroot]

Replying to alsroot:

trunk commit
 http://git.sugarlabs.org/projects/sugar-toolkit/repos/mainline/commits/845d2534e670366a92fd93be92a078a4a188ff9f

closing but setting olpc-0.84 to let forget about it for 0.84

..to not forget

[bernie]

Replying to erikos:

Bernie, there is an issue with your patch. Using the focus out event is critical when the kid does the following: open activity, change title, close activity. The focus out event will not be triggered.

Ouch, this is true.

On the other hand, I think that using a 1-second timeout to save also opens a race-condition: change the name, then quickly press ctrl-q before the timeout expires.

The timeout approach also results in spurious saves which make the activity hang for a while if the user isn't fast at typing.

Isn't there another event which is guaranteed to fire when the user finishes editing a textfield? If not, couldn't we rework my patch add an extra check on activity close time?

Why not use the patch from Aleksey in 0.84, too? Did t land already?

I not planning any new releases of Sugar 0.84 for the XO-1. Perhaps OLPC might look into it for the XO-1.5.

[erikos]

Hmmm, tricky.

Of course the 1-second timeout opens a race when saving. But it is more unlikely the user will hit it than with the focus out event. I checked, I could not find any other signal that would help us here. Etoys waits for the 'enter' key to be pressed before the title is saved (which is the general behavior in Etoys), for the kids in my class this behavior is hard to get.

In general we should try to be consistent. As we use timeouts at other places, we should not change behavior here. I would vote for going back to the timeout behavior with the fix from Aleksey and then think how we want to handle it in the future.

[bernie]

Replying to erikos:

Hmmm, tricky.

Of course the 1-second timeout opens a race when saving. But it is more unlikely the user will hit it than with the focus out event. I checked, I could not find any other signal that would help us here.

I tried to solve the issue by adding a check in the stop button, but it's not clear how I could reach the new activity title stored in the other widget.

Binding the two controls together seems overkill. Isn't there a way to make a widget "modal"? That is, prevent users from clicking on other widgets as long as it has focus.

[erikos]

Bernie, I think this is what you had in mind.

diff --git a/src/sugar/activity/activity.py b/src/sugar/activity/activity.py
index 3e97485..7aa5c64 100644
--- a/src/sugar/activity/activity.py
+++ b/src/sugar/activity/activity.py
@@ -176,6 +176,7 @@ class ActivityToolbar(gtk.Toolbar):
         self._activity.copy()
 
     def __stop_clicked_cb(self, button):
+        self._update_title(self.title.get_text())
         self._activity.close()
 
     def __jobject_updated_cb(self, jobject):
@@ -183,7 +184,9 @@ class ActivityToolbar(gtk.Toolbar):
 
     def __title_changed_cb(self, editable, event):
         title = editable.get_text()
+        self._update_title(title)
 
+    def _update_title(self, title):
         # Title really changed?
         if title == self._activity.metadata['title']:
             return False

[erikos]

save on stop button - disconnect the focus-out handler

[erikos]

I tested if the focus-out event is propagated. When hitting stop the focus-out is emitted, though we do save first. I have added now a line that just saves the title when hitting stop. Furthermore the focus-out-event handler is disconnected to not receive events when the activity is deconstructed (can be seen in current code).

[sascha_silbe]

erikos, I suppose you wanted this to get reviewed.

[erikos]

Actually, the patch is against 0.84, so no need to review it here, as the code in master is different. I actually even think we can close this ticket, if nobody has a better solution to the issue than what we do now in Sugar.

[alsroot]

Regardless of all posted patches, #2608 fixes the issue with, only, moving cursor to the beginning on gtk.Entry.

[sascha_silbe]

Seen again today on latest mainline/master. It doesn't overwrite characters because the selection isn't reset, but newly typed characters appear on the front instead of where the cursor was last positioned.