Community
Participate
Working Groups
Papyrus shall enable to configure a model in read-only mode in order to be able exchange model between users only for reading purpose.
This task can be decomposed as follow: - Runtime: Enforce the read-only state in the EditingDomain. We may throw a RollbackException when a change is detected on a read-only element. - Runtime: Enforce the read-only state in GEF/GMF requests. We may add an advice to return Unexecutable Commands for Requests based on a Read-only element - UI: Change the UI to reflect the Read Only state, e.g. gray out read only elements, prevent edition of read-only diagram elements, ... This is already partially implemented (Especially in the properties view, where read-only property editors are disabled). Most diagram requests bypass the read-only state. A few ModelExplorer commands also bypass it. - UI: Improve the support for the "Make Writable" action. Especially, libraries should be read-only by default, but it should be possible to make workspace libraries writable.
Re-assigning to myself per off-line discussion.
Some initial investigation/experimentation finds that many use cases in the diagram editors, Model Explorer, and Properties view are already handled by delegation to the Element Edit Service Provider to acquire commands from the GMF edit-helpers. The provider checks whether the element to be edited is read-only and if it is, serves up the NullElementType that doesn't provide any edit commands. However, there are use cases that expose loop-holes. Use Case: Opposite References Read-only elements may be modified when a writable element adds a reference to the read-only element via an EReference that has an opposite. For example, the Dependency::client and NamedElement::clientDependency references are opposites. 1. Create a model that imports the UML Primitive Types library, with a class diagram. 2. Create classes A and B in the class diagram. 3. Drag the type UMLPrimitiveTypes::String onto the class diagram. 4. Draw a Dependency from A to B. This dependency is created in the writable package and is referenced as a clientDependency of the writable class A. 5. Now draw a "dependency branch" from the String type to the dependency. This updates the dependency to add String as a client, which in turns adds the dependency to String's clientDependency collection. The problem is that the hand-crafted code of the MultiDependencyHelper (which is called by the CustomGraphicalNodeEditPolicy, which in turn is called by GMF-generated diagram code) doesn't delegate creation of model-update commands to the Element Edit Service: it simply creates a SetValueCommand. I suppose this will be fairly common in the code (custom or generated) behind many of the diagram palette tools. Consequently, there are no checks for read-only elements. And, even if there were, there would be no accounting for opposite references. And an unexecutable-command advice would have no opportunity to contribute without involvement of the edit helpers.
OK, so I have a back-stop in place on a local branch: the editing domain detects attempts to modify read-only objects/resources and cancels the transaction. A listener in the UI layer detects the consequent roll-back and presents a dialog indicating the reason. This covers the most obscure cases, such as bi-directional references, that are difficult to detect up-front by analyzing a command before it executes. The next step is to improve the a priori handling of read-only objects so that we don't attempt to execute commands that will just roll back and confuse the user. There are two primary mechanisms to consider: an advice that renders the edit operation unexecutable if it would modify read-only objects, and an operation approver that rejects the execution of an operation that would modify read-only objects. We already have the latter: an approver that detects attempts to modify contents of read-only IFiles in the workspace, calling out to a service (with potential team-provider hooks) to make those IFiles writable, else disallow the operation execution. This is the most appropriate solution for workspace model files because it gives the user a chance to be proactive and rescue the operation. There is another kind of read-only resource, however, which the user cannot and should not make writable: resources deployed in plug-ins. For these, I think it is appropriate to simply block execution of an operation as soon as possible. I propose that an edit-helper advice can return an unexecutable command in these cases only, where an edit would modify an object in a plug-in resource, so that the diagram editor may show no-smoking cursors and other affordances to let the user know that an operation will not even be attempted. This advice must not block operations touching read-only IFiles in the workspace because we need the operation approver to have the chance to make them writable.
I have pushed a new branch bugs/323802-readonly with some progress in five commits. Commit 4d9b5f1: Enhances the PapyrusROTransactionalEditingDomain to catch model changes that somehow slip through other a priori checks, which modify objects that are (still) read-only. When a change to a read-only object is detected, the transaction is rolled back. An operation-history listener is registered on a new extension point, which attaches registered listeners to the diagram editor's operation history. It analyzes operation execution failures to determine whether roll-back was the cause, and presents a dialog explaining the cause to the user. Commit 8ef9f17: JUnit tests for the editing domain changes. Commit 22e0c88: Adds an universal edit-helper advice (bound to the shared Papyrus client-context) that rejects edit-commands that would modify a read-only object that the operation approver would not be able to make writable (in practice, these are plug-in deployed resources). Commit da93312: JUnit tests for the edit-helper advice. Commit 03d8c7d: Integrates the new JUnit tests into the build. Also updates older tests to avoid transaction roll-backs on read-only (plug-in) resources.
Some commits aiming to improve user feed-back under normal use cases, including the prompt to make read-only resource writable and disabling edit operations a priori that would modify resources that cannot be made writable (such as no-smoking-sign cursors in the diagrams for palette tools). Commit 9f0e323: Merely disapproving the edit in an advice is not enough because in the diagram editor, the GEF EditPolicy can produce a command that is used instead of the "semantic" command produced (and null if disapproved) by the edit-helper. So, we must actually return an unexecutable command in order to block the execution of this fall-back command. It's less efficient, but necessary. Commit 4ceef01: Binds the read-only advice to the GMF element-type/edit-helper client-contexts for the various UML diagrams. With this, using the Property tool in the Class Diagram palette to drop a new property on some class or data type in the diagram will: * if the drop target is a class/datatype imported from a read-only resource in the workspace, the drop will be allowed and the operation approver will prompt to make the read-only model files writable * if the drop target is a class/datatype imported from a resource deployed in a plug-in, the drop will not be allowed (the cursor will show the no-smoking sign) Commit 7ffa96c: Fixes a problem in which the listener that presents the rollback error dialog (from commit 4d9b5f1) would not appear when edits in the Advanced tab of the Properties View are rolled back. The problem was that the operation history listener in this case received a "not OK" event without a status, leading to a NullPointerException on the attempt to extract a RollbackStatus from it. This happens when the operation execution that fails is the closing of a TriggeredOperations composite. Happily, we can access the triggering operation and (if it's a GMF ICommand) get a status out of its CommandResult. However, the actual RollbackStatus that we're looking for is buried in a RollbackException in some nested status, so the mining algorithm to find the RollbackStatus is updated to handle this case. Commit b19c2fe: Add support to the read-only handler framework for an optional protocol by which clients can ask, for a read-only object/resource, whether it might be possible to make it writable. This will generalize the determination in the read-only edit-helper advice of whether to allow a command to proceed because we can be confident that the operation approver will be able to pop up a dialog requesting to make the affected resources writable. Commit dc62f60: Let unprotected transactions make changes in read-only resources (useful or required for testing, but makes sense anyways). Commit 3e6b714: Update the read-only advice to delegate to the read-only handler framework to determine whether it would be possible to make a read-only resource writable (as enabled by commit b19c2fe). Consequently, update read-only advice tests to use our actual Papyrus editing domain (with its read-only handler) and to remove the local file cases because the read-only handler framework doesn't support them. Commit 3423647: Support editing of read-only resources (with the approver's prompt to make resources writable) in the Model Explorer and other contexts than diagrams. This involves being more aggressive in trying to extract the "affected files" from IUndoableOperations in the approver, because drag-and-drop operations (for example) in the Model Explorer are not GMF commands, but rather include GMF commands nested within wrappers and composites. So, the result of all this is that now it is possible in the Model Explorer to, for example, * drag a property from some class to a class in a package imported from a read-only resource. The prompt to make that resource writable will be shown * try to drag a property from some class to a class in a package imported from a resource (a library) deployed in a plug-in. The drop feedback will show that the drop is not allowed The controls in the Properties View for objects in read-only resources are still disabled because the objects are not (yet) writable. Likewise, read-only objects are still shown as disabled (greyed/ghosted) in the Model Explorer until they are actually made writable. I think this makes sense, because we still want the visual cue that the resource is read-only (probably for a good reason). And the prompt to make it writable, for example on drag-and-drop, can be cancelled to leave the status quo. Two bullet items remain from comment 1: > This task can be decomposed as follow: > > - UI: Change the UI to reflect the Read Only state, e.g. gray out read only > elements, prevent edition of read-only diagram elements, ... This is already > partially implemented (Especially in the properties view, where read-only > property editors are disabled). Most diagram requests bypass the read-only > state. A few ModelExplorer commands also bypass it. Are views (in the diagrams) of read-only elements supposed to have some visual decoration? My testing finds that a lot of the palette tools create commands that correctly include the ReadOnlyObjectEditAdvice. Which commands or family of commands are we thinking of that do not use the GMF edit-helpers? > - UI: Improve the support for the "Make Writable" action. Especially, libraries > should be read-only by default, but it should be possible to make workspace > libraries writable. What constitutes a "library" in the workspace? Is it simply a Model that has the <<modelLibrary>> stereotype applied? Why would we override the workspace/filesystem notion of read-only state? And how would we remember that the user wants a library to be writable? I don't think we should be adding persistent resource properties in the workspace metadata for that. Note that the current state of this branch ensures that the user will never have an opportunity to make changes to objects in libraries that are deployed in plug-ins.
> Are views (in the diagrams) of read-only elements supposed to have some visual decoration? No, although I will probably add a CSS selector for read-only elements (So that users can choose to display them differently) > My testing finds that a lot of the palette tools create commands that correctly include the ReadOnlyObjectEditAdvice. Which commands or family of commands are we thinking of that do not use the GMF edit-helpers? I think the Edit helper should be sufficient. Until it was implemented, all GMF commands used to ignore the read-only state. In the properties view, some specific widgets are not disabled for read-only elements (For example, the specific widgets used in the Profile tab). Note that I haven't checked the read-only branch so far, so you may have fixed that already.
(In reply to comment #7) > > I think the Edit helper should be sufficient. Until it was implemented, all GMF > commands used to ignore the read-only state. > > In the properties view, some specific widgets are not disabled for read-only > elements (For example, the specific widgets used in the Profile tab). Note that > I haven't checked the read-only branch so far, so you may have fixed that > already. Thanks, Camille. I haven't yet worked on anything specific in the Properties and Model Explorer views; that's up next. There are actions in the explorer, for example, that aren't consistently disabled on read-only elements.
I have pushed to branch bugs/323802-readonly several new commits to address disablement of Properties View widgets on read-only model elements and diagram views. I think this pretty much completes the Properties View read-only support (further ad hoc testing required to bear this out). Next up is some read-only checking in the Model Explorer context menu actions. Commit 1ec48a6: Update various custom widgets to account for read-only state, including Applied Comments, Association End, Extension Is-required, Message Signature, Profile Definitions, Profile and Stereotype Applications, Stereotype Appearance. Commit 6e90894: Make the gradient style section in the Appearance tab respect read-only state of the notation model. This takes advantage of the fact that the ObservableGradientData extends the GMF GradientData class with the necessary context of the notation style object that owns the gradient data. Commit fc47d25: Handle read-only state of the notation view in the Shape Decoration section of the Appearance tab. Commit aa39e09: Make presentation of the Reset Style button in the Appearance properties sensitive to the read-only state of the notation view. This requires injecting the CSS property into the widget from the XWT model so that the input may be consulted for editability. Commit e99a633: Make the presentation of the Applied Stereotypes display properties consider the read-only state of the notation view (the buttons need to be disabled when read-only).
(In reply to comment #6) > > > - UI: Improve the support for the "Make Writable" action. Especially, > libraries > > should be read-only by default, but it should be possible to make workspace > > libraries writable. > > What constitutes a "library" in the workspace? Is it simply a Model that has > the <<modelLibrary>> stereotype applied? Why would we override the > workspace/filesystem notion of read-only state? And how would we remember that > the user wants a library to be writable? I don't think we should be adding > persistent resource properties in the workspace metadata for that. Note that > the current state of this branch ensures that the user will never have an > opportunity to make changes to objects in libraries that are deployed in > plug-ins. From off-line discussion, I think I understand that the intention here is to treat model elements referenced by, but not owned by, the principal model in the editor (and Model Explorer) as read-only by default. The purpose is to help users to avoid editing conflicts in referenced models that are also open in other editors, whether as the principal model in another editor or as a referenced model. Thus, the "library model" is an ad hoc library by virtue of reference. And, once support for opening resources in the Model Explorer without explicitly importing packages (which changes the importing namespace) is committed, these library dependencies may actually not be expressed by PackageImports. It is still TBD whether the decision to allow editing a referenced model should be (optionally) persisted. In that eventuality, it is anticipated that this persistence would be associated with the referencing model (not the library) either in workspace metadata or a preference store (perhaps after the fashion of ruler and grid settings).
I have pushed a new commit to the branch that implements default read-only mode for referenced models (ad hoc "library" models). Commit edd3306: Make the contents of referenced model resources read-only by default, via a new read-only handler. A referenced model resource is one that is not a constituent of the "main" model (being edited in the current ModelSet). This includes controlled sub-model units. To determine efficiently whether a resource is a sub-unit of the main model, we add an adapter that tracks the topology of the sub-unit structure by detecting cross-resource-containment changes and caching the child-unit/parent-unit relationships. The new read-only handler makes resources writable (with user prompt) by remembering their URIs locally. This memory is not retained if the model editor is closed and then opened again.
Further commits to round out the default read-only state of referenced resources (which seems to work nicely with the new Model Explorer content showing additional resources). Commit 76b23bc: Controlling and uncontrolling resources with the CDO Integration plug-ins installed was tripping up on the new transactional editing domain check for modifications to read-only resources. This is now fixed. Commit 43f93a8: JUnit tests for the read-only handler for referenced "library" models. Includes coverage controlled units of models (to ensure that they are not mistaken for libraries).
This branch is now ready for code review. Commit aa0380b: Enable the import-package context menu in the Model Explorer only on packages that are already or can be made writable. This involves a few changes: * add a new canMakeWritable property tester to the existing isReadOnly * add a visibility filter to the import-pacakge menu declaration using this new tester * change the commands executed by the import-package handlers from EMF Commands to GMF ICommands so that they may present the affected files to the operation approver that enables write on-the-fly for resources that are read-only but can be made writable
When you drag & drop a read-only root element (e.g. UML Primitive Types model) into another Element, there is a strange result. The Model is logically moved, but remains in its resource. Only its "eContainer" is changed, which results in the Model being considered "Controlled". As this operation doesn't modify the UML Primitive Types model itself, it is not forbidden by the Read Only framework. Side effect: because the Model Explorer doesn't detect the event (There is no change in the Resource's root elements), it is not refreshed, and the Dropped model still appears at the root of the model explorer. On the following refresh (e.g. load resource), it will disappear. (Note that this side effect is related to the way the Model Explorer detects event; it's not a read-only issue). I just realized that this issue in general is not related to the read-only framework. Just a surprising result of dropping root elements in targets located in different resources... But I've been able to crash the model when playing with this "unexpected feature". This method enters in an infinite loop: org.eclipse.papyrus.infra.emf.readonly.ControlledResourceTracker.getRootResourceURI(URI) Steps to reproduce: - Create a model with more than one UML root [This cannot be done in Papyrus, but there are tools which extend Papyrus and can do that; Bug 413061] - Drop one root into the other :) - Save and restart The resource still has two roots, but one of them is logically contained (controlled) into the other. In general, we should prevent the user from dropping a root element into another element. Or, if we allow it, remove the dropped element from its former resource (When this is not forbidden by the "read-only" state) Otherwise, everything looks good.
(In reply to comment #14) > When you drag & drop a read-only root element (e.g. UML Primitive Types model) > into another Element, there is a strange result. Good catch! I'll have a look into this. I think that drag-and-drop of root packages should simply be disabled across the board. It feels like a nonsensical refactoring, as it would actually empty the resource. Alternatively, such a refactoring operation could be completed by actually deleting the originating resource (and its adherents). But, I think that workflow might be surprising. In any case, a phased approach would be appropriate, which starts by disabling the drag-and-drop of root packages anyways until the complete implementation can be achieved.
Actually, I think that all D&D operations from one resource to another are dangerous and not properly handled. There are many non-containment relashionships between models, which are based on the resource: Stereotype applications, Diagrams... When moving an Element from a resource to another one, this should be implemented as a "Copy, Paste, Delete" (Or Cut/Paste), which handles all the side effects (Or will, soon). This is still not related to the read-only mode, however. Remi, do you think this will be handled by some other tasks currently in progress? Currently, the stereotype remains in the former resource (Or even disappears, if the target is a Package on which the profile is not applied).
(In reply to comment #16) > > When moving an Element from a resource to another one, this should be > implemented as a "Copy, Paste, Delete" (Or Cut/Paste), which handles all the > side effects (Or will, soon). Never copy semantics, because that would not preserve incoming references to the elements that the user is (logically) moving. > This is still not related to the read-only mode, however. Right, but in any case, if the element being dragged is read-only, the operation should not be permitted. This is a hole in the ReadOnlyObjectEditAdvice IMO. Once I have fixed that, will we be good to merge?
> Never copy semantics, because that would not preserve incoming references to the elements that the user is (logically) moving. Indeed; I've been too fast. "Drag & Drop" is slightly different from "Cut / Paste" and "Copy / Paste / Delete". But there's still something to be done, here, which is quite similar to what's currently being done in the Copy/Cut/Paste enhancement.
> Once I have fixed that, will we be good to merge? Yes, I think that's fine. Thanks!
I've started to work on Bug 429239, which I've based on the read-only branch. In Papyrus diagrams, we use transient compartments for Symbols and Stereotypes. When we load a diagram from a read-only resource, and we try to create these compartments, this fails: !ENTRY org.eclipse.papyrus.infra.gmfdiag.common 4 0 2014-02-27 17:14:26.499 !MESSAGE Unexpected Error !STACK 0 org.eclipse.emf.transaction.RollbackException: Attempt to modify object(s) in a read-only model. at org.eclipse.emf.transaction.impl.TransactionImpl.commit(TransactionImpl.java:405) at org.eclipse.papyrus.infra.gmfdiag.common.editpolicies.ShapeCompartmentEditPolicy$1.run(ShapeCompartmentEditPolicy.java:175) at org.eclipse.swt.widgets.Synchronizer.syncExec(Synchronizer.java:185) at org.eclipse.ui.internal.UISynchronizer.syncExec(UISynchronizer.java:150) at org.eclipse.swt.widgets.Display.syncExec(Display.java:4712) at org.eclipse.papyrus.infra.gmfdiag.common.editpolicies.ShapeCompartmentEditPolicy.executeShapeCompartmentCreation(ShapeCompartmentEditPolicy.java:130) at org.eclipse.papyrus.infra.gmfdiag.common.editpolicies.ShapeCompartmentEditPolicy.createShapeCompartment(ShapeCompartmentEditPolicy.java:236) at org.eclipse.papyrus.infra.gmfdiag.common.editpolicies.ShapeCompartmentEditPolicy.activate(ShapeCompartmentEditPolicy.java:73) Basically, we detect a read-only transaction (The diagram initialization), we suspend it and switch to a read-write transaction, to add the transient elements. Then we commit this transaction and re-activate the read-only transaction. Except that, we're not in an "unprotected" transaction (Which would be allowed, according to Comment 6) I don't really like this part of the code (Creation of transient compartments on-the-fly depending on applied stereotypes and installed components), so maybe there is a better way to achieve that. Otherwise, should we start these transactions in unprotected mode? Or add a specific case in the Read-only transactions framework, to allow creation and modification of transient references and elements? (I'm not really familiar with these transient/volatile elements and what's implied here).
Commit b2674f3: Fixes gaps in the read-only validating edit-command advice: * disallow move of root element into some writable container if the originating resource is read-only (the moved element did not have a container) * disallow setting a containment reference (including adding to or replacing a containment list) if the current container of any new element is different from the reference owner and is read-only (including the case where an object was previously a root of a read-only resource)
(In reply to comment #20) This is exactly the reason why the "unprotected" transaction option exists: to start a write transaction as a child of a read transaction. The original use case for this mechanism was the updating of canonical views in GMF diagrams. So, I think it's a good fit for our situation, here. If I can reproduce the symptom, I can have a look at fixing the problem on this branch before I merge to master (glad I haven't done that, yet).
The symptoms can (I think) be reproduced by loading manually the notation resource into the current model. The diagram should then appear in the ModelExplorer, and you can open it.
Final commits (as described below) are pushed and merged to master. Commit 126ae8b: Updates several canonical edit policies to use a new GMFUnsafe utility class to perform notation changes in unprotected write transactions. These have the advantage of being compatible with an otherwise read-only context (such as read transaction for diagram refresh) and read-only diagrams (unprotected writes are permitted in read-only resources). Several occurrences of scary-looking anti-patterns are fixed by this, including: * invocations of Display.getCurrent().syncExec(...): if we're getting the current thread's Display, we're already on the UI thread, so there's no need to syncExec. Perhaps an asyncExec(...) was actually intended in these cases? * invocations of TransactionalEditingDomain.runExclusive() that do nothing except a Display.asyncExec(...). Because it's asynchronous, the read-only transaction is effectively useless (the async runnable will happen later or on a different thread, not in the read transaction) * use of the InternalTransactionalEditingDomain API to start a transaction, execute a command, then commit it *without* a finally block to ensure that the transaction is committed. Any exception in the command would result in the transaction staying active forever and no transaction ever being able to run again in that editing domain I recommend any further instances of canonical edit policies that need to make unprotected notation changes to follow the examples in this commit, using the GMFUnsafe utilities. Commit 81ab167: JUnit tests for the GMFUnsafe class.
