Bug 219173 - [context][externalization] auto save task context
Summary: [context][externalization] auto save task context
Status: RESOLVED FIXED
Alias: None
Product: z_Archived
Classification: Eclipse Foundation
Component: Mylyn (show other bugs)
Version: 2.2   Edit
Hardware: PC Linux
: P1 enhancement with 1 vote (vote)
Target Milestone: 3.3   Edit
Assignee: Shawn Minto CLA
QA Contact:
URL:
Whiteboard:
Keywords: plan
Depends on:
Blocks:
 
Reported: 2008-02-15 16:11 EST by George Lindholm CLA
Modified: 2009-10-08 15:58 EDT (History)
4 users (show)

See Also:


Attachments
patch (17.13 KB, patch)
2009-09-30 16:37 EDT, Shawn Minto CLA
no flags Details | Diff
mylyn/context/zip (8.58 KB, application/octet-stream)
2009-09-30 16:37 EDT, Shawn Minto CLA
no flags Details
mylyn/context/zip (19.75 KB, application/octet-stream)
2009-10-07 18:14 EDT, Shawn Minto CLA
no flags Details
updated patch with steffens comments (15.61 KB, patch)
2009-10-07 18:14 EDT, Shawn Minto CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description George Lindholm CLA 2008-02-15 16:11:59 EST
I'm finding that when I restart Eclipse after a crash and I had an active task,
that mylyn has forgotten about files that I've been working with before the crash.
Some appear in the context, but are not automatically re-opened, while
othere are just not there.
Comment 1 Steffen Pingel CLA 2008-02-16 14:07:18 EST
This has happened to me as well. Contexts are only saved on task deactivation. Rob, is there another bug that tracks more frequent saving of contexts?
Comment 2 Mik Kersten CLA 2008-02-19 02:56:44 EST
I don't think that we have another bug for this.  Should we implement a save policy, e.g. auto-save every 30 or 60 seconds when active?
Comment 3 Steffen Pingel CLA 2008-02-19 03:06:24 EST
+1 if the the auto-save job can be "smart", i.e. not save the context if there haven't been any changes
Comment 4 George Lindholm CLA 2008-02-19 21:20:26 EST
I think the context should also be updated when a "resource" is
added or removed.
Comment 5 Mik Kersten CLA 2008-02-20 00:54:04 EST
That seems like a great approach.  We could trigger a save after every n'th interaction event processed.  Similar to what Steffen told me today about Emacs (does saves on every 200th character event processed).
Comment 6 Steffen Pingel CLA 2008-02-23 16:43:09 EST
Rob, assigning to you since this is related to scheduling save jobs and resource locking.
Comment 7 Steffen Pingel CLA 2008-06-05 18:38:38 EDT
*** Bug 235860 has been marked as a duplicate of this bug. ***
Comment 8 Robert Elves CLA 2008-06-05 20:00:22 EDT
Unfortunately this will not get completed for 3.1. We'll investigate extraction of context externalization code for 3.1.
Comment 9 Mik Kersten CLA 2009-09-24 13:55:21 EDT
We may need to give this its own timer if we can't easily generalize the externalization framework.
Comment 10 Shawn Minto CLA 2009-09-30 16:37:15 EDT
Created attachment 148471 [details]
patch

Here is the first cut at a patch that will autosave the active context every 3-5 minutes if the user is active, or when they become inactive.  I had to add in a way to collapse and copy the context so that it could be externalized asynchronously.  Steffen, could you take a look at this to make sure I didn't miss any weird synchronization issues that I should deal with, or if you have any suggestions on how this could be improved?
Comment 11 Shawn Minto CLA 2009-09-30 16:37:35 EDT
Created attachment 148472 [details]
mylyn/context/zip
Comment 12 Steffen Pingel CLA 2009-10-07 16:59:46 EDT
The patch looks good to me. A few comments:

Can you add a few comments to ActiveContextExternalizationParticipant? We need to document that it is only responsible for saving periodically and everything else is handled by InteractionContextManager.

Why are these null checks ContextCorePlugin.getDefault() and MonitorUiPlugin.getDefault() necessary?

ActiveContextExternalizationParticipant.shouldWriteContext() does not seem to be fully implemented. Can't you simply add a version field to InteractionContext that is incremented each time there is a change?

Why is the entire LocalContextStore.saveContext() method synchronized? That seems unnecessary and dangerous since it calls out to other classes while holding the lock. Can't we limit the lock to the actual copying and persisting of the context?

What I find weird in the context architecture is that DegreeOfInterest holds on to events. Shouldn't storing those be be limited to InteractionContext?
Comment 13 Shawn Minto CLA 2009-10-07 18:13:59 EDT
> Can you add a few comments to ActiveContextExternalizationParticipant? We need
> to document that it is only responsible for saving periodically and everything
> else is handled by InteractionContextManager.

Done.
 
> Why are these null checks ContextCorePlugin.getDefault() and
> MonitorUiPlugin.getDefault() necessary?

I dont think that they are necessary.

> ActiveContextExternalizationParticipant.shouldWriteContext() does not seem to
> be fully implemented. Can't you simply add a version field to
> InteractionContext that is incremented each time there is a change?

Good catch on the TODO.  It is only there since it could be nice, but the context is only written if the user is active, which in general would mean that the context has changed.

> Why is the entire LocalContextStore.saveContext() method synchronized? That
> seems unnecessary and dangerous since it calls out to other classes while
> holding the lock. Can't we limit the lock to the actual copying and persisting
> of the context?

I moved the synchronized block to be just around the copying and writing.
 
> What I find weird in the context architecture is that DegreeOfInterest holds on
> to events. Shouldn't storing those be be limited to InteractionContext?

Probably.  I will create a new bug for this.
Comment 14 Shawn Minto CLA 2009-10-07 18:14:03 EDT
Created attachment 149061 [details]
mylyn/context/zip
Comment 15 Shawn Minto CLA 2009-10-07 18:14:24 EDT
Created attachment 149062 [details]
updated patch with steffens comments

Patch committed.
Comment 16 Steffen Pingel CLA 2009-10-07 21:06:27 EDT
I am getting this exception in my log on startup:

java.lang.Exception
at org.eclipse.mylyn.internal.tasks.ui.ActiveContextExternalizationParticipant.registerListeners(ActiveContextExternalizationParticipant.java:82)
at org.eclipse.mylyn.internal.tasks.ui.TasksUiPlugin$TasksUiInitializationJob.runInUIThread(TasksUiPlugin.java:443)
at org.eclipse.ui.progress.UIJob$1.run(UIJob.java:95)
at org.eclipse.swt.widgets.RunnableLock.run(RunnableLock.java:35)
at org.eclipse.swt.widgets.Synchronizer.runAsyncMessages(Synchronizer.java:134)
at org.eclipse.swt.widgets.Display.runAsyncMessages(Display.java:3468)
at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3115)
at org.eclipse.ui.internal.Workbench.runEventLoop(Workbench.java:2405)
at org.eclipse.ui.internal.Workbench.runUI(Workbench.java:2369)
at org.eclipse.ui.internal.Workbench.access$4(Workbench.java:2221)
at org.eclipse.ui.internal.Workbench$5.run(Workbench.java:500)
at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:332)
at org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Workbench.java:493)
at org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:149)
at org.eclipse.ui.internal.ide.application.IDEApplication.start(IDEApplication.java:113)
at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:194)
at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:110)
at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:79)
at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:368)
at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:179)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
at java.lang.reflect.Method.invoke(Method.java:597)
at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:559)
at org.eclipse.equinox.launcher.Main.basicRun(Main.java:514)
at org.eclipse.equinox.launcher.Main.run(Main.java:1311)
at org.eclipse.equinox.launcher.Main.main(Main.java:1287)
Comment 17 Shawn Minto CLA 2009-10-08 11:28:33 EDT
It looks like the fix on bug 291237 has fixed this exception.  I don't remember why I had this logging in there, should I just remove it?
Comment 18 Steffen Pingel CLA 2009-10-08 14:16:48 EDT
Yes, we should be able to remove the check now.
Comment 19 Shawn Minto CLA 2009-10-08 15:58:23 EDT
I have removed the check now.