Bug 460206 - Have Transaction.getChangeDescription() usable in postcommit
Summary: Have Transaction.getChangeDescription() usable in postcommit
Status: RESOLVED FIXED
Alias: None
Product: EMF Services
Classification: Modeling
Component: Transaction (show other bugs)
Version: unspecified   Edit
Hardware: PC Linux
: P3 normal
Target Milestone: ---   Edit
Assignee: Christian Damus CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-02-18 04:52 EST by Esteban DUGUEPEROUX CLA
Modified: 2017-02-24 15:11 EST (History)
2 users (show)

See Also:
ahunter.eclipse: mars+


Attachments
Patch (10.98 KB, patch)
2015-02-18 04:52 EST, Esteban DUGUEPEROUX CLA
no flags Details | Diff
Updated Esteban's patch with a complete fix (keeping the test cases) (17.91 KB, patch)
2015-02-21 21:34 EST, Christian Damus CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Esteban DUGUEPEROUX CLA 2015-02-18 04:52:43 EST
Created attachment 250905 [details]
Patch

When using a postcommit, i.e. ResourceSetListener, it would be useful to have InternalTransactionalEditingDomain.getActiveTransaction().getChangeDescription() usable.
In use case where main command and precommits have done many model elements addition/removal, notifications analysis can be costly. The TransactionChangeRecorder do already this notifications analysis to compute its ChangeDescription then it will be useful to have it usable.

But currently in postcommit InternalTransactionalEditingDomain.getActiveTransaction().getChangeDescription() return null and using it in ValidateEditSupport.finalizeCommit() is too early as stopRecording() on the TransactionChangeRecorder is not yet called.
In addition ChangeDescription.getObjectsToDetach() doesn't returns objects attached during precommits.

This is due to CommandChangeDescription and TriggerCommandTransaction in which we clear the ChangeDescription to not propagate it to parent transaction.
The advantage of CommandChangeDescription/TriggerCommandTransaction is that it avoid keeping ChangeDescription in memory for precommits to manage undo/redo but the drawback is that TransactionChangeDescription accessors cannot be used as it is usable for a ChangeDescription outside EMF Transaction.

I have attached a patch with a JUnit test class and a draft of fix which is not satisfying yet.

See https://www.eclipse.org/forums/index.php/t/989994/ for a discussion with Christian Damus.
Comment 1 Christian Damus CLA 2015-02-21 21:34:38 EST
Created attachment 250996 [details]
Updated Esteban's patch with a complete fix (keeping the test cases)

Hi, Esteban,

I have uploaded a new patch based on your patch that keeps your test case and fixes the reporting of changes in the CompositeChangeDescription to include all changes from trigger commands.

Your test class looks good.  Interesting use of a post-commit listener that is a ValidateEditSupport:  I've never seen anything like that before!  ;-)   I don't know why you think it's necessary, though:  the Transaction provided by the ResourceSetChangedEvent always still has its ChangeDescription.  It certainly works for me in the update I made to your test case.

The core expressions dependency that you added to the test plug-in manifest seems to be extraneous; I removed it in this patch.

The key to the problem is that, because a trigger command is expected to supply its own undo/redo behaviour (as the AddCommand does in your test case), the parent transaction does not want to include the change description that was recorded for the trigger in its own changes.  Otherwise, it would conflict with the command when undoing and redoing.  That's why the trigger transaction clears its change description at commit.

However, we do need to report the changes via the ChangeDescription API.

So, I have changed the CompositeChangeDescription to add a concept of "detaching" its child changes.  Instead of clearing its children, it puts them away off to the side.  They won't participate in undo/redo (as before when they were just cleared) but they are still used to compute the changes for the ChangeDescription's various list features.

The org.eclipse.emf.transaction.tests and org.eclipse.emf.workspace.tests suites are green with this patch.
Comment 2 Esteban DUGUEPEROUX CLA 2015-02-23 03:03:46 EST
Hi Christian,

Thanks for your help, I have tested your patch and it works fine.
I was using a ValidateEditSupport to get the transaction because the ResourceSetChangeEvent.getTransaction().getChangeDescription() API was unknown to me :).
Do you think this patch could be included in Eclipse Mars release?

Best Regards.
Comment 4 Esteban DUGUEPEROUX CLA 2015-11-18 11:29:35 EST
Hi,

A remark about CompositeChangeDescription.getObjectsToDetach(), why each call to this method do some computation while a call to CompositeChangeDescription.getObjectChanges() or CompositeChangeDescription.getObjectsToAttach() do some computation only for the first call. This can have impact on performance if for example many postcommit listeners use CompositeChangeDescription.getObjectsToAttach() API with many detachements done in an EMF command. Could we have CompositeChangeDescription.getObjectsToAttach() implementation do its computation only at the first call like other getters?

Best Regards.