Community
Participate
Working Groups
When users are entering data into an editor that is bound using the data binding framework, we need to optionally be putting commands on Eclipse's undo/redo stack so that all editing is undoable/redoable. In most cases, we should be able to do this automatically for the users if we supply an UndoableUpdateable that creates command objects that: - Save the new value as state in the command - Save the property's old value as state in the command - Implement undo and redo as simply setting the property value to the old value or new value
I think undo/redo could/should be handled at the model level, not at the level of data binding. We should make sure though that existing models for undo/redo (think EMF) are compatible with how the data binding framework works.
Disagree for the following reason: 1) In MVC, Undo/Redo is normally a Controller-level concern, not a model-level concern. ie: you almost never persist the undo/redo stack along with the command objects. 2) Eclipse itself implements undo/redo as a controller-level concern. ie: Eclipse doesn't persist its undo/redo stack with commands. 3) I need this capability (hence, why I assigned the bug to me).
I've implemented this now for IUpdatebleValue and IUpdatableCollection. Working on getting clearance to contribute the IP.
In bug #118125, Michael Scharf writes: I have created a data binding framework that binds EMF with SWT widgets similar to the jface.databinding. I had to solve a problem that seems not to be addressed in the jface.databinding (at least I did not find an obvious solution). It has to do with undo-redo support: Let's say, I want to bind a text field with a swt.Text widget. My EMF binding creates undoable commands for every change. I have two choices how to update the model: either update it on every keystroke or update it when the user leaves the text field or hits return. The first approach creates too many commands on the command stack. The second approach works fine, except for one case: when the file has not been edited and the user starts typing into the text field. In this case the editor does not know that the file has been modified (it usually observes the command stack), and therefore the save action is disabled. I solved the problem by registering two bindings for the text: one that listens to SWT.ValueChanged (to set the value in the model) and one that listens to SWT.Modify (to tell the editor, that the save button needs to be enabled, the Text field notifies the editor that is has "pending changes"). The editor also needs a way to tell the Text to set its value to the model, before the save is done (else the pending changes are not saved). That is simple: use Text.getText(). However the editor should not know all its Text fields, therefore the framework should to support it. To deal with that, I created a class CommandSupport (see below). It allows to register two types of listeners. Editors register a PendingChangesListener to get notified if a value changes. Text fields register a FlushPendingChangesListener to get notified before the editor wants to save (to put the pending changes into the model). The FlushPendingChangesListener has also a method hasPendingChanges() so that the Editor can figure out, if there are pending changes. In my framework, the equivalent to IUpdatable has a getCommandSupport() method: CommandSupport getCommandSupport(); It somehow belongs to the IDataBindingContext (my IUpdatable has also a method to access IDataBindingContext). But that's another topic... Here is my version of CommandSupport. I have updated a little bit, to make it compatible with jface.databinding. package org.eclipse.jface.databinding; /** * Text editors have their own undo stack. Text fields should * not update the model on every keystroke. However the editor * has to know if the model needs save or if an undo is allowed. * <p> * * Therefore the editor registers a * {@link CommandSupport.PendingChangesListener} * to get notified that there are pending changes. * * <p> * * {@link org.eclipse.jface.databinding.UpdatableValue} * that have pending changes register a * {@link CommandSupport.FlushPendingChangesListener} * to get notified when changes need to be flushed. * * */ public interface ICommandSupport { //---------------------------------------------------- // Support for Command Stacks //---------------------------------------------------- /** * @return true if there is a subeditor that is modified */ boolean hasPendingChanges(); /** * Listeners interested in changes of * {@link CommandSupport#hasPendingChanges()} * should register this interface */ public interface PendingChangesListener { /** * Tell the listener that the state of * {@link CommandSupport#hasPendingChanges()} * might have changed */ void pendingChangesChanged(); } /** * @param listener */ void addPendingChangesListener(PendingChangesListener listener); /** * @param listener */ void removePendingChangesListener(PendingChangesListener listener); /** * Called by a edit field that maintains a pending change. * This informs {@link PendingChangesListener} that the state * of pending Changes might have changed */ void pendingChangesChanged(); /** * Tell the {@link FlushPendingChangesListener} to flush their * pending changes. */ void flushPendingChanges(); //---------------------------------------------------- // Support for IUpdatableValue //---------------------------------------------------- /** * {@link org.eclipse.jface.databinding.IUpdatableValue} * that have pending changes should register a * FlushPendingChangesListener. * */ public interface FlushPendingChangesListener { /** * @return true if there is a pending change */ boolean hasPendingChanges(); /** * The owner of a pending change should put it into the model */ void flushPendingChanges(); } /** * @param listener */ void addFlushPendingChangesListener(FlushPendingChangesListener listener); /** * @param listener */ void removeFlushPendingChangesListener(FlushPendingChangesListener listener); }
*** Bug 118125 has been marked as a duplicate of this bug. ***
cc'ing the Undo/Redo owner on Platform UI.
> Let's say, I want to bind a text field with a swt.Text widget. My EMF binding > creates undoable commands for every change. I have two choices how to update > the model: either update it on every keystroke or update it when the user > leaves the text field or hits return. The first approach creates too many > commands on the command stack. The second approach works fine, except for one > case: when the file has not been edited and the user starts typing into the > text field. In this case the editor does not know that the file has been > modified (it usually observes the command stack), and therefore the save action > is disabled. I've been thinking about this. Based on what Michael has said, my approach of creating an IUndoableUpdatableFactory would work fine as long as the SWT factory was set to create SWT updatables with TIME_LATE flushing policies. However, the one thing he would still be missing is an isDirty() method to query the IUpdatable about if it has pending changes that need saved. Looks like we need another subtype of IUpdatable? public interface IUpdatableEditor extends IUpdatableValue { public boolean isDirty(); public boolean canSave(); // Calls the validator and returns the result public boolean save(); // Save right now; returns true if successful } We would need similar API on IUpdatableCollection or some subclass.
I've created bug #11823 to track issues related to needing editing life cycle events / status methods per comment #7. (In reply to comment #7) > > Let's say, I want to bind a text field with a swt.Text widget. My EMF binding > > creates undoable commands for every change. I have two choices how to update > > the model: either update it on every keystroke or update it when the user > > leaves the text field or hits return. The first approach creates too many > > commands on the command stack. The second approach works fine, except for one > > case: when the file has not been edited and the user starts typing into the > > text field. In this case the editor does not know that the file has been > > modified (it usually observes the command stack), and therefore the save action > > is disabled. > > I've been thinking about this. > > Based on what Michael has said, my approach of creating an > IUndoableUpdatableFactory would work fine as long as the SWT factory was set to > create SWT updatables with TIME_LATE flushing policies. However, the one thing > he would still be missing is an isDirty() method to query the IUpdatable about > if it has pending changes that need saved. > > Looks like we need another subtype of IUpdatable? > > public interface IUpdatableEditor extends IUpdatableValue { > public boolean isDirty(); > public boolean canSave(); // Calls the validator and returns the result > public boolean save(); // Save right now; returns true if successful > } > > We would need similar API on IUpdatableCollection or some subclass. >
Sorry; that was bug #118323. :-) (In reply to comment #8) > I've created bug #11823 to track issues related to needing editing life cycle > events / status methods per comment #7. > > (In reply to comment #7) > > > Let's say, I want to bind a text field with a swt.Text widget. My EMF binding > > > creates undoable commands for every change. I have two choices how to update > > > the model: either update it on every keystroke or update it when the user > > > leaves the text field or hits return. The first approach creates too many > > > commands on the command stack. The second approach works fine, except for one > > > case: when the file has not been edited and the user starts typing into the > > > text field. In this case the editor does not know that the file has been > > > modified (it usually observes the command stack), and therefore the save action > > > is disabled. > > > > I've been thinking about this. > > > > Based on what Michael has said, my approach of creating an > > IUndoableUpdatableFactory would work fine as long as the SWT factory was set to > > create SWT updatables with TIME_LATE flushing policies. However, the one thing > > he would still be missing is an isDirty() method to query the IUpdatable about > > if it has pending changes that need saved. > > > > Looks like we need another subtype of IUpdatable? > > > > public interface IUpdatableEditor extends IUpdatableValue { > > public boolean isDirty(); > > public boolean canSave(); // Calls the validator and returns the result > > public boolean save(); // Save right now; returns true if successful > > } > > > > We would need similar API on IUpdatableCollection or some subclass. > > >
Like Michael Scharf, I have also tried binding SWT controls to EMF model objects and stumbled upon the undo-redo problem accounted for in comment #4. First, my solution utilizes the EMF command framework. Like the JFace data binding framework, my framework listens to SWT events for the bound controls and generates change events. On these change events, the model is changed by executing commands (EMF's SetCommand) on the command stack. Now, the interesting part is that change events that generated by keystrokes are marked as temporary; on these events, the model is changed by executing a SetCommand which is wrapped as a TemporaryCommand (based on EMF's CompoundCommand). On succeeding change events on the same binding, the TemporaryCommand is undone and a new SetCommand is executed on the stack with the new value from the text field (again as a TemporaryCommand if the event signified a temporary change caused by a keystroke, or a plain SetCommand if the text field lost focus). I found this solution to simplify my editor implementation, because the editor's dirty state is defined by the state of the command stack without having to keep track of pending changes. Perhaps you may find this concept of temporary changes appropriate in your design of the data binding framework, perhaps it is an abstraction best left out of JFace.
The platform already provides a complete undo/redo implementation, including support for composite operations. Please refer to the platform documentation for "Advanced workbench concepts->Undoable operations", plus IUndoableOperation and IOperationHistory. These classes use an approach similar to comment #10, but instead of removing and replacing temporary command objects, the smaller objects are accumulated into a larger ICompositeOperation. IOperationHistory.openOperation(ICompositeOperation ...) IOperationHistory.execute(IUndoableOperation ...) ... IOperationHistory.execute(IUndoableOperation ...) IOperationHistory.closeOperation(...) The main difficulty with using this API is the integration with client code. IOperationHistory expects various parameters that the binding framework has no business touching. Perhaps the binding framework should respond to trigger events by building IUndoableOperation instances and passing them to client code. The clients can deal with the IOperationHistory protocol or simply call execute() and ignore undo/redo.
What if the read/write methods have side effects other than changing the named property value? setX(oldValue) is not necessarily the inverse of setX(newValue), and (getX() == getX()) is not reliable in a multi-threaded environment. The binding framework can't even assume that IOperationHistory implements simple stack behavior. For anything but the most trivial cases, client code must interact with undo/redo at a fine-grain level. Perhaps automatic undo/redo support is best as an optional bolt-on feature that should only be used when client code can satisfy a well-documented behavioral contract.
(In reply to comment #11) > The platform already provides a complete undo/redo implementation, including > support for composite operations. Please refer to the platform documentation > for "Advanced workbench concepts->Undoable operations", plus > IUndoableOperation and IOperationHistory. Our take on this has been that the data binding framework should not mandate a particular undo/redo implementation, or even that operations are undoable at all, but that it should be possible to make whatever ends up on the model side undoable. This was one of the motivators for the big refactoring we did back in 3.2M6 - we wanted to ensure that it is possible to intercept all write accesses to model objects by wrapping the IObservableFactory that creates observables for the model side within another factory that is able to create intercepting IObervables.
Interesting idea. Someone (perhaps the framework) could provide an UndoableObservableFactory that intercepts the factory methods and decorates arbitrary IObservables. The decorated classes could implement a complete undo/redo regime, assuming that the IObservables follow a straightforward contract. (I.e., invertible set behavior, etc.) This would dramatically reduce the code necessary to implement basic default behavior. Configuration would be encapsulated at the factory. Client code could either extend, replace, or ignore the factory to implement their own undo behavior.
(In reply to comment #14) > Interesting idea. Someone (perhaps the framework) could provide an > UndoableObservableFactory that intercepts the factory methods and decorates > arbitrary IObservables. The decorated classes could implement a complete > undo/redo regime, assuming that the IObservables follow a straightforward > contract. (I.e., invertible set behavior, etc.) Yes. I believe this is how Dave did it, but I am not sure what the state is with regards to the IP issues he mentioned in comment #3. Note that this can only work if you know which observables are returned from the factory you are wrapping, e.g. if you know it only returns IObservableValue/List/Set/Mapping instances, you can return wrappers for these.
(In reply to comment #15) > (In reply to comment #14) > > Interesting idea. Someone (perhaps the framework) could provide an > > UndoableObservableFactory that intercepts the factory methods and decorates > > arbitrary IObservables. The decorated classes could implement a complete > > undo/redo regime, assuming that the IObservables follow a straightforward > > contract. (I.e., invertible set behavior, etc.) > > Yes. I believe this is how Dave did it, but I am not sure what the state is > with regards to the IP issues he mentioned in comment #3. The IP is clean and could be contributed. But it was written 6 months ago and was never migrated to the post-M5 refactored codebase. The upshot: it basically needs to be rewritten now.
(In reply to comment #14) > Interesting idea. Someone (perhaps the framework) could provide an > UndoableObservableFactory that intercepts the factory methods and decorates > arbitrary IObservables. The decorated classes could implement a complete > undo/redo regime, assuming that the IObservables follow a straightforward > contract. (I.e., invertible set behavior, etc.) This would dramatically > reduce the code necessary to implement basic default behavior. Configuration > would be encapsulated at the factory. Client code could either extend, > replace, or ignore the factory to implement their own undo behavior. Yes, this is how I did it. This works really well. It's really cool to be able to hit Ctrl-Z and watch your edits automatically be undone. It's extra cool to know that it all works automatically using Eclipse's centralized undo/redo stack and all you did to support it was use data binding and set up some factories.
I'm assuming this feature isn't going to make it into the 3.3 release. But comment #17 seems to indicate that there is a somewhat painless way to get this working. Is there any way you could provide some sample code that illustrates how to integrate undo/redo with the final databinding API. Thanks.
Here's one idea. No time for code yet, but maybe it'll get you started. Basically, in order to get Undo/Redo working, you have to intercept all copy to model writes, buffer the current value (your undo information), then execute the actual copy-to-model write inside an Eclipse command on the Eclipse undo/redo stack. Undo is implemented by setting the old value back into the model. Assuming your model has proper property change support, this will automatically trigger a data binding refresh back to your UI. Redo is implemented by re-setting the value back on the model object. Same deal--your model objects will signal a change and data binding will update the UI. In order to make this work, you need a set of IObservable[Value|List|Set] objects that implement this behavior and a factory that can create these. So instead of creating your model observable using BeansObservables, you will use your own UndoableBeansObservables factory. Hope this gets you started...
I've been thinking about whether I want to implement this myself, and there are a few subtleties here that make the problem complex. First is the issue of "event compression" -- merging many fine-grained change events into more useful ICompositeOperations. It would be abusive to force users to undo each keystroke in a text change individually. A naive approach would be to open a new ICompositeOperation for the first operation on a particular IObservable and close it when a change occurs on any other IObservable. However, this might compress too much -- typing a sequence of characters and deleting a different sequence should be separate operations. Detecting this requires custom logic for String data and a small state machine. Off the top of my head, I can't think of any other case where event compression is necessary for undo. A special case just for Strings might be reasonable. The second issue is "external updates". By this I mean changes to the model that occur for some reason other than user input. This might occur as a result of a change by a remote user or a system process. I honestly don't know what the user-friendly response to this might be. It is unintuitive that a user should be able to "undo" something that they didn't "do" in the first place. There could also be bad interactions if the user is in the process of editing a value that is externally updated. Thirdly, you would need to deal with undo contexts. In the good ol' days of a global undo stack and purely linear undo/redo, there was no issue with reordering operations. The Eclipse operations framework does not guarantee this simple behavior. Most Eclipse IDE editors have a separate local undo context that integrates with a global context. Most refactorings and operations on views go to different contexts. It is attractive to implement the undo decorator on the model observables, since the model is a convenient single point of control for all changes. If the model is shared by multiple editors, it will not know the context from which a change occurred. Even if the correct context is known, there can be problems if changes to the same model are reordered. Unless changes are strictly represented as diffs, and undo/redo affects only the minimum subset of data directly affected by a change, stale data can be accidentally brought back when operations are reordered. Fourth, data stored as undo/redo context must be immutable. A naive approach that just stores a reference to the values changed by setValue() calls would fail, since the stored context might be changed while it is inside the undo history. Another editor might change the value, thereby redefining the "old" state. When the old value is "restored" by undo, the model might not actually be changed at all, since the "old" value is actually a reference to the same object as the "current" value. The undo stack must freeze the state of any context it stores. Serialization will not work, since undo/redo would then reset the transient state of any objects that were serialized (including the PropertyChangeListeners that drive the undo decorators). Serialization would also be likely to capture too much context, leading to the stale data issues mentioned above. Considering these issues, I'm not surprised that the DataBindings team decided not to take this on. I suspect that a reliable solution would need to take into account a number of application-specific policy decisions.
Has anything happened with this?
Dave, do you still plan to work on this? I would mark it as WONTFIX, arguing that proper undo/redo support can only be implemented in the model layer and is therefore out of scope for data binding.
I've been thinking about this. The original design isn't valid, but the kernel of the idea still is. Basically, for an operation to be undoable, it has to be wrapped inside some sort of undoable command object, and then executed by Eclipse's undo/redo stack handler. This could be implemented similarly to the current WritableList and WritableValue implementations: Create an IObservableValue that wraps a regular IObservableValue. Instead of mutating the underlying value directly, it places a command object on the Eclipse undo/redo stack to change values (after buffering the old value, for undo purposes). An IObservableList, IObservableSet, etc., could be implemented similarly. Yes, I'm interested in implementing this and there's hope that I might get some bandwidth to do it during the 3.5 release cycle. If someone else gets bandwidth before I do and wants to take a crack at this, feel free to contact me. It doesn't look like it would be too hard to do.
(In reply to comment #20) > I've been thinking about whether I want to implement this myself, and there are > a few subtleties here that make the problem complex. These are also good points. I haven't spiked anything yet, but my approach to dealing with these would be to try *not* to deal with them. IE: 1) A given IUndoableObservable would have API to associate it with a particular undo/redo context, but nothing more. It's the programmer's job to figure out how all of the concerns you enumerated apply to their particular situation. 2) There would be API on the IUndoableObservables to begin/end batching composite undo operations, but again it would be up to the programmer to call those APIs at the right time, for the values of "right" that apply to his/her particular application. Thoughts?
What about a transaction API, e.g. with similar semantics to ObservableTracker.runAndMonitor. That is, as observables are accessed from within the transaction runnable, we listen for changes to those observables and log each mutation as part of a single command on the undo/redo stack. The drawback is that this may not play nice with bindings. Another option is to wrap each observable in a TransactionObservableValue/Set/List/Map, which would make the transaction semantics more explicit and less magic (and less dependent on proper implementation of @TrackedGetter methods in third party code). In this way a single object could be made to receive all interesting mutations, allowing the user to place them on the Eclipse undo stack or any other undo stack for that matter. The JFace plugin could offer built-in support for the Eclipse command stack. These are just rough ideas...
I'm wondering if the right answer to this is: Use EMF + data binding. EMF already supports undo/redo. Thoughts?
Yes, it does (via EMFEditObservables). Unfortunately, the EMF + databinding combination has an undesired behavior. Currently, all change events generated by keystrokes are triggering an EMF SetCommand instead of when the widget loses the focus. So, it seems that something needs to be done in the JFace databinding API so each key stroke does not change the model.
(In reply to comment #27) > Yes, it does (via EMFEditObservables). Unfortunately, the EMF + databinding > combination has an undesired behavior. Currently, all change events generated > by keystrokes are triggering an EMF SetCommand instead of when the widget loses > the focus. Does this happen even with: SWTObservables.observeText(aText, SWT.FocusOut) ?
Sorry Dave, I missed that. It actually works as desired with SWTObservables.observeText(aText, SWT.FocusOut) So, yes, at least for me the right approach is to use EMF + databinding since EMF already takes care of the undo/redo functionality. Thanks!
Does it really work with SWTObservables.observeText(aText, SWT.FocusOut)? Some time ago I had two problem with that (not with databinding, but with my own inplementation): 1. I implemented an editor. Unless the user change anything the save state is disabled. So, if the user starts editing a field and then tries to save the editor it does not work (it is disabled), because the save is enabled only after you leave the field (and FocusOut is triggered).... 2. The other problem was, that Save did not trigger a FocusOut and therefore information in the last edited field was not saved. I wonder how databinding solves both problems...
Well, in my case works because I don't have Save button so I am relying on FocusOut to update my EMF model, however, there are situations where the model is not updated because no FocusOut-Event occurs (i.e.: a hit on the Save button is such a case). So, as Tom Schindl mentioned it, before doing any save action you'll need to ensure that the field holding the current focus is updating the model programmatically by calling updateTargetToModel().
I'm wondering if listening to SWT.DefaultSelection in the observeText(SWT.FocusOut) observable would fix this.
(In reply to comment #27) > Yes, it does (via EMFEditObservables). Unfortunately, the EMF + databinding > combination has an undesired behavior. Currently, all change events generated > by keystrokes are triggering an EMF SetCommand instead of when the widget loses > the focus. > > So, it seems that something needs to be done in the JFace databinding API so > each key stroke does not change the model. > See Bug 180746 which appears to have made it into 3.4 - that provides an API to batch text modify events at the Data Binding layer. A while back I proposed Bug 206773 asking for an API on Text widgets (at the SWT level) to batch modify events. It was denied because of minimalist philosophy.
(In reply to comment #30) > Does it really work with SWTObservables.observeText(aText, SWT.FocusOut)? > > Some time ago I had two problem with that (not with databinding, but with my > own inplementation): > > 1. I implemented an editor. Unless the user change anything the save state is > disabled. So, if the user starts editing a field and then tries to save the > editor it does not work (it is disabled), because the save is enabled only > after you leave the field (and FocusOut is triggered).... That's a real nagging problem. For our 3.3-compatible version we are treating it as a "user training issue." I plan to address it when we move to requiring 3.4 as a base for our product. > 2. The other problem was, that Save did not trigger a FocusOut and therefore > information in the last edited field was not saved. I only encountred this as a problem if the editor is closed - save seems to trigger the FocusOut event (at least, it does in a Forms-based editor). To solve the problem of no FocusOut event when the editor is closed (via X button), I override EditorPart.isSaveOnCloseNeeded() like this: @Override public boolean isSaveOnCloseNeeded() { // If one of this editor's widgets has focus, make sure it fires change notification to // update the model before asking if save is needed. Control focusedChild = EclipseUIUtils.getFocusedChild(getActivePageInstance().getManagedForm().getForm()); if (focusedChild != null) { focusedChild.notifyListeners(SWT.FocusOut, null); } return super.isSaveOnCloseNeeded(); }
(In reply to comment #34) > That's a real nagging problem. For our 3.3-compatible version we are treating > it as a "user training issue." I plan to address it when we move to requiring > 3.4 as a base for our product. You could also copy the code for DelayedObservableValue and use it in your 3.3-compatible version. As for the missing FocusOut events in some cases, I think it would be good to start another round of discussions with the SWT team about this. For this discussion, we should write a SWT only snippet that demonstrates the problem. We need to be specific: under which circumstances, and on which platforms do we expect a FocusOut event but don't receive one?
I'll try to tackle this in 3.6. Another problem that I don't think has been mentioned, is that if you try to implement this by just listening to the model observables, that this will break down as soon as you use undo (pushing a command to the undo stack that you didn't want) or change the selection in a master-detail scenario (pushing a command to the undo stack for every detail observable that changed value. Currently the plan is to introduce some decorating observables that will push commands to the undo/redo stack whenever a setter/mutator method is called. Thus: IObservableValue personSelection = ViewerProperties.singleSelection().observe(personViewer); ctx.bindValue( WidgetProperties.text(SWT.Modify).observeDelayed(400, nameText), WorkbenchObservables.observeUndoableValue( BeanProperties.value("name").observeDetail(personSelection)); An important distinction is that a command will only be pushed to the stack when a mutator method is called, e.g. IObservableValue.setValue. No command will be pushed on any events received from the underlying observable, as this may represent a command being undone, or perhaps some change initiated by a command elsewhere.
Looking at this again, there is a problem with the approach laid out in comment #36. Take the following scenario: TableViewer personViewer = ... Text personNameText = ... IObservableList people = ... ViewerSupport.bind(personViewer, persons, BeanProperties.value("name")); IObservableValue personSelection = ViewerProperties.singleSelection().observe(personViewer); bindingContext.bindValue( WidgetProperties.text(SWT.Modify).observeDelayed(400, personNameText), WorkbenchObservables.observeUndoableValue( BeanProperties.value("name").observeDetail(personSelection) ) ); Now suppose you select the person "Joe" and change the name to "Joseph". The undoable value observable creates a command changing the name from "Joe" to "Joseph" on the child observable. The command is pushed to the undo/redo stack and executed. Now select the person "Mike." There is no undoable command pushed because the change took place internally--this is what we want. Now click undo. Oops! Mike's name has been changed to Joe. This is because the command is not aware of which Person object is actually being changed--it is only aware that there is an observable that needs to change values, but that observable is now representing Mike whereas the command is really intended for Joe. Therefore our solution has to take into account the actual model objects that the changes apply to (Joe in this case), the property being changed (BeanProperties.value("name")), and the changes applied (s/Joe/Joseph/). The property API seems a good fit for this kind of thing, but I'm not sure how to make it work for the moment. Suggestions welcome :)
We would have to add new methods to IValueProperty to make it possible for an undoable property wrapper to undo a command. Otherwise we run the risk of trying to undo a change on an observable that could be disposed. The problem is that these methods are not present in 3.5, so when adding them in 3.6 to class ValueProperty they would have to be non-abstract, even though ValueProperty cannot possibly know how to implement them. The best we could do is throw UnsupportedOperationException.
To clarify, the new methods would be: public Object getValue(Object source); public void setValue(Object source, Object value); These methods are already present in SimpleValueProperty but are not exposed through the IValueProperty interface, nor are they present in the ValueProperty base class.
Created attachment 141911 [details] Screencast showing Matt's test-case This does not seem to occur if you use the EditingDomainEObjectObservableValue (e.g. class UndoableObservableValue extends EditingDomainEObjectObservableValue) IObservableValue targetObservableValue = observeText(control); UndoableObservableValue modelObservableValue = new UndoableObservableValue(target, propertyDescriptor); .... .... Binding binding = dbc.bindValue(targetObservableValue, modelObservableValue, targetToModel, modelToTarget); targetToModel.setBinding(binding); The attached screencast shows our detail view on the right; which is created using the databinding API, and the undo affects only the correct element. Unfortunately, this might not be of much help since only works if you are using EMF. However, I thought this might be helpful to someone.
(In reply to comment #40) > Unfortunately, this might not be of much help since only works if you are using > EMF. However, I thought this might be helpful to someone. Exactly. And the reason this works for EMF is that the commands send to the undo/redo stack operate in terms of the EObject and EStructuralFeature, not in terms of the IObservableValue. Using properties I think we could duplicate this in a generic way and make it available to clients using beans instead of EMF.
Bug 285307 introduces API that we will need in place for an effective solution here. Those cc'd here are encouraged to read bug 285307 comment 11 which is relevant to this bug.
Removed from 3.6. Owners can re-assess. PW
I see nothing has happened on this and it has been a couple of years since the last comment. I'm not surprised as I agree that the proposed design is flawed. It has been discussed that the observables will likely no longer be around when an operation is undone. However the object itself may not be around either. Consider the following: - (1) Add an instance of a new object to the model - (2) Change a property of another model object to reference that new object - (3) Change that property reference again to be null - (4) Delete the new object - undo the fourth change - undo the third change The last undo operation will fail if the command (3) had kept a reference to the previous value. When (4) is undone, depending on the persistence layer, a new object may be created. If the model layer is responsible for creating new model objects then it will not be possible to re-use the old instance of the Java object. If the object has been deleted from an underlying database and then re-created in the database then it is likely that an auto-increment id assigned by the database would be different, so we can't key off that. I also believe that undo/redo should not be done as a part of data binding. I have implemented a couple of frameworks that support undo/redo. Although it is a layer that goes between the model and the controller, in my experience it works best when integrated more in the model than it the controller. Note that not everyone uses data binding. Even in projects where data binding is in widespread use, features such as data import as generally implemented without data binding. The undo/redo should put everything on the command stack, and imports must be undone/redone as a single operation. This means there must be transaction support in the framework. In my opinion, undo/redo is an integral part of model transactional support. There is much discussion in this bug report about changing the bindings on text fields from SWT.Modify to SWT.FocusOut. However I think that discussion just shows that doing undo/redo as a part of data binding is really not appropriate. There are very good reasons for binding using SWT.Modify apart from the issues already covered in this bug. For example there may be error indicators that need to be updated as the user types. If you are already using transactional support for database persistence (you probably wouldn't want data to be written to the database on each keystroke) then putting undo/redo into that layer makes far more sense. You simply pass a short description to the commit() method and that description is used as the label for the undo/redo. I am involved in an open source project (jmoney.sf.net) in which I wrote transactional support which included undo/redo support. It is old code (written in 2004 without data binding), but it has been very reliable. I have started hacking around on it and I believe I can produce from it a good general purpose framework that provides undo/redo and that will be IP clean. It will use the Eclipse data binding but will not be a part of data binding. I'll keep you posted.
(In reply to comment #44) > > I am involved in an open source project (jmoney.sf.net) in which I wrote > transactional support which included undo/redo support. It is old code > (written in 2004 without data binding), but it has been very reliable. I > have started hacking around on it and I believe I can produce from it a good > general purpose framework that provides undo/redo and that will be IP clean. > It will use the Eclipse data binding but will not be a part of data binding. > I'll keep you posted. Thanks Nigel. PW
