Opened 9 years ago

Closed 6 years ago

Last modified 6 years ago

#1948 closed defect (fixed)

Race condition with name widget in the activity toolbar

Reported by: bernie Owned by: erikos
Priority: Unspecified by Maintainer Milestone:
Component: Sugar Version: Git as of bugdate
Severity: Major Keywords: olpc-0.84
Cc: richar.saucedo@…, sayamindu, sascha_silbe, tonyforster Distribution/OS: Unspecified
Bug Status: New

Description

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.

Attachments (3)

0001-sl-1948-Race-condition-with-name-widget-in-the-activ.patch (2.6 KB) - added by bernie 9 years ago.
0001-Reset-timeout-delay-on-every-TitleEntry-change-1948.patch (1.0 KB) - added by alsroot 9 years ago.
0001-Save-title-when-closing-1948.patch (2.0 KB) - added by erikos 9 years ago.
save on stop button - disconnect the focus-out handler

Download all attachments as: .zip

Change History (31)

comment:1 Changed 9 years ago by sascha_silbe

  • Cc sascha_silbe added

comment:2 Changed 9 years ago by sascha_silbe

  • Bug Status changed from Unconfirmed to New

comment:3 Changed 9 years ago by bernie

  • Owner changed from tomeu to sayamindu
  • Status changed from new to assigned

Does not affect 0.88.

comment:4 Changed 9 years ago by bernie

  • Keywords r? olpc-0.84 added

Proposed fix attached.

comment:5 Changed 9 years ago by bernie

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

comment:6 follow-up: Changed 9 years ago by sayamindu

  • Keywords r+ added; r? removed

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

comment:7 in reply to: ↑ 6 Changed 9 years ago by bernie

  • Keywords olpc-0.84 removed
  • Version changed from 0.84.x to Git as of bugdate

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.

comment:8 Changed 9 years ago by alsroot

  • Component changed from sugar to sugar-toolkit
  • Keywords r? added; r+ removed
  • Resolution set to fixed
  • Status changed from assigned to closed
  • Version changed from Git as of bugdate to 0.88.x

Last patch is for 0.86+ code base

comment:9 Changed 9 years ago by alsroot

  • Resolution fixed deleted
  • Status changed from closed to reopened

comment:10 Changed 9 years ago by alsroot

  • Owner changed from sayamindu to erikos
  • Status changed from reopened to assigned

comment:11 Changed 9 years ago by erikos

  • Milestone changed from Unspecified by Release Team to 0.88.x

As it was even backported to 0.84...

comment:12 Changed 9 years ago by erikos

  • Keywords r+ added; r? removed

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.

comment:13 Changed 9 years ago by 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?

comment:14 follow-up: Changed 9 years ago by alsroot

  • Keywords olpc-0.84 added; r+ removed
  • Resolution set to fixed
  • Status changed from assigned to closed

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

comment:15 in reply to: ↑ 14 Changed 9 years ago by 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

comment:16 Changed 9 years ago by bernie

  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

comment:17 follow-up: Changed 9 years ago by 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.

comment:18 in reply to: ↑ 17 Changed 9 years ago by 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.

comment:19 Changed 9 years ago by 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

Changed 9 years ago by erikos

save on stop button - disconnect the focus-out handler

comment:20 Changed 9 years ago by 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).

comment:21 Changed 9 years ago by sascha_silbe

  • Keywords r? added

erikos, I suppose you wanted this to get reviewed.

comment:22 Changed 9 years ago by erikos

  • Keywords r? removed

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.

comment:23 Changed 8 years ago by tonyforster

  • Cc tonyforster added

comment:24 Changed 8 years ago by alsroot

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

comment:25 Changed 8 years ago by sascha_silbe

  • Milestone changed from 0.88.x to 0.92
  • Severity changed from Critical to Major
  • Version changed from 0.88.x to Git as of bugdate

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.

comment:26 Changed 6 years ago by godiard

  • Resolution set to fixed
  • Status changed from reopened to closed

I can't reproduce this issue in sugar 0.98

Closing as fixed

comment:27 Changed 6 years ago by dnarvaez

  • Component changed from sugar-toolkit to Sugar

comment:28 Changed 6 years ago by dnarvaez

  • Milestone 0.92 deleted

Milestone 0.92 deleted

Note: See TracTickets for help on using tickets.