Bug 283131 - Let CDOObjects send normal EMF notifications on rollback
Summary: Let CDOObjects send normal EMF notifications on rollback
Status: CLOSED FIXED
Alias: None
Product: EMF
Classification: Modeling
Component: cdo.core (show other bugs)
Version: 4.0   Edit
Hardware: All All
: P3 enhancement (vote)
Target Milestone: ---   Edit
Assignee: Eike Stepper CLA
QA Contact: Eike Stepper CLA
URL:
Whiteboard:
Keywords: noteworthy
Depends on:
Blocks:
 
Reported: 2009-07-10 07:42 EDT by Caspar D. CLA
Modified: 2011-06-23 03:41 EDT (History)
7 users (show)

See Also:


Attachments
Patch v1 (5.25 KB, patch)
2011-01-27 04:34 EST, Eike Stepper CLA
no flags Details | Diff
Testcase v1 (11.07 KB, patch)
2011-01-27 11:28 EST, Pascal Lehmann CLA
no flags Details | Diff
Testcase v2 - ready to be committed (11.02 KB, patch)
2011-01-27 11:42 EST, Eike Stepper CLA
no flags Details | Diff
Enables EMF notifications on transaction.rollback() (2.76 KB, patch)
2011-01-28 04:52 EST, Erwin Betschart CLA
no flags Details | Diff
Testcase v3 - ready to be committed (74.91 KB, patch)
2011-01-28 05:24 EST, Martin Fluegge CLA
no flags Details | Diff
Enables EMF notifications on transaction.rollback() (3.03 KB, patch)
2011-01-28 10:42 EST, Erwin Betschart CLA
stepper: iplog+
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Caspar D. CLA 2009-07-10 07:42:26 EDT
Currently, when #rollback is called on a CDOTransaction, the CDOObjects that are restored from their DIRTY state to their CLEAN state, do not send EMF Notifications to their eAdapters.

This is problematic, for example, if the affected CDOObjects are being displayed in a UI, as any presentation layer depending on Notifications will be unaware of the rollback and therefore continue to display the unrollbacked (i.e. dirty) state of the objects.

We propose therefore to send a normal EMF Notification for each change that is made to each of the CDOObjects that are being restored from DIRTY to CLEAN during a rollback, so that, as far as any adapters are concerned, the state change of the object is communicated to them normally. That is, from the point of view of the adapters, there will be no difference between a modification of a CDOObject due to a rollback, and a "normal" modification. If it seems necessary to allow adapters to distinguish between normal changes and rollback changes, then perhaps the Notifications can carry some state to provide this extra info.
Comment 1 Simon Mc Duff CLA 2009-07-10 07:53:49 EDT
SHould it receives one notification per feature ? or one for everything ?

Like the changerecorder will behave (one per feature)

Because if it not.... should we not just listen on CDOTransaction for rollback ?


Simon
Comment 2 Simon Mc Duff CLA 2009-07-10 07:56:32 EDT
Another comments...
EMFNotification are create for remote changes... In your case it is not. SO I'm wondering what will be the best design to add that.

Should we add another options.. to notify object on rollback ? 
- Complete (default) (One per features)
- Compact (One per object)
- None NONE
Simon
Comment 3 Caspar D. CLA 2009-07-10 11:15:31 EDT
(In reply to comment #2)

For the use cases I was thinking of, the "complete notifications" option makes most sense. But indeed, some types of adapters are probably better served through "compact" notifications, or might not even want rollback notifications at all. So I agree, making this configurable is a good idea.
Comment 4 Eike Stepper CLA 2010-06-29 04:50:44 EDT
Rebasing all outstanding enhancements requests to version 4.0
Comment 5 Eike Stepper CLA 2011-01-27 04:34:54 EST
Created attachment 187715 [details]
Patch v1

Pascal offered to write a test...
Comment 6 Eike Stepper CLA 2011-01-27 04:37:01 EST
Committed revision 6945
Comment 7 Eike Stepper CLA 2011-01-27 10:27:31 EST
Committed revision 6948
Comment 8 Pascal Lehmann CLA 2011-01-27 11:28:33 EST
Created attachment 187745 [details]
Testcase v1

I added a couple of different test. If I missed an important one please let me know and I can add it :)

Also, when writing the tests, I became aware that the rollbacked object stay in PROXY state until accessed again. The notification is not sent until the object is reloaded, so it would be nice to have the option of the objects being reloaded automatically on rollback.
Comment 9 Eike Stepper CLA 2011-01-27 11:39:52 EST
I changed rollbackRevision.get() ro remove() in the LoadTransition.

Committed revision 6949
Comment 10 Eike Stepper CLA 2011-01-27 11:42:27 EST
Created attachment 187747 [details]
Testcase v2 - ready to be committed

Just reformatted.
Comment 11 Eike Stepper CLA 2011-01-27 11:45:58 EST
(In reply to comment #8)
> Also, when writing the tests, I became aware that the rollbacked object stay in
> PROXY state until accessed again. The notification is not sent until the object
> is reloaded, so it would be nice to have the option of the objects being
> reloaded automatically on rollback.

I wonder if we should better expose rollbackObjects.keySet() in the CDOTransactionFinishedEvent. Maybe we should separate two event types for Committed and RolledBack...
Comment 12 Eike Stepper CLA 2011-01-27 11:58:11 EST
The new tests all seem to fail in LEGACY. Are there any equals checks that do not consider legacy adapters? Investigating...
Comment 13 Erwin Betschart CLA 2011-01-28 04:51:22 EST
I think it would be a good idea to emit EMF notfications in case of a transaction rollback. 
Maybe a new option should be introduced which enables this feature. 


I'll attach a patch...
Comment 14 Erwin Betschart CLA 2011-01-28 04:52:52 EST
Created attachment 187818 [details]
Enables EMF notifications on transaction.rollback()

I confirm to
1) The number of lines that you changed is smaller than 250.
2) You are the only author of these changed lines.
3) You apply the EPL to these changed lines.
Comment 15 Martin Fluegge CLA 2011-01-28 05:21:46 EST
The CDOLegacyWrapper had to be optimized to work with the new changes. The cdoInternalPostRollback() method now behaves more comparable to the native call. Changes in the LoadTransition are now also availbale in the legacy mode.

Committed revision 6955
Comment 16 Martin Fluegge CLA 2011-01-28 05:24:33 EST
Created attachment 187823 [details]
Testcase v3 - ready to be committed

I changed the test case a bit to make it work with legacy. All test a now passing. 

Testcase v3 - ready to be committed
Comment 17 Erwin Betschart CLA 2011-01-28 10:42:09 EST
Created attachment 187850 [details]
Enables EMF notifications on transaction.rollback()

Performance improvments after discussion with Eike...
Now fetching all revisions in one call.


I confirm to
1) The number of lines that you changed is smaller than 250.
2) You are the only author of these changed lines.
3) You apply the EPL to these changed lines.
Comment 18 Eike Stepper CLA 2011-01-29 01:46:45 EST
Committed revision 6959 (Testcase v3)
Comment 19 Eike Stepper CLA 2011-01-29 07:28:29 EST
Committed revision 6964
Comment 20 Martin Fluegge CLA 2011-01-30 12:17:21 EST
Committed revision 6980:
- trunk/plugins/org.eclipse.emf.cdo
Comment 21 Eike Stepper CLA 2011-01-30 12:19:46 EST
Committed revision 6980
Comment 22 Eike Stepper CLA 2011-01-30 12:24:33 EST
Committed revision 6981:
- trunk/plugins/org.eclipse.emf.cdo
Comment 23 Eike Stepper CLA 2011-06-23 03:41:20 EDT
Available in R20110608-1407