Community
Participate
Working Groups
From bug 429239 comment 11: -------- 8< -------- Even if we had a FSReadOnlyHandler which properly detects read-only external files, and can make them writable, there is still a problem of conflicting ReadOnlyHandlers: the read only handlers are mutually exclusive. In the case of the Sash file in the .metadata/ folder, the FSReadOnlyHandler is not even called, because SashModelReadOnlyHandler returns "ReadOnly = false" (And has a higher priority). So, we can't even try to make this file writable, because it is already considered writable. Basically, we have (at least) 3 read-only handlers which pretend to give a read-only state for the SashModel: - Priority 5: FSReadOnlyHandler: "Physical" state: Read only = true - Priority 15: ReferencedModelReadOnlyHandler: "Logical" state. Read only = true (Considered as an external library because the sash URI is different from the ModelSet's URI) - Priority 20: SashModelROHandler: "Logical" state. Read only = false (Because we know it's not an external library) Moreover, the SashModelROHandler should not test the file-system state of the *.sash file, because it could be stored in a CDO resource. It really needs to check its "logical" Read-Only state, and the "physical" read-only state should be checked by either FSReadOnlyHandler or CDOReadOnlyHandler. And we can't make a "AND" of all ReadOnlyHandlers either, because ReferenceModelReadOnlyHandler would always return "ReadOnly = true". So I really think we need to distinguish between the physical read-only state (Based on the underlying filesystem or database) and the logical one (Based on the resource's contents and role). And if we want to be able to read models for which the *.di file is read-only on the FileSystem (and can't be changed for whatever permission reason), we need to be able to make resources "logically writable" and skip the save action (Especially important for the case of the read-only sash model: even if we know that we won't be able to save it, we may still want to open and close diagrams). A first action might be to isolate the save errors (Currently, there is a single try/catch around the global ModelSet save action) and properly report these to the user. -------- >8 --------
I have pushed a new branch bugs/429826-ro-axes, based on the latest master, with two commits implementing a new API for differentiating the orthogonal dimensions (I call them "axes") of read-only concerns. Commit b97caff: Introduces a ReadOnlyAxis enumeration in the core infra package alongside the IReadOnlyHandler API. Currently, it defines two axes: PERMISSION (the "physical" dimension per comment 0) and DISCRETION (the "logical" dimension per comment 0). The IReadOnlyHandler2 interface is enhanced to accept, in each of the isReadOnly/canMakeWritable/makeWritable queries, a set of axes on which the client wants the read-only condition to be tested. Corresponding API overloads are added to the EMFHelper utility. For backward compatibility, implementors of the IReadOnlyHandler interface that do not implement IReadOnlyHandler2 are considered to have an affinity with the PERMISSION axis, as in past releases of Papyrus this was the only read-only concern that we dealt with. The EMFHelper::isReadOnly(...) APIs that don't include the Set<ReadOnlyAxis> parameter delegate with the set of all axes, so that the result is a union of all possible read-only concerns. This is useful in the UI for disabling edit function as previously. However, clients that need more fine-grained control (such as for managing the sash model) now have a mechanism for that. All of the currently defined read-only handlers are updated to implement affinity with one or the other of the PERMISSION and DISCRETION axes, as seemed reasonable to me. Somebody with more familiarity with some of the corner cases involved should review these changes. Commit f08cbd7: Updates the PapyrusROTransactionalEditingDomain's backstop checking for modifications of read-only resources for an awareness of the new read-only axes. A new transaction option lets a client indicate the axes on which read-only state should be tested; this may be used, for example, by the transactions that update the sash model to assert only the DISCRETION axis, not PERMISSION, to avoid the problem of not being able to update the *.sash resource if its is not writable in the filesystem (as in this case we would just not try to save it). Also new is a transaction option indicating whether the transaction is part of an user-interactive operation. Initially this allows automated tests to avoid popping up the prompt dialog for making resources writable, but it could be useful for manipulation of the sash model, too. The TransactionHelper class adds convenience API for working with these new options.
> All of the currently defined read-only handlers are updated to implement affinity with one or the other of the PERMISSION and DISCRETION axes, as seemed reasonable to me. Somebody with more familiarity with some of the corner cases involved should review these changes. This looks good for me, but I'm not really familiar with the Control Mode. However, shouldn't the "Make writable" action use the 2 axes? Because if we try to make a "logically read-only" resource writable, the action doesn't do anything, because the resource is considered writable already (According to the PERMISSION axis). Importing a workspace library (RO according to the DISCRETION axis, writable according to the PERMISSION axis) and making it writable should be possible.
(In reply to comment #2) > > However, shouldn't the "Make writable" action use the 2 axes? Because if we try > to make a "logically read-only" resource writable, the action doesn't do > anything, because the resource is considered writable already (According to the > PERMISSION axis). Indeed, the "Make Writable" action and other clients of the ReadOnlyManager are now updated in commit 82393c4 on branch bugs/429826-ro-axes. That commit also adds a listener protocol that the Model Explorer view uses to refresh its presentation of read-only content when read-only state changes occur (as implemented by read-only handlers). > Importing a workspace library (RO according to the DISCRETION axis, writable > according to the PERMISSION axis) and making it writable should be possible. This should work as expected, now, on the branch.
The "Import" menu is broken now. It is only available for "ReadOnly and potentially writable" resources, instead of "Writable and potentially writable" ones (Apparently, there's a "!" which has disappeared in the ReadOnlyTester). Now, regarding the initial use case, I guess the save action needs to be re-written, to account for the different axes. Currently, it relies on "AnyAxis", which returns false (Because the Discretion for SashModel is still false, and it has a higher priority than the Permission from FS). So, when we call isReadOnly(anyAxis), the result is "false" (Which is actually quite surprising). I'm not sure whether this is a bug in the RO Manager, or in one of the RO Handlers, or if we should reimplement the save action to test each axis separately.
I have pushed two new commits to the branch. Commit 5bb2853: Fixes the read-only tester to restore the enablement of the import actions (and probably others). Added JUnit tests for the ReadOnlyTester to catch future regressions. This includes a refactoring of some of the fixtures from other tests as a reusable JUnit rule that provides an editing domain and a workspace project containing a model loaded from a resource in the test plug-in. Check out the PapyrusROEditingDomainFixture and its superclass. Commit 1b5e1fc: In testing the previous commit, I noticed that I wasn't being prompted to make a referenced library model writable when adding a PackageImport to it via the Import Registered Package action. The problem was that the ReadOnlyOneFileApprover asks whether any of the three components (di/uml/notation) of the model is read-only, and the SashModelReadOnlyHandler returns a definitive negative because one of these is the DI resource. That's not correct because this is a statement that *all* of the resources are writable (which is the negation of *any* of the resources is read-only). So, this commit updates the SashModelReadOnlyHandler to answer a definitive negative result only when all of the resource URIs being queried are resources that it knows to be logically writable. This should still make the Save use case work because in that case, read-only state of resources is queried by the IModels on an individual basis. I shall, of course, look further into the save use case per the remark in comment 4.
(In reply to comment #4) > > Now, regarding the initial use case, I guess the save action needs to be > re-written, to account for the different axes. Currently, it relies on > "AnyAxis", which returns false (Because the Discretion for SashModel is still > false, and it has a higher priority than the Permission from FS). Save ultimately boils down to ModelSet::save(...), which checks only the permission axis. That seems reasonable to me. The actual save action is provided by the Eclipse Platform and delegates to saveable parts (which, in this case, delegates to the ModelSet). Papyrus doesn't implement its own save action, does it?
> Save ultimately boils down to ModelSet::save(...), which checks only the permission axis. That seems reasonable to me. Actually, the ModelSet#save only directly takes care of deleting resources (And making some resources writable). The actual save is performance by IModels (e.g. EMFLogicalModel#save), which checks for isReadOnly() (i.e. "anyAxis") Moreover, if the "Discretion" is considered read-only (e.g. Applied profiles, referenced models...), then we shouldn't try to save either. 1) Permission = false, Discretion = true: Do not save 2) Permission = false, Discretion = false: Do save 3) Permission = true, Discretion = true: Do not save 4) Permission = true, Discretion = false: Do not save The issue with "Permission" is doesn't take Discretion into account (Obviously). The issue with "AnyAxis" is that it takes the priority into account, and consequently, doesn't take into account the Permission axis, in the case of the *.sash and *.di files (Not sure whether your fix solved this case). During the save, we should probably test both separately, to have an accurate result. It may be useful to add "Discretion or Permission" and/or "Discretion and Permission", in addition to "Any Axis" (Which returns the first present boolean) Would this make sense?
I am able to close and open diagram editors in a read-only *.di resource, because the transaction's check for changes to read-only resources operates on one resource at a time, thus letting the SashModelReadOnlyHandler short-circuit the process with an Optional.of(false) result. However, it would still be nice to ensure that the page manager's transactions only check the discretion axis, so I'll work on that.
> The actual save is performance by IModels I meant "is performed by"
(In reply to comment #7) > > Save ultimately boils down to ModelSet::save(...), which checks only the > permission axis. That seems reasonable to me. > > Actually, the ModelSet#save only directly takes care of deleting resources (And > making some resources writable). The actual save is performance by IModels (e.g. > EMFLogicalModel#save), which checks for isReadOnly() (i.e. "anyAxis") Ah, right. > Moreover, if the "Discretion" is considered read-only (e.g. Applied profiles, > referenced models...), then we shouldn't try to save either. I'm not sure that I agree. The various other mechanisms (read-only edit advice, read-only operation approver, transaction read-only check) should have blocked any changes being made in these resources, so they shouldn't be modified and should therefore not need to be saved in the first place. *If* some component needed to override the read-only check and make changes to one of these resources, then we should save those changes if we can, no? > During the save, we should probably test both separately, to have an accurate > result. It may be useful to add "Discretion or Permission" and/or "Discretion > and Permission", in addition to "Any Axis" (Which returns the first present > boolean) > > Would this make sense? I need to think about it. Though a seemingly simple problem in concept, the details are too complex for my poor little brain. :-/ One possible ingredient of a solution that I shall ponder is separating the priority of the handler by axis, further cementing the notion that a handler has an affinity with a particular axis. But, this significantly changes the iteration logic of the ReadOnlyManager and I shall start to become concerned about the impact on performance. Not that user-initiated edit operations will ever generate huge numbers of Notifications (large-scale bulk modifications such as transformations by-pass the transaction framework anyways, I hope — right?)
I don't really know. There are many cases: - A resource is modified with transient objects, which don't need to be saved (e.g. open a read-only diagram from the workspace) - A resource is modified in-memory, but is read-only on the file system. This may or may not be a problem (This is a problem for e.g. *.uml resource, but not for the SashModel) - A resource has been modified by mistake (Shouldn't happen anymore with the read-only framework, unless the EditingDomain wasn't able to properly rollback the transaction... This may probably happen in some cases, for example when listeners on an EObject directly modify the model... they may be notified during the Rollback. I think we've had some cases like that) - There may be some cases where we don't know whether a resource has been modified or not. In these cases, we always save it, just to be sure, unless it is read-only (From a Permission or Discretion point of view). It seems safer to skip all read only resources during save: read only elements shouldn't be modified anyway, and if they are modified, the modifications shouldn't be important (Volatile and/or optional information), and shouldn't *need* to be saved. Implementing a true read-only mode was also a way to enforce this statement (Avoid modifications "by mistake"). > I need to think about it. Though a seemingly simple problem in concept, the details are too complex for my poor little brain. :-/ Agreed, this isn't simple :) IMO, the main idea is: - Some resources should be writable in memory, even if not saveable (SashModel) - Some read-only resources may be modified with transient/volatile elements (Notation), which should not be saved - Resources which are read-only, either from a Discretion or Permission reason, should never be saved, because they shouldn't contain anything which should need to be saved (A resource which has been made writable remains writable... Except maybe from a Permission point of view... But I think that in this case, the model set will warn the user and ask him to make them writable again)
(In reply to comment #8) > > However, it would still be nice to ensure that the page manager's transactions > only check the discretion axis, so I'll work on that. I can't do this with the plug-in dependencies as they are currently, because the ReadOnlyAxis type is not available to the oep.infra.core.sasheditor.di bundle.
(In reply to comment #11) > I don't really know. There are many cases: > > - A resource is modified with transient objects, which don't need to be saved > (e.g. open a read-only diagram from the workspace) Right. These canonical views use cases modify transient content in otherwise read-only transactions, so they're a special case anyways and don't consider any read-only state. > - A resource is modified in-memory, but is read-only on the file system. This > may or may not be a problem (This is a problem for e.g. *.uml resource, but not > for the SashModel) Yep. For this reason we need to be able to update the DiSashModelManager, TransactionalDiContentProvider, and TransactionalPageManagerImpl to add the read-only axis transaction option to their off-stack transactions. However, we can't until the dependency relationship between oep.infra.core and oep.infra.core.sasheditor.di is turned around (per comment 12). > - A resource has been modified by mistake (Shouldn't happen anymore with the > read-only framework, unless the EditingDomain wasn't able to properly rollback > the transaction... This may probably happen in some cases, for example when > listeners on an EObject directly modify the model... they may be notified during > the Rollback. I think we've had some cases like that) Well, that's a corrupt model and saving it is probably a bad idea anyways because that will just entrench the corruption. There can hardly be any defense against this, but avoiding save at least helps. > - There may be some cases where we don't know whether a resource has been > modified or not. In these cases, we always save it, just to be sure, unless it > is read-only (From a Permission or Discretion point of view). That sounds scary. I suppose an inappropriate use of unprotected transactions could cause this to happen, but that's a programming error! > It seems safer to skip all read only resources during save: read only elements > shouldn't be modified anyway, and if they are modified, the modifications > shouldn't be important (Volatile and/or optional information), and shouldn't > *need* to be saved. Implementing a true read-only mode was also a way to enforce > this statement (Avoid modifications "by mistake"). OK, so this is what the IModel implementations of saveModel() currently attempt to do. The only problem, as I see it, is that the SashModel will not detect that a *.di (workspace) or *.sash (metadata area) resource is read-only because the SashModelReadOnlyHandler will short-circuit with an Optional.of(false) (meaning writable) result. Is this an accurate assessment? I think that this sash-model problem can be addressed by separating the priority for each axis. Given the following, we will not think that a *.sash/*.di resource that is filesystem-read-only is writable: * prio(SashModelROHandler, DISCRETION) precedes prio(FSROHandler, DISCRETION) * prio(FSROHandler, PERMISSION) precedes prio(SashModelROHandler, PERMISSION) * querying read-only on multiple axes queries each handler in priority order on each axis separately and combines the result as a conjunction So, I'll sketch up further changes on this branch that let a read-only-handler extension declare a per-axis priority. For compatibility, the old priority attribute is still supported and applies to all axes if specified. Perhaps simpler, though, would be to let handlers declare an affinity with an axis and filter out handlers in each partition query that aren't applicable to the particular axis: * the DISCRETION pass will skip the FSROHandler, compute Optional.of(false) * the PERMISSION pass will skip the SashModelROHandler, compute Optional.of(true) * the conjunction is Optional.of(true), which is what we want
I have pushed a new commit to the branch to fix the problem with read-only state of sash model resources. Commit 6c21353: Add affinity with specified read-only axes to the extension metadata for read-only handlers and use this to partition the queries for read-only state by axis in the ReadOnlyManager. A new JUnit test demonstrates that this fixes the problem of read-only *.sash/*.di resources being mistaken as writable because of the SashModelReadOnlyHandler. Because this handler depends on the ModelSet, the JUnit test fixture is updated to use a ModelSet instead of a simple ResourceSet. Without the ReadOnlyManager changes, this test case fails. The FSReadOnlyHandler is updated to report read-only state of file: scheme URIs and the EMFReadOnlyHandler accepts these URIs in addition to platform:/resource URIs. This is necessary to ensure that sash models stored in the workspace metadata area now are not mistakenly reported as read-only by the EMFReadOnlyHandler (which would happen because the read-only queries are now partitioned by axis). All in all, a fairly complex situation, now, but it seems to work. And I don't see regressions in enablement of menu actions in the Model Explorer nor in various UI affordances and prompts to make resources writable, etc.
So, only this issue remains: > > However, it would still be nice to ensure that the page manager's transactions > > only check the discretion axis, so I'll work on that. > > I can't do this with the plug-in dependencies as they are currently, because the ReadOnlyAxis type is not available to the oep.infra.core.sasheditor.di bundle. It's too late to refactor APIs (M6 is today), but we can probably move the implementation of PageManager/DiContentProvider to infra.core. I will check that later. The branch can be merged to master (This needs to be done today anyway :) ) Thanks!
Thanks, Camille. I have pushed a merge of the bugs/429826-ro-axes branch to master. So, the glitch that we have now until the hangover mentioned in comment 15 is addressed is that if a *.sash model in the workspace metadata area happens to be unwritable, then it is not possible to open and close diagram editor tabs. In particular, clicking the X to close a tab simply has no effect. This isn't a severe problem because users shouldn't know about the files in the metadata area anyways to chmod 444 them.
