Community
Participate
Working Groups
Randy, "Randy Hudson" <none@ibm.com> schrieb im Newsbeitrag news:ahgvkq$e2p$1@rogue.oti.com... > Step through CommandStack.undo(). > The command created by the undoable PS entry will call some API on > IPropertySource to set/reset the value. This should cause your model to > fire a change. I found that there is a SetPropertyValueCommand and this command does the undo but not correctly IMHO. If IPropertySource.isPropertySet returns true, the SetPropertyValueCommand stores the old value and restores it on undo. But if IPropertySource.isPropertySet returns false, the SetPropertyValueCommand simply resets the property value instead of restoring the old one. A proper implementation of IPropertySource.isPropertySet "returns false if the notion of a default value is not meaningful for the specified property". This is the case in my model. With the currently implementation of SetPropertyValueCommand it is not possible to undo a property value change if your model doesn't use default values. Will you change the implementation? Cu, Gunnar
The JavaDOC for IPropertySource is incorrect. I argued a while back (before 1.0) that this method should be called isPropertyResettable(Object ID). You *have* to use the method to mean that. The JavaDOC states: "false if the notion of a default value is not meaningful". *BUT* false is ALSO returned in the case where a default value *IS* meaningful and the property's value is currently the default value. You can't overload the meaning of false in this way. If you do, the method is meaningless. How can GEF know whether to call setPropertyValue(ID, oldValue), or resetPropertyValue (ID)? You must implement resetPropertyValue(Object ID) for all properties which return isPropertySet(ID) FALSE, because FALSE means that the current value is the DEFAULT value, not that default is undefined.
What we really need is a method on IPropertySource that says canPropertyBeReset (Object propertykey). In addition to isPropertySet. isPropertySet is absolutely necessary for those properties that can be reset. As Randy noted, this method was impropertly overloaded within these meanings. But can we have the API changed now to include this new method. It would be a valuable addition because then IPropertySheetEntry could use this method and then the PropertySheet itself could disable the reset property value action if the property sheet entry returned false for this. (Note: PropertySheet would have to go through the PropertySheetEntry for this, it should not go directly to the IPropertySource because it doesn't know anything about the IPropertySource).
I agree with what Rich said. But I think the new API should be called isPropertyResetable(). Here are the 3 cases: 1) foo.x = [default value]; 2) foo.x = 5, but a default value exists. 3) foo.x = 5, but there is no such thing as a default. As a result: case: undoShouldReset: resetButtonEnabled: ----------------------------------------------- (1) TRUE FALSE (2) FALSE TRUE (3) FALSE FALSE If you decide that disabling the reset button is not important, then cases 2&3 can be merged. But in order to merge 2&3, the JavaDOC should state that isPropertySet(Object) must return *true* if the notion of a default property does not exist. It currently reads the opposite. If reset button does not disable, the user will click and nothing will happen. Also, a no-op will be on the Undo/Redo stack.
Please consider for 3.0. In the very least, update the javadoc.
It is very bad because this breaks nearly every GEF application. Undo doesn't work for any of our property sources in GEF. The time for breaking API changes is now over so we have to define a new interface IPropertySourceExtension. The API of IPropertySource is simply not clear in this situation. Will it be possible to include this in 3.0 if prepare a patch for this issue? If yes, what is allowed and what is not allowed?
Created attachment 8452 [details] Patch for IPropertySource JavaDoc This is a patch for IPropertySource.java (Revision 1.6) that only corrects the JavaDoc.
Created attachment 8453 [details] Patch to add new API (instead of JavaDoc patch) This patch should be used INSTEAD the IPropertySource-JavaDoc patch to add an additional interface (IPropertySourceExtension) that provides the API for asking a property source if a property is resettable. This patch also includes modification of PropertySheetEntry to use the new API. It must be applied to package "org.eclipse.ui.views.properties". The new API must be used additionally to the already existing API: // reset a value IPropertySource source = ... if (source.isPropertySet(descriptor.getId())) { boolean doReset = true; // call new API to check if property is resettable if(source instanceof IPropertySourceExtension) { IPropertySourceExtension extendedSource = (IPropertySourceExtension) source; // if property is not resettable then don't do a reset if (!extendedSource.isPropertyResettable(descriptor.getId())) doReset = false; } if(doReset) source.resetPropertyValue(descriptor.getId()); }
Nick is the owner of the properties view component area. This bug doesn't cause loss of data, so its either blocker (preventing development work) or major (loss of important functionality). I'm moving it to major. Feel free to change.
This bug could cause anything to occur. GEF is interpreting isPropertySet() to mean what is should. If clients expect it to mean what the JavaDoc says, then they may have GEF calling resetProperty() when they don't expect it. What are the results of doing this? Who knows, maybe the client code calls: System.exit(1) :-)
Perfect, than this is critical as one of our clients already had a data loss. use case: edit property, perform save, perform undo -> property still has the new value If the client doesn't have any backup of the underlying resources (either in some versioning system of real backup) then the data is lost. Note, now we simply return true and averything works fine.
I'll have a look for M9.
I can't accept the patch above because it changes the spec of IPropertySource.isPropertySet. Any existing implementations that conformed to the old spec will now be "broken". Randy and Gunnar, can you please collaborate on a revised patch that will address the needs of GEF without breaking any existing API (even if the old API was effectively broken). Nobody on the UI team is actively working in this area, so if this is going to be fixed for 3.0, it's going to have to come from you.
GEF will make the proposed fix of checking the value before and after calling setValue. So in a sense, a boolean property can indicate three states!?! The javadoc could be changed to reflect this without breaking its meaning. In other words, isPropertySet means one of two things if it returns false, and you won't know which until you call setProperty. Is the cat dead? You'll have to open the box to find out.
Pratik, track whether isPropertySet(...) changes when we call setProperty. If it does change, then it can be reset. Otherwise, we need to set it back to the old value on undo.
See bug 54250 for a possible patch with lots of comments that may be removed ;)
Gunnar, I forgot about that bugzilla. That means these are either dupes, or we should route this back to platform UI (and GEF will address the other). I'll let you decide.
Nick, can you explain more detailed? For me the JavaDoc looks just wrong. The first patch only the fixes the JavaDoc. The 2nd patch creates new (additional API) like it is seen everywhere in the workbench.
The 2nd patch also includes incompatible changes to old API.
I also want to ensure you, Randy and any other stakeholders in this PR are in agreement before reviewing any patch. Please make it exceedingly easy for me to put a fix in <g>, since I really need to focus on other areas at the moment.
I'd have to agree with Gunnars patch. The current API is misleading. I don't see the negative side effects to re-aligning the API with its indended meaning. For those who ignore the change, they would be returning false incorrectly. But, this is not a problem in the propertysheet by itself. It is only a problem for clients which try to implement Undo such as GEF. But this is already broken, so you can't break something futher.
Sorry, but I'm not going to change the existing API, and go through the hassle of documenting this breaking change in the porting guide. If it was incorrect or ambiguous before, then the new API can correct for it. Please provide an updated patch.
Ok, I will update the patch. Randy, do we really need the second interface or is it suitiable when we just change the JavaDoc?
The update to the JavaDoc works in the case where the caller is implementing UNDO, and the caller is the one invoking setProperty(), and can therefore query the value after setting the property.
Created attachment 10225 [details] Proposed patch with fixed JavaDoc and new interface This is a proposed patch that will correct the JavaDoc and add a new interface IPropertySource2. This interface contains API that will be called to check if a property is resettable (in PropertySheetEntry#resetPropertyValue). Corrected API: IPropertySource#isPropertySet(Object) New API: IPropertySource2#isPropertyResettable(Object) I also formated IPropertySheetEntry and enhanced the JavaDoc of IPropertySheetEntry#resetPropertyValue to describe the behavior when a default value is not meaningful.
Please review, I'll create a patch for bug 54250 that will use the new API.
I have fixed SetPropertyValueCommand to handle this problem by checking the value of isPropertySet(String) before and after the new value is set. So, as far as GEF is concerned, this is not a problem anymore. There's probably no reason to include the new interface into this release, unless of course there are others who are affected by the same problem.
So there's nothing to do here? Gunnar, do you agree that Pratik's workaround will suffice for 3.0? Note that Friday is our API freeze.
At least the JavaDoc should fixed. I'll rework the patch.
Nick has asked me to look to at this and submit something that is acceptable to both Gunnar and Randy as well as not breaking the existing API. Gunnar and Randy what is your consensus on this. Are you still after the new interface?
I think both of us are really just concerned with the Undo/Redo scenario. We have implemented Gunnar's suggestion, which is to call isPropertySet() before and after calling setProperty(..). So there is no known scenario which requires the new interface to work properly. However, it is too bad that the original interface is not perfect, but I think we can salvage it with some better JavaDoc and guidelines on how to treat isPropertySet(...) returning false.
Released Gunnars final patch and fixed the javadoc warnings in PropertySheetEntry.
The JavaDoc changes made to IPropertySource#isPropertySet(Object) are breaking API changes. And, it doesn't reflect the way PropertySheetEntry#resetPropertyValue() uses the API. Pre-3.0: returning false indicates that the specified property does not have a default value R3.0: returning true indicates that the specified property does not have a default property The new R3.0 definition is correct *only* if you implement IPropertySource2, but otherwise it is incorrect. If you implement only IPropertySource and a given property does not have a default value, then according to the API, you should return true. But this means that the reset action on the propertysheet will get enabled, and resetProperty(Object) will be called on the IPropertySource. So maybe we should restore the original API, and re-declare the method in IPropertySource2 stating the new definition. Also, the DefaultsAction has never been smart about disabling.
Gunnar or Randy, do you think anything further needs to be done here for 3.1? If so, if either of you could provide a patch, I'd really appreciate it.
Created attachment 20599 [details] New patch with corrected JavaDoc and interfaces This patch corrects the API we broke in 3.0. It reverts the JavaDoc for IPropertySource to the 2.1 contract and redefines it in IPropertySource2.
Thanks Gunnar. I've asked the PMC to approve this API change.
Kevin gave the +1 for this. Also need to update the plug-in migration guide for this API change.
Patch applied. I've added the following to the Javadoc for IPropertySource2.isPropertySet: * <code>IPropertySource2</code> overrides the specification of this <code>IPropertySource</code> * method to return <code>true</code> instead of <code>false</code> if the specified * property does not have a meaningful default value. * <code>isPropertyResettable</code> will only be called if <code>isPropertySet</code> returns * <code>true</code>.
I've added the following to the migration guide: Changes to isPropertySet(boolean) in IPropertySource and IPropertySource2 What is affected: Plug-ins that implement org.eclipse.ui.views.properties.IPropertySource or IPropertySource2. Description: In Eclipse 3.0, the specification for the method org.eclipse.ui.views.properties.IPropertySource.isPropertySet(boolean) was incorrectly changed to specify that true should be returned if the specified property did not have a meaningful default value. In previous versions, it specified that false should be returned in this case. This was an inadvertent breaking API change, although the implementation worked the same as before if the property source implemented IPropertySource and not IPropertySource2. This has been corrected in 3.1, with IPropertySource.isPropertySet(boolean) being reverted back to its earlier specification (that false should be returned in this case), and IPropertySource2.isPropertySet(boolean) overriding this to specify that true should be returned in this case. For more details, see bug 21756. Action required: Any classes implementing IPropertySource or IPropertySource2, where some of the properties do not have meaningful default values, should be checked to ensure that they return the appropriate value for isPropertySource(boolean). Clients should check that the Restore Default Value button in the Properties view works as expected for their property source.
cc'ing Jim for the API change.
approved - I'll add this one to the Annals of API Development :-)
Verified in I20050509-2010.
