Community
Participate
Working Groups
A few different renderers use the ConditionalDeleteService to test whether an object may be deleted from a reference, including the MultiReferenceSWTRenderer and the TableControlSWTRenderer. The problem is that these references are not always deletions: in the case of cross-reference, the object will not be deleted but merely removed from the reference. So checking whether a DeleteCommand can be executed is too strict, as the object may not be deletable (e.g., it may be in a read-only resource) but the reference may be editable. The ConditionalDeleteService should be expanded to more generally test the removability of an object from a reference, with the delete case for containments only.
New Gerrit change created: https://git.eclipse.org/r/156021
Gerrit change https://git.eclipse.org/r/156021 was merged to [develop]. Commit: http://git.eclipse.org/c/emfclient/org.eclipse.emf.ecp.core.git/commit/?id=a78f15206e41df1e85ef2118f42e6c7c38c1ba41
(In reply to Eclipse Genie from comment #2) > Gerrit change https://git.eclipse.org/r/156021 was merged to [develop].
Verified in the 1.24 M2 build.
should this be marked with the test tag, as it changes the behavior?
(In reply to Jonas Helming from comment #5) > should this be marked with the test tag, as it changes the behavior? I don't think so. For containment references, the behaviour is unchanged. For cross-references, the erroneous behaviour that disabled the delete button when it should not be disabled is fixed. The only adverse impact would be on applications that were (for whatever reason) relying on the erroneous disablement of the delete button for cross-references to read-only objects; it would have been unwise of applications to rely on that :-)
I actually agree to everything you said except that you actually provided the reason to mark this with "test" and even already kind of the test description. You would be suprised how often applications rely on "erroneous behavior". It is not that they really rely on it, but for whatever reason the bug results in the behavior they expect. Now when such a bug gets fixed, even if the previous behavior was wrong, it results in a change and any change should be at least know, even better tested.
(In reply to Jonas Helming from comment #7) > I actually agree to everything you said except that you actually provided > the reason to mark this with "test" and even already kind of the test > description. > > You would be suprised how often applications rely on "erroneous behavior". No, I'm not surprised. I've seen it before. But an application depending on this erroneous behaviour would be manifestly broken and hostile to its users, so I wouldn't have much sympathy for it. ;-)
## TESTING INFORMATION ### Summary of the critical part of the change The ConditionalDeleteService now tests the validity of removing an object from a cross-reference using a RemoveCommand, not a DeleteCommand. The DeleteCommand is still used for containment references as removal from containment is logically deletion. ### Potential regressions None. Delete semantics simply are not applicable to cross-references. ### Affected areas / use cases Enablement of the Delete/Remove buttons in - link renderer - multi-reference renderer - table renderer - tree master-detail renderer - tree master-detail composite (not the renderer) ### Things that shall be tested Applications that present cross-references in form views where the referenced objects may not be deletable (because, for example, they are in read-only "library" resources) but where its should be permitted to un-reference those objects.
thanks!
