Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 240775

Summary:

ResourceUndoContext should be more flexible regarding affected resource policy

Product:

[Modeling] EMF Services

Reporter:

Sébastien Gandon <sgandon>

Component:

Transaction

Assignee:

Christian Damus <give.a.damus>

Status:

VERIFIED FIXED

QA Contact:

Severity:

enhancement

Priority:

CC:

ahunter.eclipse, ldamus

Version:

unspecified

Keywords:

plan

Target Milestone:

---

Hardware:

OS:

Windows XP

Whiteboard:

Client Control

Bug Depends on:

Bug Blocks:

252378

Attachments:

Description	Flags
Proposed API for resource undo-context policy	give.a.damus: review?

Description Sébastien Gandon

2008-07-15 03:31:22 EDT

The ResourceUndoContext should be enhanced to accept a policy that do not add the cross reference model in the affected resource during a "Set"

summary of the discussion on the emf new group : http://www.eclipse.org/newsportal/article.php?id=33685&group=eclipse.tools.emf#33685

Hello all,
I am using a TransactionnalEditingDomain to edit 2 different models at the
same time.
One model as Elements say ExecutionProfiles that have an attribute called
middleware which refer to a Middleware instance in the second model.
My problem is when I set the attribute in the first model, the second
model is added in the affected resource of the undo context or only the
first model containing ExecutionProfiles.

I have looked at the ResourceUndoContext that confirm that fact, the
handleCrossResourceReference() method adds the new value and old value
resource in the affected list or the resource has not been affected?
I use the ResourceUndoContext.getAffectedResources(event.getOperation())
when I am notified of was hopping filtering event out when the resource my
editor deal with does not belong to the affected resources, but it
considered affected although nothing changed in the resource.

------------------------------------------answer from Chrstian.

The purpose of this behaviour is to ensure, as much as possible,
consistency of the referencing resource's state (including its editor's
undo stack) when operations in the referenced resource's editor are
undone, because the referencing editor's operations are dependent on the
history of changes in the referenced editor.

As a concrete example from your metamodel, consider this case:

1. In resource A, a Middleware object named Foo is created.
==> this operation "affects" resource A
2. In resource B, an ExecutionProfile object named Bar is changed by
adding Foo to its middleware reference.
==> this operation "affects" both A and B resources

Consider what happens when the user invokes the "Edit -> Undo" menu
action in the editor of resource A. Because the modification of the
execution profile Bar in resource B appears in A's undo menu, the first
undo operation in A removes Foo from Bar's middleware reference.
Another undo deletes class Foo from A.

Now, imagine that operation #2 was not added to the A editor's context.
The first undo in A would delete the Middleware object Foo. Now Bar in
B is basically corrupt, because it references a Foo that no longer
really exists. Saving B will result in a dangling HREF exception.
Other operations subsequently performed in B could be dependent on Bar's
reference to Foo, and be invalidated.

On Mon, 2008-07-14 at 22:21 +0000, SeB.fr wrote:

Hi Christian,

Thanks for your answer but I do not agree with this policy.
You never can avoid dandling Href, if user delete the Middleware named
foo, after steps 1 and 2, you get this dandling href. And I am ok with
that.
I think too much is done during this "set" of a reference.
To me both models should be independent in terms of modifications.
Nothing is modified in resource A when doing the "set" in resource B so
resource A should not be part of the affected resource.
Do you have an idea on how to avoid this behaviour apart from modifiying
the ResourceUndoContext class ?

SeB.

Actually, the DeleteCommand will affect both resources because it uses a
UsageCrossReferencer to clear all references to the object being
deleted, but that's beside the point :-)

I understand that perhaps you don't need such a pessimistic algorithm.
I think you can just use some other undo context for your editors
instead of the ResourceUndoContext, but you would probably find yourself
re-implementing a good deal of what the WorkspaceCommandStackImpl does
in computing the ResourceUndoContexts and attaching them to the
operations.

Perhaps you could raise an enhancement request for more flexibility in
this algorithm, and we'll see what we can do in the 1.3 release.

Cheers,

Christian

Comment 1 Christian Damus

2008-10-29 21:44:00 EDT

I think this mechanism should be able to help GMF to sort out some problems in managing the editor's dirty state according to its custom ModificationTrackingAdapter rules (cc'ing Linda fhi).

Comment 2 Linda Damus

2008-10-31 10:30:23 EDT

Thanks for drawing this to my attention, Christian!

This is exactly what we need in GMF. As I wrote on the EMF newsgroup (http://www.eclipse.org/newsportal/article.php?id=36900&group=eclipse.tools.emf#36900), in GMF we use the isModified state of the resource to determine whether or not a given GMF editor is dirty.
GMFResource overrides ResourceImpl.ModificationTrackingAdapter to exclude notifications on transient features from changing the modified state of the GMFResource.

In order for me to fix bug 149214, which will remove the dirty marker from GMF editors when changes to the resource are undone back to the last save point, my approach has been to make use of the ResourceUndoContexts added to undoable operations by the WorkspaceCommandStackImpl.DomainListener. I have assumed that if an operation has a ResourceUndoContext for a given resource, then the operation has in some way changed the state of that resource. Once all operations with the context for a given resource have been undone back to the last known save point, if the resource is in a GMF editing domain I will set the isModified state back to false.

For this to work, GMF needs the logic that sets the modified state to true to correspond to the logic that adds ResourceUndoContexts to operations on the history.

With this enhancement to WorkspaceCommandStackImpl, I expect GMF will be able to provide its own policy for when to add the ResourceUndoContexts for operations affecting its resources. Our policy will exclude notifications on transient features.

Anthony, I've added you to the CC list, just FYI.

Comment 3 Christian Damus

2008-11-01 13:16:39 EDT

(In reply to comment #2)
> Thanks for drawing this to my attention, Christian!
> 
> This is exactly what we need in GMF.  As I wrote on the EMF newsgroup
> (http://www.eclipse.org/newsportal/article.php?id=36900&group=eclipse.tools.emf#36900),
> in GMF we use the isModified state of the resource to determine whether or not
> a given GMF editor is dirty.

Good!  That was the conversation that reminded me of this bug in the first place.  I'm glad to see more interest in it.

> In order for me to fix bug 149214, which will remove the dirty marker from GMF
> editors when changes to the resource are undone back to the last save point, my
> approach has been to make use of the ResourceUndoContexts added to undoable
> operations by the WorkspaceCommandStackImpl.DomainListener.  I have assumed
> that if an operation has a ResourceUndoContext for a given resource, then the
> operation has in some way changed the state of that resource.  Once all

You mean the persistent state of the resource, right?  As I understand it, GMF does some funkiness with transient containment (which is an odd notion, IMHO).

> operations with the context for a given resource have been undone back to the
> last known save point, if the resource is in a GMF editing domain I will set
> the isModified state back to false. 

Sounds reasonable.

> For this to work, GMF needs the logic that sets the modified state to true to
> correspond to the logic that adds ResourceUndoContexts to operations on the
> history.

I suppose that GMF's resource implementation and undo-context strategy (implementing the new API of this bug) would share a single definition of these rules.

> With this enhancement to WorkspaceCommandStackImpl, I expect GMF will be able
> to provide its own policy for when to add the ResourceUndoContexts for
> operations affecting its resources.  Our policy will exclude notifications on
> transient features.

M3 is coming soon.  Could I ask you to evaluate a patch when it is ready, so that this can be committed for the milestone (Wed. 5 Nov.)?

Comment 4 Christian Damus

2008-11-01 15:41:34 EDT

Created attachment 116700 [details]
Proposed API for resource undo-context policy

Attached a proposed new API for client control over the undo-context policy.

The basic idea is to extract the static ResourceUndoContext::getAffectedResources(...) API into a policy object that determines what resources are in an operation's undo context.  Thus, we have an object that can be provided by clients with a default implementation supplied that behaves as the 1.2 release.

The new IResourceUndeContextPolicy interface defines the protocol for this policy object.  A default implementation is supplied that works as before, additionally resolving the isTouch() issue (bug 252378).  Clients can plug in their own implementations by overriding a new getResourceUndoContextPolicy() method in the WorkspaceEditingDomainFactory.

The AbstractResourceUndoContextPolicy class is the default implementation, with convenient override hooks for:

  - the directed cross-resource reference issue for which this bug
    was originally raised
  - handling of certain Resource properties that don't affect serialization

I would like that somebody provide feed-back on this patch before I proceed any further.

Comment 5 Linda Damus

2008-11-02 13:02:01 EST

I've tried out the proposed API, and I'd like to request just one small change.  Could you refactor the isTouch check in AbstractResourceUndoContextPolicy out into a new protected method?  That will make it very simple for me to extend the criteria that determine whether or not a given notification should be ignored by the algorithm.  In GMF we need to exclude notifications on transient features.

Apart from that, the new API looks great for what we need in GMF.  Thanks!

Comment 6 Christian Damus

2008-11-02 13:08:13 EST

Yes, of course, that's an obvious hook that I should have provided in the first place.

Thanks for testing the patch!

Comment 7 Christian Damus

2008-11-02 13:46:33 EST

Committed the patch to HEAD (1.3 release) branch, with changes:

  - added an isAbstractChange(Notification) hook to factor out the
     !isTouch() criterion
  - factored out the handling of resource and object events into
     resourceChange(...) and objectChange(...) methods, respectively.
     Also added resourceSetChange(...) because, well, you never know
     what folks may need and these events were ignored, before

Comment 8 Christian Damus

2008-11-03 11:40:07 EST

Fix available in HEAD: 1.3.0.I200811021821.

Comment 9 Christian Damus

2008-11-24 13:17:32 EST

Restore original target after milestones were deranged.