Bug 160729 - [ltk] - Undoing a resource delete does not restore session and persistent properties
Summary: [ltk] - Undoing a resource delete does not restore session and persistent pro...
Status: RESOLVED WONTFIX
Alias: None
Product: JDT
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 3.2   Edit
Hardware: All All
: P5 normal with 1 vote (vote)
Target Milestone: ---   Edit
Assignee: JDT-UI-Inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords:
: 214992 (view as bug list)
Depends on: 216884
Blocks:
  Show dependency tree
 
Reported: 2006-10-12 14:37 EDT by Susan McCourt CLA
Modified: 2019-12-22 08:57 EST (History)
7 users (show)

See Also:


Attachments
Patch for I20071213-1700 (2.27 KB, patch)
2008-01-29 05:34 EST, Francis Upton IV CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Susan McCourt CLA 2006-10-12 14:37:05 EDT
When a resource is deleted, the DeleteResourcesOperation stores a description of the resource so it can be later restored.  There is currently no way in the IResource API to enumerage what session and persistent properties are available, so that they can be remembered and restored.

Further, it's not clear whether this is a good idea, since these arbitrary properties could be large.

What is the best strategy?
Comment 1 Susan McCourt CLA 2006-10-12 14:38:57 EDT
Discussed this with JohnA, who suggests:

>I'm not sure if it's necessary to incorporate this information into >undo/redo.  The kind of information stored in these properties typically >isn't information that the user has any way of knowing about - it's just >arbitrary metadata associated with the resource that we're holding on behalf >on some plugin.  Typically this is cache-like information that can be >recomputed if it's lost. I'd suggest leaving this out for now and see if it >becomes an issue.


Comment 2 Susan McCourt CLA 2006-10-12 14:39:38 EDT
This may not be a priority for 3.3, but marking 3.3 for now so that any remaining limitations of the new workspace undo support can be examined and documented. 
Comment 3 Micha Riser CLA 2007-02-13 10:17:16 EST
There is a minor flaw with source control systems which may be related to this bug:
- Choose a file that is under source control
- Delete it
- Restore the file from local history

The source control doesn't consider the file as unchanged as it is (it was deleted and the deletion was reverted). 
- The eclipse CVS shows the file as modified but there is no difference
- Subclipse shows the file as deleted
Comment 4 Susan McCourt CLA 2007-04-09 20:08:36 EDT
The scenario in comment #3 is not related to this bug, as it refers to restoring a file manually from local history.  If you a delete a file under CVS control and select "Edit>Undo Delete Resources" the file is restored and CVS does not mark the file as modified.

Marking this bug as WONTFIX since there is no compelling case to address it.
Comment 5 Susan McCourt CLA 2008-01-14 10:00:57 EST
*** Bug 214992 has been marked as a duplicate of this bug. ***
Comment 6 Francis Upton IV CLA 2008-01-14 11:26:55 EST
I would like to reopen this bug; I wrote bug 214992 which is duped against this.  

My application uses some it its own persistent properties to determine the type of the object so that the correct icon/label/etc is associated with it in the navigator.  Further, it uses the Eclipse defined property to associated the desired editor with the resource.

My preference would be that the persistent properties stay with the resource even through the source control system.  However, there seems to have been a lot of discussion about this and people feel it's better for each application to have its own mechanism of persisting these properties through source control.  I can live with that and invented my own mechanism to do so.

When resources are moved and copied, the persistent properties happily go with the resource.  However, when they are deleted and the deletion is undone, the properties are lost.  I think this is inconsistent and inconvenient.  Although the IResource API does not expose a method to get all properties associated with a resource, I think it would be simple (technically anyway) to extend the API to do so (The underlying IPropertyManager API supports this).  And it would be equally simple then to store the properties (along with the other object state) in the descriptor associated with the deleted object.

In the absence of this, each application would first have to discover the problem through testing (like I did), and then invent a mechanism to restore the properties upon delete undo.  This is particularly problematic for applications with session properties who have no expectation that the properties persist in source control. In this case, Eclipse seamlessly manages their properties except in this one case, so they would have to invent a parallel mechanism to store/restore the properties just for delete undo.
Comment 7 Francis Upton IV CLA 2008-01-14 11:30:50 EST
Oh, and I would be happy to provide patches if it will help things along.
Comment 8 Susan McCourt CLA 2008-01-14 11:43:43 EST
yes, a patch will help things along.  When I first looked at this, it required new  API on IResource to be able to enumerate the properties.  I think this is still the case, so any API changes would have to get in for M5.
Comment 9 Francis Upton IV CLA 2008-01-14 13:31:10 EST
Thanks for reopening Susan.

In further investigation, getting the properties is pretty simple as long as you have access to the Workspace object (as opposed to being forced to use IWorkspace).  I don't know much about how the use of the official API is encouraged in the UI projects; can I use the Workspace object directly to get the property manager (IPropertyManager)?  If not, I can propose API extensions (essentially adding IWorkspace.getPropertyManager() and making IPropertyManager part of the API - but this might be too big).

How would you like me to proceed?
Comment 10 Francis Upton IV CLA 2008-01-14 13:32:31 EST
(Copied this comment from bug 214992 for convenience):

In 3.4 we might have to implement this in
org.eclipse.ltk.internal.core.refactoring.resource.undostates.AbstractResourceUndoState
since the default delete is the LTK delete.

PW
Comment 11 Susan McCourt CLA 2008-01-14 13:38:12 EST
Yes, you need to stick with the official API (IWorkspace, IResource, etc.) rather than count on internal implementations of those things.

What I suggest is that you also open a bug against the resource API for whatever it is you need, and make this bug depend on that one.  If the resource team thinks it's a reasonable API extension, then you are right, the implementation should be fairly straightforward.
Comment 12 Francis Upton IV CLA 2008-01-15 01:09:33 EST
I added bug 215299 for the resources component.  Can you make this bug dependent on that one?

Comment 13 John Arthorne CLA 2008-01-15 22:43:30 EST
Would you really hang onto the persistent properties of all deleted resources in memory via the undo stack? The number and size of persistent properties could be quite large - these values are never held in memory by the resources plugin... they live only on disk.
Comment 14 Francis Upton IV CLA 2008-01-16 01:54:25 EST
@John,

I think you have a good point; I wonder if it's possible to leave those deleted properties on disk somewhere (perhaps associated with somekind of hidden resource or something like that) until they fall off the undo stack.  

On the other hand, I don't know how big the undo stack typically gets.  If it's not too big, then it may not be a problem.  Each property is limited to 2K, do many applications use large properties like that?  My application only uses a few properties and they are just a small number.

In the absence of some data about the potential memory consumption, maybe we start with the much simpler solution of keeping the properties in memory and seeing if it's a problem; if so, the necessary work can be done to leave them on the disk.
Comment 15 Susan McCourt CLA 2008-01-16 10:07:59 EST
I'm wondering if we should just say that plug-ins that need to store their properties away should be adding an undo participant which takes care of it.  I think that's what the story is for other structures that need to be restored on delete.  Paul?
Comment 16 Francis Upton IV CLA 2008-01-16 10:41:22 EST
If this does not involve writing much code, then I would be happy with a mechanism like this.  I have never heard of an undo participant, can you provide me a reference?  Is this a 3.4 feature (I'm still on 3.3).

(In reply to comment #15)
> I'm wondering if we should just say that plug-ins that need to store their
> properties away should be adding an undo participant which takes care of it.  I
> think that's what the story is for other structures that need to be restored on
> delete.  Paul?
> 

Comment 17 Paul Webster CLA 2008-01-17 11:31:04 EST
See the org.eclipse.ltk.core.refactoring.deleteParticipants extension point description and the class org.eclipse.ltk.core.refactoring.participants.DeleteParticipant

Basically, on delete you will be asked to create a "Change" object that holds your changes (which would be minimal) but that change object also gets to provide the Undo Change object (which could restore the properties you care about).

PW
Comment 18 Francis Upton IV CLA 2008-01-26 04:37:06 EST
I'm getting ready to make a patch for this this weekend and want to do it respecting your comment; that is, to keep the changes on disk.  However in order to do that, the simple extension of the IResource to get the properties won't work  for the persistent properties.

My thinking is to use the property manager directly to save the properties of the resource under some name that could never possibly be a valid resource name (I'm not sure at present how to create such a path; perhaps a different top level space where deleted properties is the right place to put this).  This name would be recorded in the delete undo state.  And when that state is disposed, the property manager would be called again to remove the properties.

I can imagine some API on the IPropertyManager like this:

IPath saveDeletedProperties(IResource resource) - returns a IPath which is the path of where the properties were saved (this is something that PropertyManager2 would determine)

void deleteProperties(IPath path) - would delete the properties at this path.

I can imagine not wanting to export the IPropertyManager as API, so perhaps we can create an IResourcePropertiesDeleteUndo interface (having only the above 2 methods) which can be returned from a method like this on IResource:

IResourcePropertiesUndo getPropertiesDeleteUndo()

I await your wisdom and guidance.

(In reply to comment #13)
> Would you really hang onto the persistent properties of all deleted resources
> in memory via the undo stack? The number and size of persistent properties
> could be quite large - these values are never held in memory by the resources
> plugin... they live only on disk.
> 

Comment 19 Francis Upton IV CLA 2008-01-29 05:34:24 EST
Created attachment 88117 [details]
Patch for  I20071213-1700

This is a patch that depends on the patch attached to bug 216884.  Please mark this bug dependent on bug 216884 and remove the dependency on bug 215299, as a different approach to the problem was taken than originally proposed.

This patch has been tested using a large batch test suite.  I also verified that the persistent properties were being properly removed on disk when the undo was no longer possible.
Comment 20 Susan McCourt CLA 2008-01-29 11:34:50 EST
Francis, thanks for the patch.
I've changed the dependency on this bug as you request.

I will await the resolution of bug #216884 before looking more closely at this patch.  Please ping me via this bug if you need help/opinions before then.

It's awesome that you have a huge batch of your own tests, but we would want to have something in the regular undo test suite so we can regularly test this support.  The more thorough the tests, then obviously the more confidence we would have.  When we are ready to integrate, could you add a patch for some tests in org.eclipse.ui.tests.operations.WorkspaceOperationsTests?

Thanks again.
Comment 21 Susan McCourt CLA 2008-01-29 12:58:24 EST
Actually, now that delete resource goes through LTK, this bug belongs with JDT UI.  So any tests provided should fold into the LTK suite.  
Comment 22 Francis Upton IV CLA 2008-02-15 03:59:22 EST
Paul/Susan - is the old undo mechanism going to go away soon?  I wonder if it makes sense to make this change to both the old (IDE) and new (LTK) undo mechanism if the old one will not last that long.
Comment 23 Francis Upton IV CLA 2008-02-17 17:39:52 EST
Paul,

For the record this extension point does not quite do what I need.  The problem is that all of the participants are executed *after* the main processing in the "do" case, and of course *before* the main processing in the "undo" case.  So with a requirement like mine this will not work for undo as the resource is not present when then participant has executed.

I have worked around this using a really gross hack, I schedule the processing for my undo (caused by my DeleteParticipant) asynchronously, checking for the presence of the resource.  This seems to work fine, but is far from desirable.

Fixing bug 63149 would address this issue.

Francis

(In reply to comment #17)
> See the org.eclipse.ltk.core.refactoring.deleteParticipants extension point
> description and the class
> org.eclipse.ltk.core.refactoring.participants.DeleteParticipant
> 
> Basically, on delete you will be asked to create a "Change" object that holds
> your changes (which would be minimal) but that change object also gets to
> provide the Undo Change object (which could restore the properties you care
> about).
> 
> PW
> 

Comment 24 Martin Aeschlimann CLA 2008-03-11 11:48:26 EDT
Refactoring participants can now run before the refactoring, which means that the undo will run after the refactoring undo (bug 63149). 
This should allow you to store and restore the properties you need on delete.

I think this is a better solution than to try to solve this in a general way for all persistent/session properties. It would be wrong to assume that all properties expect to be restored. Also see the discussion in bug 216884.

Ok if I set this bug to WONTFIX?
Comment 25 Francis Upton IV CLA 2008-03-11 11:57:06 EDT
I would prefer this bug be kept open, and I plan on working on a way to address this also as part of 216884, which I'm waiting for help with in reviewing some ideas.  Certainly we won't be able to address it for 3.4, but I would like to consider this for 3.5.  I think automatic undo/redo support for the persistent properties at least makes sense in the context of the way resources work otherwise (i.e. the persistent properties are currently automatically moved and copied with resources, so it's inconsistent from the user point of view that delete undo is not handled in the same way).  

(In reply to comment #24)
> Refactoring participants can now run before the refactoring, which means that
> the undo will run after the refactoring undo (bug 63149). 
> This should allow you to store and restore the properties you need on delete.
> 
> I think this is a better solution than to try to solve this in a general way
> for all persistent/session properties. It would be wrong to assume that all
> properties expect to be restored. Also see the discussion in bug 216884.
> 
> Ok if I set this bug to WONTFIX?
> 

Comment 26 Martin Aeschlimann CLA 2008-03-11 12:03:38 EDT
I suggest we concentrate on bug 216884. If we get results there and LTK is required to do something it will be easier to file a new specific bug.

Setting this bug to WONTFIX.
Comment 27 Martin Aeschlimann CLA 2008-03-11 12:06:42 EDT
Ok, WONTFIX sounds wrong. I'll leave it open.
Comment 28 Eclipse Genie CLA 2019-12-21 19:36:06 EST
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet.

If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant.

--
The automated Eclipse Genie.
Comment 29 Dani Megert CLA 2019-12-22 08:57:20 EST
No one is working on this. If you want to see this fixed, please reopen with an attached Gerrit change.