Opened 7 years ago

Closed 6 years ago

Last modified 5 years ago

#6014 closed defect (fixed)

Shutdown should sync activities data

Reported by: michael.tiemann Owned by: marco
Priority: blocker Milestone: 8.2.0 (was Update.2)
Component: sugar Version: Build 653
Keywords: relnote 8.2.0:+ joyride-2220:+ r+ Cc: cjb, cscott, gregorio
Blocked By: #7574 Blocking:
Deployments affected: Action Needed: finalize
Verified: no

Description

My daughter was using her XO on a long trip and typing pages of a story into the text editor activity. After a while she decided to rest, and she shutdown the computer as we did not have car adaptor to keep the XO plugged in. When she rebooted some time later, she observed that her changes from the last session were all lost.

I understand that if she had closed the activities first, before issuing a shutdown command, the activities would have prompted her to save her work. The system-wide shutdown should cause the various activities to run their shutdown processes, which would then prompt for saves and avoid lost work. I understand that sending a dbus message would accomplish this.

Attachments (3)

sync_data.patch (3.5 KB) - added by erikos 7 years ago.
Sync the data when reboot/shutdown and suspend
sm.patch (4.6 KB) - added by marco 6 years ago.
Patch that should work but doesn't for weird reasons
sm2.patch (4.7 KB) - added by marco 6 years ago.
Working patch

Download all attachments as: .zip

Change History (34)

comment:1 Changed 7 years ago by morgs

  • Component changed from dbus-python to sugar
  • Milestone changed from Update.1 to Never Assigned
  • Owner changed from Collabora to marco
  • Summary changed from Laptop shutdown should send dbus message to all open applications to Shutdown should close Activities

Though D-Bus is mentioned, this isn't a bug in dbus-python but rather a system issue, probably involving Sugar and ohm.

comment:2 Changed 7 years ago by jg

  • Cc cjb added
  • Keywords text editor abiword shutdown lost changes removed
  • Milestone changed from Never Assigned to Future Release

Ohm and sugar should cooperate in shutting down cleanly on low power....

comment:3 Changed 7 years ago by erikos

  • Owner changed from marco to erikos

The attached patch adds the dbus method 'SyncData' to the activity service so that we can sync the data to the datastore when we reboot or shutdown. This method is called for all the running activities when you use the palette in the home view to reboot or shutdown.

To verify:

  • open an activity like calculate and type some equations
  • reboot or shutdown (without changig the active activity since this will sync the data as well)
  • resume the activity and check that the data is there

Furthermore the 'SyncActivityData' dbus method is added to the shell service so that ohm can call it to sync the data of all the running activities when going to suspend.

Changed 7 years ago by erikos

Sync the data when reboot/shutdown and suspend

comment:4 Changed 7 years ago by cjb

Looks good to me. Reassign to me once the patch is reviewed and in sugar, and I'll add to OHM.

comment:5 Changed 7 years ago by erikos

  • Keywords review? added

Shouldn't we get this into update.1, Jim?

comment:6 Changed 7 years ago by erikos

  • Milestone changed from Future Release to Retriage, Please!
  • Summary changed from Shutdown should close Activities to Shutdown should sync activities data

Loosing data is bad. Maybe we should at least get the fix in for the shutdown/reboot case. This is sugar only.

comment:7 Changed 7 years ago by marco

I think we probably want to reuse some standard way to do this, rather then providing our own interface.

comment:8 Changed 7 years ago by jg

  • Keywords relnote added; review? removed
  • Milestone changed from Retriage, Please! to Future Release

I agree with Marco.

There are existing desktop conventions for notifying applications their session is shutting down. We should not invent our own...

comment:9 Changed 7 years ago by cscott

  • Blocked By 4877 added
  • Cc cscott added

See:
http://www.news.com/8301-10784_3-9859801-7.html?part=rss&subj=news&tag=2547-1_3-0-20

I think Update.2 is a reasonable target; there seems to be some disagreement on how best to do this. olpc-update will need to be patched as well; see http://dev.laptop.org/git?p=users/cscott/olpc-update;a=blob;f=bitfrost/update/setup.py;h=772f97e82c1eef1f3b2c593a174e5874497aa7c9;hb=HEAD#l258

Trac #4877 and #4878 are related.

comment:10 Changed 6 years ago by marco

  • Keywords 8.2.0:+ added
  • Milestone changed from Future Release to 8.2.0 (was Update.2)

comment:11 Changed 6 years ago by marco

  • Owner changed from erikos to marco

comment:12 Changed 6 years ago by cjb

  • Action Needed set to never set

Hi Marco,

It sounds like this is implemented, but we aren't yet triggering it before shutting down due to low power. Could you tell me how I can make the state-saving call over dbus from OHM, so that I can add it to a trigger for when we're at critical battery life?

Thanks!

comment:13 Changed 6 years ago by gregorio

  • Cc gregorio added
  • Priority changed from high to blocker

Is this fixed?

Can you update the action needed field and whatever else makes sense to make sure we track this for 8.2.0?

I upped the priority to blocker but I could be convinced to lower it if this is hard.

Thanks,

Greg S

comment:14 Changed 6 years ago by marco

  • Action Needed changed from never set to test in build

It's on my radar. I'll try to look at it over the weekend.

comment:15 Changed 6 years ago by marco

  • Action Needed changed from test in build to code

Code needs a bit of love. Save is async and we need to ensure it's finished before we allow session shutdown. Should be easy.

We also need a rainbow fix for this work, Tomeu is opening a separate ticket which will block this one.

comment:16 Changed 6 years ago by tomeu

  • Blocked By 7574 added

Changed 6 years ago by marco

Patch that should work but doesn't for weird reasons

Changed 6 years ago by marco

Working patch

comment:17 Changed 6 years ago by marco

  • Keywords 8.2.0:? r? added; 8.2.0:+ removed

This needs careful review and testing.

comment:18 Changed 6 years ago by marco

|Testcase|

Start Write. Type some text. Go back to the home view, hover the XO and click "Reboot". When Sugar is back, go to the journal and open the last Write document. Make sure the text you typed is there.

comment:19 Changed 6 years ago by erikos

Test result: Applied the sugar patch and the rainbow one and the data was saved and accessible after reboot. Tested with write and browse and tested that there were no errors introduced to save in general with the patch.

comment:20 Changed 6 years ago by erikos

  • Action Needed changed from code to review
  • Keywords r+ added; r? removed

Code looks good.

comment:21 Changed 6 years ago by erikos

  • Action Needed changed from review to package

We must make sure that #7574 is in when we want this to work.

comment:22 Changed 6 years ago by marco

  • Action Needed changed from package to finalize
  • Keywords 8.2.0:+ joyride-2220:+ added; 8.2.0:? removed

comment:23 follow-up: Changed 6 years ago by cscott

Just so I'm sure I understand this correctly: this ensures that data is sync'ed when you shutdown from the home view, but doesn't make 'shutdown -h now' from the terminal sync data. Could this be fixed by just adding a simple python script to the initscripts to invoke 'syncData' on shutdown? This might cause us to sync data twice if you shutdown from the home view -- is that a problem?

For context, I'm trying to fix #4878 now that the blocker is gone. Should I be doing this by making 'shutdown' safe, or should olpc-update explicitly call sugar's syncData before it invokes /sbin/shutdown?

comment:24 in reply to: ↑ 23 ; follow-up: Changed 6 years ago by marco

Replying to cscott:

Just so I'm sure I understand this correctly: this ensures that data is sync'ed when you shutdown from the home view

Correct. This does basically the same gnome-session does in GNOME (we reuse a lot of that code too).

but doesn't make 'shutdown -h now' from the terminal sync data.

Correct.

Could this be fixed by just adding a simple python script to the initscripts to invoke 'syncData' on shutdown?

Not sure. A tricky aspect is that activities can block shutdown, so the script would have to deal with it.

This might cause us to sync data twice if you shutdown from the home view -- is that a problem?

I'd rather not save twice. It can be expensive for big files (and we don't have a dirty flag in activities at the moment).

For context, I'm trying to fix #4878 now that the blocker is gone. Should I be doing this by making 'shutdown' safe, or should olpc-update explicitly call sugar's syncData before it invokes /sbin/shutdown?

sugar/src/session.py should expose a DBus service (maybe with the same interface as GNOME session http://svn.gnome.org/viewvc/gnome-session/trunk/gnome-session/org.gnome.SessionManagement.xml?revision=4777&view=markup). And I tend to think olpc-update should call the Shutdown method on it directly.

comment:25 Changed 6 years ago by gregorio

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

comment:26 in reply to: ↑ 24 ; follow-up: Changed 6 years ago by cscott

Replying to marco:

sugar/src/session.py should expose a DBus service (maybe with the same interface as GNOME session http://svn.gnome.org/viewvc/gnome-session/trunk/gnome-session/org.gnome.SessionManagement.xml?revision=4777&view=markup). And I tend to think olpc-update should call the Shutdown method on it directly.

"should expose" means this isn't implemented yet, right? If not, then #4877 shouldn't be closed, since that's what that bug was all about.

comment:27 Changed 6 years ago by cjb

  • Resolution fixed deleted
  • Status changed from closed to reopened

Agreed, OHM needs this exposed too before the original filer's bug will be addressed.

comment:28 Changed 6 years ago by gnu

The original poster (my friend Mike Tiemann) said his daughter "shutdown the computer" -- not that it ran out of battery. See top of this page. Thus his particular bug appears to be fixed.

I agree that it's a good idea for the EC, Ohm, and Sugar to cooperate to cleanly shut down a machine that's about to lose power anyway. But that's a different bug -- #4824, I believe.

comment:29 in reply to: ↑ 26 Changed 6 years ago by marco

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

Replying to cscott:

"should expose" means this isn't implemented yet, right?

Correct.

If not, then #4877 shouldn't be closed, since that's what that bug was all about.

Ok, sorry. It was not very clear from the description, which only talked about saving activity state (which is actually implemented). I'll reopen it.

This bug should stay closed though. It's about saving data on explicit shutdown, which is fixed.

comment:30 Changed 6 years ago by michael.tiemann

I agree that this is about saving data in the case of explicit shutdown, not saving data in the face of arbitrary lossage.

BTW, thanks for all the effort on this. I showed the trail to my daughter just last night, and she was impressed. I look forward to loading Update.2 when it is ready.

comment:31 Changed 6 years ago by marco

  • Blocked By 4877 removed

(In #4877) cjb, #6014 is about explicit shutdown, see description.

I'm delaying this to 9.1.0, unless one of the bugs it blocks is made a release blocker.

Note: See TracTickets for help on using tickets.