Bug 206811

Comment 1 Christian Damus 2007-11-12 11:08:20 EST

Created attachment 82668 [details]
Proposed changes for J2SE 5.0

Attached a patch with J2SE 5.0-ification of the org.eclipse.emf.transaction* and org.eclipse.emf.workspace* plug-ins.

The only interesting opportunities for adding a generic signatures to existing types and methods are:
  - org.eclipse.emf.transaction.RunnableWithResult:  parameterize the result value type.
    This prompted the addition of new generic TransactionUtil methods for executing these
    runnables as the TransactionalEditingDomain methods runExclusive(Runnable) and
    createPrivilegedRunnable(Runnable) cannot be made generic owing to the void Runnable
  - TransactionUtil::getAdapter(...) is generic, with the return type covariant with the adapter
    class parameter

Comment 2 Christian Damus 2007-11-12 11:49:26 EST

Created attachment 82673 [details]
Updated TriggerCommand patch

Attached a patch to update the TriggerCommand to make it compatible with current usage in GMF's custom editing domain implementation.  This aligns the constructor signature with the usual semantics of copying the source list, hence allowing the List<? extends Command> signature that the base CompoundCommand does not employ because it does not copy the list.

This patch replaces the TriggerCommand changes only from the first patch.

Comment 3 Christian Damus 2007-11-12 11:50:14 EST

Ed, Anthony,

I would greatly appreciate one or both of you reviewing the proposed changes that I have attached.  Let me know if you see that I've missed anything.

Comment 4 Anthony Hunter 2007-11-13 19:28:16 EST

Hi Christian, only saw two things. Otherwise looks good to me.

TransactionUtil.runExclusive - I am not sure I understand why we needed this.

Why to we always use Map<?, ?> for options, when is an option not String, boolean?

Comment 5 Christian Damus 2007-11-14 07:39:45 EST

Thanks, Anthony, for looking this over!  To answer your questions:

The "<T> TransactionUtil::runExclusive(RunnableWithResult<T>)" method is defined to provide a type-safe API for executing the newly generic RunnableWithResult<T>.  The existing "Object TransactionalEditingDomain::runExclusive(Runnable)" method cannot be changed to be generic on the RunnableWithResult signature because the parameter is Runnable and must remain, so it can only continue to return Object.  It didn't seem worth adding an extension interface for the domain to implement another runExclusive.

Regarding the options:  <?, ?> is used for two reasons.  Firstly, because this is the signature used throughout the core EMF APIs (load/save options for Resources, in particular) and secondly, because there is actually an option that accepts a non-Boolean value.  The Transaction::OPTION_VALIDATE_EDIT accepts either a Boolean or an instance of ValidateEditSupport (several EMF load/save options work similarly).  It also provides more flexibility for implementors of the editing domain API.  Unfortunately, since we already have String-keyed options, I cannot now introduce a more OO design such as I did in MDT OCL with the Option<T> that declares the value type and an intrinsic default value.  One of Ed's comments (off-line) was that, since the accessors (getOptions etc.) for these options maps generally provide read-only views, they should likewise be typed as Map<?, ?> instead of Map<Object, Object>.

Does that address your concerns?  I am amenable, of course, to making further changes.

Comment 6 Christian Damus 2007-11-14 08:16:51 EST

Created attachment 82864 [details]
Updates from review comments

Attached a new patch with changes addressing review comments (thanks Ed and Anthony).

  - use Map<?, ?> where possible as return type for methods
    returning read-only views of transaction options, instead of
    Map<Object, Object>

  - use Boolean.valueOf(String) instead of
    Boolean.TRUE.toString().equalsIgnoreCase(String)
    in Tracing::shouldTrace(String) methods

  - in ResourceUndoContext::handleCrossResourceReference(...),
    updated ADD_MANY and REMOVE_MANY cases to cast newValue/oldValue
    to Collection<EObject> respectively to simplify the loops

Comment 7 Anthony Hunter 2007-11-14 09:38:43 EST

(In reply to comment #5)
> Does that address your concerns?  I am amenable, of course, to making further
> changes.

Thanks for the answers, I approve of your changes. 

Comment 8 Christian Damus 2007-11-14 13:16:50 EST

Committed the patch to CVS with the following additional changes:

  - replace insertion of primitive boolean "true" and "false" values into maps with
    Boolean.TRUE and Boolean.FALSE, respectively.  This applies to Map.put(...) and
    Collections.singletonMap(...) calls.  For some reason, javac autoboxes literal
    booleans using Boolean.valueOf(boolean) just as for other boolean-value expressions
  - "Code Clean-up" action to add missing @Override and @Deprecated annotations
  - recomputed feature.xml plug-in dependencies

Comment 9 Christian Damus 2007-11-14 15:32:27 EST

Fixed in the EMF TRANSACTION 1.2.0 I200711141501 build.

Comment 10 Nick Boldt 2008-01-28 16:35:50 EST

Move to verified as per bug 206558.