Community
Participate
Working Groups
This is in support of bug 160729 (support persistent property restore on delete undo). And this replaces the requirement outlined in bug 215299 (add methods to get session and persistent properties). This is to allow persistent properties to be restored automatically if a resource deletion is undone. Currently they are dropped on the floor. The original proposal (bug 215299) envisioned providing a Map of the properties and they would be stored in the LTK deletion context. However, given https://bugs.eclipse.org/bugs/show_bug.cgi?id=160729#c13, I am proposing an implementation which will keep the properties on disk at all times to be consistent with the current implementation. This is done by having a separate BucketTree in PropertyManager2 for the purpose of storing these deleted properties. This thus involves adding the API methods below to IResource and the corresponding support in IPropertyManager and its implementation. I'm open to other ways to proceed with this and will alter my patch as required. Also, I have left the documentation in IResource less than complete until this patch is agreed to; once that is done, I will fix IResource to be perfectly pretty. This patch also includes a minor refactoring in Resource eliminating a few lines of duplicate code that appear in many places. I have an extensive test suite (over 1600 tests) which this patch has been run on, which includes tests for delete undo. /** * Removes the previously saved persistent properties. * * This is used to support saving persistent properties when a resource * has been deleted so that the properties can be restored is the delete is undone. * @throws CoreException */ public void removeDeletedPersistentProperties() throws CoreException; /** * Sets this resource's persistent properties to be those previously * saved when the using the savePersistentPropertiesAsDeleted() method. * * This is used to support saving persistent properties when a * resource has been deleted so that the properties can be restored is the delete is undone. * @throws CoreException */ public void restorePersistentPropertiesFromDeleted() throws CoreException; /** * Saves this resource's persistent properties so they persist after the * resource has been deleted. This must be followed either by a * restorePersistentPropertiesFromDeleted() call (if the delete is undone) * or a removeDeletedPersistentProperties (if the delete can never * be undone. * * This is used to support saving persistent properties when a resource * has been deleted so that the properties can be restored is the delete is undone. * @throws CoreException */ public void savePersistentPropertiesAsDeleted() throws CoreException;
Created attachment 88116 [details] Patch for I20071213-1700
And again, thanks for the patch. However as I suggested in the bug 215299, I would unify the API for persistent and session properties.
(In reply to comment #2) > And again, thanks for the patch. However as I suggested in the bug 215299, I > would unify the API for persistent and session properties. > I agree with the unification of the APIs, however that does not solve the memory issue for persistent properties as mentioned in bug 160729. So I think this approach is necessary for performance reasons.
Also, if you will accept this patch, I will provide the additional test cases to thoroughly test it. The patch currently passes the existing resource test suite.
Francis, I (very) briefly looked at this patch. I'm by no means an expert in the ways of IResource, so these comments are from a client perspective. It seems odd to me that IResource would introduce API for saving and restoring properties upon deletion. After seeing that, I would expect, for example, for similar API to be provided for markers. Why are markers restored one way and properties another? It seems the responsibility for undo is getting spread around a bit. The existing file history keeps track of content, the operations framework keeps track of markers, then another mechanism gets introduced for properties. I understand you need to solve the problem, I just think we need to be careful about where this is added. John or Szymon, any thoughts?
Susan, I completely agree with you, I'm a little uncomfortable with this patch as it seems to pollute the resource API a bit (I'm thinking that this is the root of Szymon's concerns). The real issue is one of implementation. I think the changes to PropertyManager2 are good, in that it fairly elegantly deals with delete/undo of persistent properties in a way that does not tie up memory; I think reusing this implementation somehow is key to efficiently making this work. If people agree on that point, the issue is: how do we expose those features to the undo mechanism? One simple way is to allow you to get at the IPropertyManager (or some subset that we can define) through the IResource api. The other alternative is the mechanism proposed using bug 215299, which is an acceptable extension to the IResource API, but means that the undo mechanism will need to store the state. This can easily be done in memory, as was the original idea, but that's probably too resource intensive. I want to work on a solution that provides the correct undo of delete for resource properties, and am willing to spend the time on creating patches and tests. I could easily solve my particular problem by adding a custom undo handler along the lines Paul suggested in the original bug report, but I would prefer to contribute to the more general solution to help the community. I'm open to any ideas about this. (In reply to comment #5) > Francis, I (very) briefly looked at this patch. I'm by no means an expert in > the ways of IResource, so these comments are from a client perspective. > > It seems odd to me that IResource would introduce API for saving and restoring > properties upon deletion. After seeing that, I would expect, for example, for > similar API to be provided for markers. Why are markers restored one way and > properties another? It seems the responsibility for undo is getting spread > around a bit. The existing file history keeps track of content, the operations > framework keeps track of markers, then another mechanism gets introduced for > properties. > > I understand you need to solve the problem, I just think we need to be careful > about where this is added. John or Szymon, any thoughts? >
>I could easily solve my particular problem by adding a custom undo >handler along the lines Paul suggested in the original bug report, but I would >prefer to contribute to the more general solution to help the community. I appreciate your motivation... Off the top of my head - what if the code were factored into a generic participant that persists certain properties described in the participant class. If it were general purpose enough, the actual class could live in LTK? Plug-ins could register a property-persisting participant for their model objects, perhaps subclassing it or otherwise instantiating it with a certain set of properties it cares about. If an efficient implementation requires access to internal resource methods, it would be more likely that this could be negotiated between LTK and the resource plug-in. Then there is no specialized code needed at all in the LTK undo code, it's just a matter of making a generally useful participant available. cc'ing Martin for an opinion.
Susan, I think what you propose could be a good solution, but if you consider the product from the perspective of the developer or user, it adds additional complexity for them (which is what I'm trying to avoid). They would first have to recognize that delete undo does not deal with the persistent properties, and then figure out the mechanism to fix it. I believe the solution I have proposed would just automatically handle it (in the same way as when resources are copied), so from the user's point of view that's better. Of course the problem with my solution is it clutters the resource API, which I agree is not good. I'm encouraged by your comment that it's possible for the LTK to user some internal APIs. If the LTK would be willing to use the IPropertyManager API as I have modified it, and some other interface be provided (IPropertyManagerProvider?) which Resource can implement to get the IPropertyManager, then I think we might be done, because everyone will have what they want.
After discussion with John, one of my concerns about changes in PropertyManager2 is that some things can hang forever for a deleted resource. I like two solutions of this problem so far. First is the one from the comment #7. I have been thinking about it for a while too. And second is to undo properties like it is done for the content of files. We could access properties of deleted resources via the history store or a similar mechanism. This would have some advantages... e.g. we could control the size of deleted properties and we wouldn't have to delete them manually, they will be deleted in the same way like the history entries. I hope it makes sense.
Just to clarify, the only time deleted properties will accumulate (in this implementation) as far as I know will be in the event of a crash. In normal operations, when the undo change is disposed, the deleted property is removed (I checked that this works with this patch). Still, with crashes they can accumulate and that is bad. I think I prefer the 2nd solution below (even though I don't know anything about the history mechanism) because it's fully automatic. I will investigate this and come up with a patch (or questions) soon. (In reply to comment #9) > After discussion with John, one of my concerns about changes in > PropertyManager2 is that some things can hang forever for a deleted resource. > > I like two solutions of this problem so far. > > First is the one from the comment #7. I have been thinking about it for a while > too. > > And second is to undo properties like it is done for the content of files. We > could access properties of deleted resources via the history store or a similar > mechanism. This would have some advantages... e.g. we could control the size of > deleted properties and we wouldn't have to delete them manually, they will be > deleted in the same way like the history entries. > > I hope it makes sense. >
Thanks Francis. I will help, if you need a hand.
Ok, here is my thinking so far, after having done a little investigation into the HistoryStore. 1) Extend the IHistoryStore, IFileState, etc to accommodate persistent properties. This can be implemented by extending HistoryBucket (and thus a version bump), or by making a separate tree for properties and using the PropertyBucket. The latter is preferred since it's reusing an implementation suited for storing properties and we don't have to do the version change. 2) Hook the LTK delete undo mechanism to get the properties from the history state upon undo. One thing I have not yet found out is that is the lifecycle of the history state vs. delete undo. Does the history live past the delete? Of course we would want the history state to stay around as long as there is the possibility of a delete undo. Can you give me some guidance for how history is maintained and cleaned up, and how to make it stay past a deletion? I know where the hooks are in the LTK mechanism to notify that the delete undo is either done or impossible, so at these points it could call the history mechanism to clean up (at least from the delete perspective). I think there have been other requests for history information about persistent properties (IIRC), so fixing this would have the side effect of satisfying those.
'Undo delete' is only one way of restoring a file. Restore from history would be another. But there are also operations to restore content like 'Replace from HEAD, or 'Import'. So I wonder, are properties really the way to go to keep 'important' information? Maybe you have to better keep metadata in its own files, like '.classpath'. Before we decide to store properties in the history, we need to make sure we understand the usage of properties. Are all property values meant to be restored?
Please refer to bug 160729 which describes the underlying problem I'm trying to solve. Currently, when you undo a delete, the resource is recreated without the persistent properties. This is inconsistent from a user point of view, since for example copying a resource copies the persistent properties. I'm trying to find a general solution to the problem (actually the patch to this bug report has the semantics I'm trying to achieve, it's just got some concerns in the implementation and layering). I'm doing this to try and contribute something for all users, not just solve my particular problem. I could easily solve my problem with an LTK undo handler. Personally, I'm not sure it is a good idea to store the properties in the history. I don't know enough about how the history is used. My original idea was to store the properties in the (now LTK) delete undo context in memory, but John mentioned the potential performance problems associated with that. So I came up with a patch to keep them on disk like all other properties, but there are cleanup issues (in the event of a crash) and questions about cluttering the resource layer with undo stuff. Now people are pointing me to the history mechanism, but it's not clear that's the best case. I think perhaps the cleanest solution at this point might be to have the LTK mechanism use an implementation of the IPropertyManager to store the properties on disk, and have something that will deal with the cleanup issues in the event of a crash (that is deleted properties that have no corresponding undo information). This would not involve the resources code at all. However it would mean that LTK would need to use some non-API code. But perhaps this could be negotiated. But I'm not sure who to talk to. I'm quite happy to do the work here, I just want to make sure the next patch I submit will actually be accepted, so I want guidance on where it should go. (In reply to comment #13) > 'Undo delete' is only one way of restoring a file. Restore from history would > be another. But there are also operations to restore content like 'Replace from > HEAD, or 'Import'. > So I wonder, are properties really the way to go to keep 'important' > information? Maybe you have to better keep metadata in its own files, like > '.classpath'. > > Before we decide to store properties in the history, we need to make sure we > understand the usage of properties. Are all property values meant to be > restored? >
I'm in the LTK refactoring team. No, can't use internal API. We would first make things API. Undo is now at two places: platform.UI and LTK.refactoring. Platform still has API that offers to store the state of a resource before delete. This API is still in use, for example in JDT.UI. We have to keep the two implementations in sync.
:) Good that I'm talking to the right person. And thanks for the tip about keeping the implementations in sync; I'm familiar with both implementations (when I first made the patch in this bug report I did it for platform.UI). What do you think about exposing some level of API so that the LTK/UI can use the property manager implementation to store the persistent properties on disk for undo purposes? (In reply to comment #15) > I'm in the LTK refactoring team. No, can't use internal API. We would first > make things API. > > Undo is now at two places: platform.UI and LTK.refactoring. Platform still has > API that offers to store the state of a resource before delete. This API is > still in use, for example in JDT.UI. We have to keep the two implementations in > sync. >
(In reply to comment #15) > Platform still has > API that offers to store the state of a resource before delete. This API is > still in use, for example in JDT.UI. We have to keep the two implementations in > sync. Martin, what API are you referring to in platform? I looked through the JDT.UI stuff and could not find it. Can you give me a class name? I assume you are not talking about the resource history stuff (IFileState, etc).
I meant ResourceDescription in 'org.eclipse.ui.ide'. ResourceUndoState in 'ltk.refactoring.core' is a copy of ResourceDescription.
Susan/Martin/Szymon - Request for comments - outline for proposed patch (in core.resources): 1) The objective would be to extend the history mechanism to keep track of persistent properties for file resources. This could then easily be used by the higher level (LTK) undo support. 2) A new interface IFileStateProperties (extending IFileState) is added which includes one method, getPersistentProperties(). The FileState class is changed to support this interface. We must add a new interface to maintain API compatibility. This is the only API change. 3) Persistent properties would be stored in the HistoryStore using a new version in the HistoryBucket which looks like this: * Version 3 (3.4 M6): * <pre> * FILE ::= VERSION_ID ENTRY+ * ENTRY ::= PATH STATE_COUNT STATE+ * PATH ::= string (does not include project name) * STATE_COUNT ::= int * STATE ::= UUID LAST_MODIFIED PROPERTIES * UUID ::= byte[16] * LAST_MODIFIED ::= byte[8] * PROPERTIES ::= (PropertyEntry value portion) * </pre> 4) The version checking in Bucket.load() would have a subclassed portion to deal with reading older versions; both versions 2 and 3 of the HistoryBucket would be read, though only version 3 would be written. This is acceptable because we don't need to allow older versions once someone has upgraded. 5) The implementation of HistoryBucket will call the PropertyBucket code to read/write the PropertyEntry value portion into the version 3 HistoryBucket. 6) LTK (new)/IDE undo (old) are easily modified to use this since they already get the contents from the existing history mechanism. This would add no new issues for cleanup of the properties, as this is already handled by the existing history mechanism. This also keeps the persistent properties only on disk. 7) Tests would be altered in the LTK test suite to check for undo of properties, and to the resource history test cases to check that properties are correctly stored. 8) Note that this change does not accommodate restoring of persistent properties for non-file resources, as they have no history (unless I'm mistaken). I'm not sure how (or even if) this should be addressed.
After all, I'm not sure if we should persist all properties in the history. We should think about a mechanism (API or an extension point) that would allow plug-ins to register properties that should be stored. The issue here (as Martin noticed) is that there could be properties (let's say modification_time) that shouldn't be stored in the history.
I agree that it is undesirable to have the history mechanism restore the persistent properties along with the rest of the history. However, the history mechanism (as pointed out in comment #9) comes with a lot of advantages about being able to bound the size, having it be automatically cleaned up so that we don't leak, etc. So I think using that implementation makes sense for storing the deleted properties on the disk (which is what we want to do, rather than in memory) for the purpose of delete undo. If we don't use the history mechanism (as proposed in my original patch), then we will have to reinvent all of this mechanism. I think the issue of restoring the properties with the history is a separate one than saving the properties in the history. The patch I'm proposing will not restore the properties when the history is restored, it will only make them available when requested (and in this case it's requested by the undo handlers). The issue is not that all properties should not be stored in the history, it's that they should not necessarily be restored. Modification time for example would make sense to be preserved in the case of a delete undo, however we would not want to restore it if restoring history. I agree that there could be a mechanism to control selecting properties to restore as part of the history (API or extension point), but I think that's a separate item of work (that depends on this one). (In reply to comment #20) > After all, I'm not sure if we should persist all properties in the history. We > should think about a mechanism (API or an extension point) that would allow > plug-ins to register properties that should be stored. > > The issue here (as Martin noticed) is that there could be properties (let's say > modification_time) that shouldn't be stored in the history. >
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.
