Community
Participate
Working Groups
This task acts as the top-level task for all 3-files model refactoring (Including work on the PageManager and ModelSet). The idea is to move the *.di file to the user preference space instead of being in the workspace, and to make it optional (Automatically created when absent). This *.di file introduces problems when users work on a shared model, and doesn't bring much value (It just contains the current editor's layout, which is user-specific, as well as an index of available pages, which could easily be computed dynamically). The main tasks will be: - Move the *.di resources out of the Resource Set. Opening/Closing editors will be done outside the Command Stack, and will not mark the model as dirty anymore. It will not be undoable either. - Improve the SashModel, to be able to load a Papyrus Model when the *.di file does not exist - Re-implement the PageManager#allPages() method to compute a dynamic result - Fix the PageManager to create pages in the right *.notation resources (When more than one option would make sense: controlled models, diagrams for imported libraries?) - Automatically load *.notation resources along with *.uml resources, when they are available (To make diagrams available as well) - Fix the hyperlinks/navigation for imported diagrams (And other features based on the PageManager) Expected benefits are: - Improve collaborative work, by not sharing user-specific layouts - Improve work with multi-model, by importing diagrams along with semantic models and enabling inter-models navigation - Improve the accuracy of the "is dirty" state
I thought I understood (had assumed, more likely) that the *.di resource was intended to be able to support other modeling languages than UML. For example, a Papyrus implementation of Ecore modeling, Entity-Relationship modeling, etc. would also employ *.di and *.notation resources with, say, *.ecore or *.er as the third leg in the role that the *.uml resource plays in UML models. Whether this understanding is or is not correct, how will a *.uml resource be distinguished as a "Papyrus Model" and not simply a UML resource? Will Papyrus henceforward implicitly (whether by accident or not) treat *.ecore resources, *.er, etc. as "Papyrus Models"? In particular, I think that the simple fact of a *.notation resource existing with the same base name as a *.uml is not sufficient to form an ad hoc pairing that is a "Papyrus Model". This has caused enough problems already, such as accidental data loss when deleting the *.notation resource deletes the *.uml also. The *.di has value at least in being a resource that ties the model and notation content together. Perhaps the problem is simply that it has content (such as open pages) that it shouldn't? What if the *.di is, instead of being eliminated, simply reduced to a list of pointers to the member resources (the *.notation, *.uml, *.ecore, etc.)?
There is currently no plan for supporting other languages. However, we may indeed keep the *.di file for further use, and just remove all its contents. > simply reduced to a list of pointers to the member resources (the *.notation, *.uml, *.ecore, etc.)? Sometimes, the resources are empty (e.g. a Model without any diagram). We may have nothing to point to.
(In reply to Camille Letavernier from comment #2) > > Sometimes, the resources are empty (e.g. a Model without any diagram). We > may have nothing to point to. Actually, I was thinking just of a list of relative URIs of the member resources, not EReferences to objects contained in those resources.
> Actually, I was thinking just of a list of relative URIs of the member resources, not EReferences to objects contained in those resources. Stored in a String[*] attribute? It would be dangerous; hard to maintain (e.g. with the "Save as" and other similar features). But well, that's an option. I wouldn't rely too much on it, though.
(In reply to Camille Letavernier from comment #4) > > Actually, I was thinking just of a list of relative URIs of the member resources, not EReferences to objects contained in those resources. > > Stored in a String[*] attribute? I would prefer an EDataType(instanceClassName=oee.util.URI), actually. > It would be dangerous; hard to maintain (e.g. with the "Save as" and other > similar features). But well, that's an option. I wouldn't rely too much on > it, though. Yes, more for refactoring participants to deal with, etc. Not ideal by any means. I just want to be sure that for 1.0 we find a robust answer to the question of what is, structurally speaking, a "Papyrus Model". It's our best chance to fix some of the problems that the current inference scheme has caused (if we believe those to be problems; I don't know).
Hi Christian, I've got a first version which works pretty much fine (at least until my last update of the read-only framework :) ). When I remove the *.di file and open the *.notation or *.uml file with the Papyrus editor: - The SashModel now loads or creates a *.sash resource from/in the preference store - If needed, this resource is populated with a default empty sash model - The page list is ignored and automatically computed (allPages()) Currently, I do not have a new *.di model, but it will be added later on. The main issue I have is related to the TransactionalEditingDomain. In a first version, I'd like this resource to belong to the TransactionalEditingDomain (i.e. we cannot modify it without a write transaction, we need a command, etc.) In the final version, it should be excluded from the transaction framework, or at least from the command framework (Especially, modifying the SashModel should not dirty the model, and opening/closing a diagram should not be undoable). I can jump directly to the final version if the first version is more complicated than the second. In the first version, I have one problem with the ReadOnlyHandlers: the EMFReadOnlyHandler considers that a workspace preference file is read-only. This results in a RollBackException when we try to open a diagram, or even create a default empty sashmodel. In the second version, we could create the sash resource in a different resource set, so that it is not handled by the TransactionalEditingDomain, and can bypass the Read-only framework. However, I'm not sure it would be a good idea to bypass the transaction framework. So, my questions are: - Should we use Unsafe transactions for modifying the SashModel inside the TransactionalEditingDomain? - Should we load the SashModel outside the TransactionalEditingDomain? - Any other suggestion? (In both cases, we may not even need to change the ReadOnlyHandlers) One important thing to note is that the Notation and UML models do not reference the SashModel. So, if the SashModel is loaded in a different resource set, it will not be loaded again in the Papyrus ModelSet. I think the two resource sets can live together.
(In reply to Camille Letavernier from comment #6) > Hi Christian, > > > I've got a first version which works pretty much fine (at least until my > last update of the read-only framework :) ). When I remove the *.di file and > open the *.notation or *.uml file with the Papyrus editor: Great! Could you please share it on a branch so that I may get to work on adapting the CDO integration layer? :-) > - The SashModel now loads or creates a *.sash resource from/in the > preference store > - If needed, this resource is populated with a default empty sash model > - The page list is ignored and automatically computed (allPages()) > > Currently, I do not have a new *.di model, but it will be added later on. Will it be required or optional? Will it live in the preferences area only? -------- 8< -------- > In the second version, we could create the sash resource in a different > resource set, so that it is not handled by the TransactionalEditingDomain, > and can bypass the Read-only framework. However, I'm not sure it would be a > good idea to bypass the transaction framework. Yeah, one of the features of the transaction API is the exclusive access to the resource set contents, to support concurrent applications. That applies as well to the sash model as to anything else, although in a system that by-and-large operates on the UI thread, Eclipse already provides a synchronization mechanism and I'm not sure how valuable the transactions are (though they are imposed by GMF anyways). > So, my questions are: > > - Should we use Unsafe transactions for modifying the SashModel inside the > TransactionalEditingDomain? Well, that would at least still provide the exclusive access for concurrency (unprotected transactions are still transactions and therefore exclusive). > - Should we load the SashModel outside the TransactionalEditingDomain? Considering that the only interactions with the sash model are for management of the UI, it can be expected that only the UI thread (or a modal context; same difference) will operate on it. This provides an equivalent exclusive access guarantee as the transactions, so it would seem a feasible approach. And easier to explain than a bunch of GMFUnsafe invocations, which would look odd, because that really is intended for canonical diagram refreshes. However, there is the question of what to do about the UML CacheAdapter. As you indicated, the UML and Notation content will not reference the SashModel content, so it will not cause the CacheAdapter to be propagated to the sash models. So, concurrent access to this shared adapter should not be an issue. But, maybe we want to attach the CacheAdapter to the SashModel to make inverse-reference queries more efficient (e.g., answering which page in the sash model references a particular Diagram view)? I doubt it would be necessary because the SashModel wouldn't be expected to attain any kind of large scale as the UML content would. But, after all that, it would be awkward to implement this as a separate resource set because it would have to resolve cross-references to the UML and Notation resources in a different resource set: it mustn't re-load this stuff locally, in the eventuality that a sash model is loaded from persistent storage (whether in the preferences are or the workspace). A command that operates on the sash model could answer false to canUndo(), which should prevent it being put on the stack, but would that (a) still dirty the editor and (b) flush the command history because previous commands would no longer be reachable for undo? I just cringe at the thought of so many unprotected transactions; perhaps what we need is a utility that executes normal write transactions (that can roll back etc. if necessary) but then we need the sash model resources to be recognized as writable (can be accomplished by a new ReadOnlyHandler, no need to update existing handlers). Does that help?
> Great! Could you please share it on a branch so that I may get to work on adapting the CDO integration layer? :-) I could, but I'm still experimenting things. It's nothing more than a proof-of-concept yet. It may be interesting as a base for discussion, but probably too early to integrate with CDO :) > Will it be required or optional? Will it live in the preferences area only? The sash model will be optional. To be more precise: - If the *.di exists and contains a SashModel, it is used (Legacy mode) - Otherwise (If the di doesn't exist, or if it exists but doesn't contain a SashModel), a new *.sash file is created in the user preference space As this is still a prototype, there is no option to force the creation of the SashModel inside the *.di file or in the *.sash file. > Yeah, one of the features of the transaction API is the exclusive access to the resource set contents, to support concurrent applications. That applies as well to the sash model as to anything else, although in a system that by-and-large operates on the UI thread, Eclipse already provides a synchronization mechanism and I'm not sure how valuable the transactions are (though they are imposed by GMF anyways). We're trying to not run everything in the UI Thread. Especially, modifying the SashModel is done through the PageManager, which is (in theory at least) thread-safe. Then, the IMultiDiagramEditor reads the contents, and jumps to the UI Thread to refresh the display when necessary. Bug 396483 tracks some efforts in this direction For example, it may be important for an automated script to be able to run in background, and still be able to open pages (e.g. automatic diagram generation, simulation...). We have a simulator with Model Animation (Moka) which interacts with the current model from another thread. It needs to open pages, highlight elements, ... From offline discussion, the easiest/safest way might be to use Internal write transactions without a Command (Or use the current transaction, if e.g. the page is opened in a command, as part of the creation of a diagram...). We'd need a helper similar to the GMFUnsafe helper, except that the transactions would not be "Unsafe" (i.e. they would not bypass the ReadOnlyManager)
I've pushed the branch bugs/429239-models I've also added a naive read-only handler for the SashModel (I may need to move it to another plug-in, it should probably be in an editor-related plug-in, but there are still dependency issues around infra.core and the editor) I'm not sure how I'm supposed to properly implement the SashModel read-only handler, because it should check two things: - If the model is a sash model (*.di, *.sash), it should not be read-only - EXCEPT when the underlying resource on the file system is read-only... But at the same time, it should be agnostic of the resource implementation (CDO, File system, ...). Well, I admit that this is a very specific use case which is unlikely to occur. But we may need to have two levels of read-only handlers: the "logical" read-only (Applied profiles, imported libraries... we could write them, but we don't want to), and the "physical" read-only (The resource is locked and we just can't write it: deployed plug-in, read-only file...). For the case of a sash model which is read-only on the file system, we may still want to edit it in memory (i.e. be able to open/close diagrams), but we'll just not save the changes. It should not prevent us from saving the rest of the model (Currently, it crashes, because the SashROHandler says that it is writable, whereas the physical FileSystem cannot actually write the file) Steps to reproduce: - Delete the *.di file from a Papyrus model (Otherwise it will be reused and the *.sash model will not be created) - Open the *.notation or *.uml file with the Papyrus Editor Core - Open a diagram - Save & close the model - Go to the workspace (.metadata/.plugins/oep.infra.core/path-to-your-model/model.sash) - Make the sash file read-only (From the OS filesystem) - Reopen the model - Close or open a new tab (It works because the SashModelROHandler says that the SashModel is writable) - Save: crash, because the resource cannot actually be written Beside this corner-case (I'm not sure whether it could happen in a more realistic situation), many things still need to be done: - Properly implement the IPageManager#allPages() method, based on contributors (i.e. a contributor for all diagrams, another contributor for all tables, ...) - Replace commands with internal transactions for the IPageManager - Re-implement wizards, so that they don't create a *.di file - Fix many bugs and many missing features :) This is just a prototype
(In reply to comment #9) > - Go to the workspace > (.metadata/.plugins/oep.infra.core/path-to-your-model/model.sash) > - Make the sash file read-only (From the OS filesystem) > - Reopen the model > - Close or open a new tab (It works because the SashModelROHandler says that the > SashModel is writable) > - Save: crash, because the resource cannot actually be written Interesting. I had already noticed that the FSReadOnlyHandler deliberately handles filesystem read-only state of workspace resources only, not external (file:// scheme, outside of the workspace) resources. But, with JavaSE 6, we have the necessary API available to improve this situation, to let the FSReadOnlyHandler change the write permissions of external files. Is that something we might consider? > Beside this corner-case (I'm not sure whether it could happen in a more > realistic situation), many things still need to be done: It is a foolish user that will mess around with write permissions on files buried in the dot-directories in the workspace metadata area. But, there are various virtual filesystems on which a workspace may be stored that could cause writability issues (and also horrible performance) so this corner case probably does warrant some consideration ... > - Properly implement the IPageManager#allPages() method, based on contributors > (i.e. a contributor for all diagrams, another contributor for all tables, ...) > - Replace commands with internal transactions for the IPageManager > - Re-implement wizards, so that they don't create a *.di file > > - Fix many bugs and many missing features :) This is just a prototype Anything that you may be looking to others to contribute? Considering that there are still API refactorings, UML 2.5, etc. on your plate ...
> Interesting. I had already noticed that the FSReadOnlyHandler deliberately handles filesystem read-only state of workspace resources only, not external (file:// scheme, outside of the workspace) resources. But, with JavaSE 6, we have the necessary API available to improve this situation, to let the FSReadOnlyHandler change the write permissions of external files. Is that something we might consider? I'm not sure this would be sufficient in this case. 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. > Anything that you may be looking to others to contribute? Considering that there are still API refactorings, UML 2.5, etc. on your plate ... Well, I still don't really know what the UML 2.5 migration implies, but I won't do everything on my own anyway. I'll see tomorrow what needs to be done and what I can do myself. Thanks :)
I wanted to check the tests before pushing the new commits, but... Well, most of them are failing now, which is expected, because they test things such as "Undo open page" (This is not a command anymore...), "Add page" (This is now ignored as the pages are computed dynamically) and so on... Well, this means that I'll need to update the tests and add new ones. ------- I've pushed the branch, which adds: - A TransactionHelper, for running Write transactions outside the CommandStack. If these transactions are executed within the CommandStack, they will be properly undone. - A ReadOnlyHandler for the SashModel (This is a naive implementation, which always returns true for *.di and *.sash resources, but as described in Comment 11, I'm not sure we can do much better with the current ReadOnlyManager) - A Transactional implementation of the IPageManager and DiContentProvider (ISashWindowsContentProvider) The DiModel and HistoryModel are currently disabled (Or partially disabled). The DiModel is reserved for future use of the *.di resources (Which is now empty, because the PageList has been removed and the SashWindowsMngr has been moved to the *.sash resource), and the HistoryModel needs to be a slave of the DiModel (Instead of the SashModel) There's a bit of logic in the SashModel to retrieve the right resource for the SashWindowsMngr element (Legacy = *.di file next to the *.notation and *.uml files, Current = *.sash file in the plugin preference store), which may need to be made public, within a helper, and maybe extensible as well (For CDO). Legacy-model test cases (The ones which start an editor from an existing set of di/notation/uml files) do not seem to have regressed. However, all tests which rely on the ModelSet#createModels and expect a *.di file to be created are now failing, as well as most of the SashEditor and editor.integration tests (Because all the specification has changed). Also, the IPageManager#allPages() method still has a temporary, hard-coded implementation, which does not return accurate results (And may fail in the "exotic" setup of test cases, i.e. page manager used without a resource and so on). Note that this branch also contains an old commit related to Bug 422247 which has not been pushed to master (Which was related to the resource management refactoring): Commit fbc1f0cd
(In reply to comment #12) Thanks, Camille. This is great. > I wanted to check the tests before pushing the new commits, but... Well, most of > them are failing now, which is expected, because they test things such as "Undo > open page" (This is not a command anymore...), "Add page" (This is now ignored > as the pages are computed dynamically) and so on... That's quite normal: the requirements have changed, so naturally the tests will have to adapt. > - A ReadOnlyHandler for the SashModel (This is a naive implementation, which > always returns true for *.di and *.sash resources, but as described in Comment > 11, I'm not sure we can do much better with the current ReadOnlyManager) I'll be happy to work on some new ReadOnlyManager API to try to sort this out. I agree that we now have conflicting "read-only" requirements that need to be resolved. Should I work on your branch or re-branch it? > There's a bit of logic in the SashModel to retrieve the right resource for the > SashWindowsMngr element (Legacy = *.di file next to the *.notation and *.uml > files, Current = *.sash file in the plugin preference store), which may need to > be made public, within a helper, and maybe extensible as well (For CDO). Yes, I have already discovered that this will need to be overridable for CDO. I'll work on this aspect, as well as the pluggable sash model storage.
I will create a new bit of metamodel for the *.di resource. Probably nothing more than a "PapyrusModel" empty EClass (Reserved for future use). This should fix the exceptions due to the empty *.di file when creating a new Papyrus model. I should also be able to restore the HistoryModel. I also need to implement contributions for the Pages (Diagrams, Tables, ...). These tasks should not conflict too much with the CDO integration and ReadOnlyManager refactoring. Then I will need to do a little bit of cleanup with all the references to DiModel and SashModel (Which used to have the same file extension). > Should I work on your branch or re-branch it? As you wish. You should probably start from this branch, at least, as it already provides an interesting use case. And I don't expect too many conflicts.
Many element contain references to the PageList (And bypass the IPageManager#allPages() method). As the PageList is not serialized anymore (For legacy models, it is still here, but it is not maintained anymore), all usages of this class will either throw NPE or fail. In general, I think it is a bad practice to use directly the di Ecore model. It was expected to be used through the ISashWindowsContentProvider (DiContentProvider) and IPageManager (PageManagerImpl). ------ In commit 698fd75, I've restored the DiModel and HistoryModel. If the *.di file doesn't exist, the model will still be loaded, and the resource will exist in memory. It will be serialized during save if an object is added to the *.di resource. I might do the same thing for NotationModel, so that we can open a pure UML file directly in the Papyrus editor and start creating diagrams for it. Currently, the notation resource is still mandatory (Although even an empty XMI notation resource will work) I've also made a minor change to the *.di metamodel: the SashWindowsManager#pageList now has a multiplicity of [0..1] instead of [1] (Well, that's not minor from an API point of view :) ). All references to PageList and SashWindowsManager#pageList have been deprecated in the sasheditor.di plug-in.
I'm currently fixing the existing tests. I will deprecate the ones which don't make sense anymore, and fix the ones for which the expected behavior has changed (Undo/Redo of PageManager, number of pages returned by allPages() when some existing pages are invalid...) The branch should be ready for merge today (As well as UML 2.5 which has been built on top of this branch: Bug 429744)
The branch bugs/429239-models has been merged to master. The tests have been updated to match the behavior refactoring (Especially for the PageManager and non-undoable transactions) I've also fixed the implementation of Papyrus Tables, which didn't properly adapt to IOpenable, and weren't considered as Pages.
I currently have the following exceptions when saving a model: java.io.FileNotFoundException: /metamodels/Ecore.metamodel.di at org.eclipse.osgi.storage.url.bundleresource.Handler.findBundleEntry(Handler.java:44) at org.eclipse.osgi.storage.url.BundleResourceHandler.openConnection(BundleResourceHandler.java:169) at java.net.URL.openConnection(Unknown Source) at org.eclipse.emf.ecore.resource.impl.URIHandlerImpl.createOutputStream(URIHandlerImpl.java:111) at org.eclipse.emf.ecore.resource.impl.ExtensibleURIConverterImpl.createOutputStream(ExtensibleURIConverterImpl.java:349) at org.eclipse.emf.ecore.resource.impl.ResourceImpl.save(ResourceImpl.java:996) at org.eclipse.papyrus.infra.core.resource.EMFLogicalModel.saveModel(EMFLogicalModel.java:58) at org.eclipse.papyrus.infra.core.resource.AbstractModelWithSharedResource.saveModel(AbstractModelWithSharedResource.java:137) java.io.FileNotFoundException: /metamodels/UML.metamodel.di at org.eclipse.osgi.storage.url.bundleresource.Handler.findBundleEntry(Handler.java:44) at org.eclipse.osgi.storage.url.BundleResourceHandler.openConnection(BundleResourceHandler.java:169) at java.net.URL.openConnection(Unknown Source) at org.eclipse.emf.ecore.resource.impl.URIHandlerImpl.createOutputStream(URIHandlerImpl.java:111) at org.eclipse.emf.ecore.resource.impl.ExtensibleURIConverterImpl.createOutputStream(ExtensibleURIConverterImpl.java:349) at org.eclipse.emf.ecore.resource.impl.ResourceImpl.save(ResourceImpl.java:996) at org.eclipse.papyrus.infra.core.resource.EMFLogicalModel.saveModel(EMFLogicalModel.java:58) at org.eclipse.papyrus.infra.core.resource.AbstractModelWithSharedResource.saveModel(AbstractModelWithSharedResource.java:137) ... I haven't investigated on the source of the issue yet (Why these resources are loaded at all). Until Bug 429826 is solved, we cannot consider these resources as read-only, and we'll try to save them. Of course, the actual issue is that they are loaded in the resource set, although they don't exist at all. But that's also an interesting case, where the ModelSet#save action should be more robust.
(In reply to comment #18) > I currently have the following exceptions when saving a model: I just rebased and now, when running the tests, I see the log spammed by this exception. This is new since yesterday's bugs/428239-models branch. Also, for me, all of the class diagram tests are dirtying the editor so that, when the test tries to close it, the flow is interrupted by the dialog prompting to save. Could that be related?
> Also, for me, all of the class diagram tests are dirtying the editor so that, when the test tries to close it, the flow is interrupted by the dialog prompting to save. Could that be related? Yes, most likely, because when I run the tests locally with a patched version of the save (To workaround the exception), the tests run properly. I will fix that ASAP.
The issue is related to the ProxyManager, which tries to load the *.di model and find the history, when it cannot find a specific EObject. I think this is related to the ControlMode, when an Object A can move from an URI something.uml#A to controlled.uml#A. The DI resource cannot be found, but it is still loaded, and then we try to save it (Because the SashModel ROHandler always says it is writable). So, we need to either fix the ProxyManager (There is an option for resources, which is "failed on load", and doesn't seem to work properly on this case), or rely on the Axis-based Read Only Manager to mark these missing resources as read-only (According to the PERMISSION axis). Merging the ro-axes branch might be sufficient, because (I think) we can't remove the erroneous resources from the resource set without introducing performances issues (Otherwise, the resource set will try to reload them many times). I will also try to see why Papyrus tries to save a resource which should be considered as "Failed on load".
The ModelUtils#resourceFailedOnLoad(Resource) method probably needs to be respecified/re-implemented. It's an old method which contains strong assumptions on the underlying resources/file system, and isn't properly documented. The Papyrus default serialization/deserialization options have changed since then, and I think that non-critical errors occurring during resource loading (e.g. missing package) are properly caught, and are not considered errors anymore. So, a naive implementation based on resource.getErrors().isEmpty() might be better. But we may miss some cases with that (When non-critical errors are being reported); except that I don't know which ones. From a few basic test cases, based on models in the FileSystem, we can expect at least: - IOExceptions (e.g. FileNotFoundException), when loading a file located outside the current workspace, and the file doesn't exist (e.g. *.di associated to a plug-in library) - XMIException when the XML is not valid (e.g. parse errors). - org.eclipse.core.internalresources.ResourceException, which was previously the only supported case (I guess that's because we couldn't save anything else than the workspace files, because the IModels used to only handle the 3 files + the controlled files) Any idea?
In commit e9eb2ae, I've pushed two changes to improve the save action: - The resourceFailedOnLoad() method has been improved to support all kinds of exception. If the resource failed on load, but some contents have been added to it (e.g. the file didn't exist, but we populated the resource), it can still be saved. Otherwise, it is ignored - The ModelSet/IModel #save methods now catch and log the exceptions, but don't break the global save action (To avoid half-saved models, often caused by a minor exception, such as trying to save a read-only library which hasn't even been modified)
(In reply to comment #21) > The issue is related to the ProxyManager, which tries to load the *.di model and > find the history, when it cannot find a specific EObject. I think this is > related to the ControlMode, when an Object A can move from an URI > something.uml#A to controlled.uml#A. Interesting. So, this is a latent problem now exposed by the changes to the UML metamodel that have now broken references? (In reply to comment #22) > It's an old method which contains strong assumptions on the underlying > resources/file system, and isn't properly documented. The Papyrus default > serialization/deserialization options have changed since then, and I think that > non-critical errors occurring during resource loading (e.g. missing package) are > properly caught, and are not considered errors anymore. So, a naive > implementation based on resource.getErrors().isEmpty() might be better. Indeed. Trying to analyze the viability of a resource that failed to load by looking into exceptions is probably not feasible. Especially because there is an unknown set of possible persistence domains involved. I think that anything that is not covered by the OPTION_RECORD_UNKNOWN_FEATURES load option should be considered a fatal error, and the model should just refuse to open. > But we may miss some cases with that (When non-critical errors are being > reported); except that I don't know which ones. Apart from unknown features, there are maybe a few other schema changes to account for, such as IllegalValueExceptions that may occur when constraints such as the values of an enumeration or the limits on a numeric range are changed. But, these cases are clearly distinguished by EMF and have nothing to do with persistence issues (such as file-not-found).
> Interesting. So, this is a latent problem now exposed by the changes to the UML metamodel that have now broken references? Maybe; I haven't checked into the details. I often have this exception with profiles (Because I've removed the projects from my workspace, so this is expected), but I haven't investigated why the Ecore Metamodel is failing as well. This happens for these 3 URIs (only, I think): pathmap://UML_METAMODELS/Ecore.metamodel.uml#EMap-_ownedTemplateSignature pathmap://UML_METAMODELS/Ecore.metamodel.uml#EMap-_ownedTemplateSignature-_ownedParameter.0 pathmap://UML_METAMODELS/Ecore.metamodel.uml#EMap-_ownedTemplateSignature-_ownedParameter.1 Because these URIs cannot be resolved, the ProxyManager tries some additional strategies, one of which implies loading the Ecore.metamodel.di, and this fails as well. And because this *.di resource is now present in the resource set, and because the "failedOnLoad" doesn't handle this case, and because the SashModelReadOnlyHandler specifies that *.di resources are always writable, and because this URI is actually not writable... The save fails. Anyway, fix from Bug 429826 and commit e9eb2ae from Comment 22 should both fix the issue.
In Bug 421411, it seems we have another corner case related to transactions. The explicit hyperlinks navigation is based on a Gef Request (OpenElementRequest), for which we return a standard Gef Command. This command is executed in the DiagramCommandStack. When this command opens an editor, it needs a transaction. As a result, the editor becomes dirty. Executing the same command without a transaction (e.g. hyperlink to a web page), or the same transaction without a command (e.g. dynamic hyperlink navigation with alt + hover) doesn't dirty the model. It seems that the model is dirty because the Operation has a ResourceUndoContext related to the Sash resource. I'm not sure how we should properly solve this case. - Is it the "isDirty" method which is not properly implemented? (I think the command would still be "Undoable", even if the actual Undo does nothing) - Should we remove the ResourceUndoContext when it is related to a sash/di resource? I'm not sure what the side effects would be... - Something else? Steps to reproduce: - Drop a (closed) Diagram on an element in a Diagram (To create a default hyperlink) - Double click on the element (To navigate the Hyperlink) Expected result: - The diagram is opened - The editor is not dirty - There is no new "Undo" command in the stack Current result: - The diagram is opened - The editor is dirty - There is a new "Undo" command in the stack (Note: the HyperLinkEditor class has not been updated to specify that it doesn't need a command anymore, but this doesn't make a difference, because the Transaction will be started in any case (Either by the TransactionalCommand, or by the TransactionalPageManager))
What is the status of this bugzilla? It targets M6, which has passed, and I think it is complete (or is there still more to do?). This blocks a CDO Integration bug for which there is no further work to do ...
Issue described Comment 26 is still not solved. It's related to the automatic management of the Dirty state by the DiagramCommandStack/OperationHistory. It's a minor issue from a usability point of view, but not so minor in terms of implementation I think. I'm not really familiar with the DiagramCommandStack, so I'm kind of stuck.
Except for subtasks Bug 394775 and Bug 422247, this task is complete. These subtasks are "related" more than "blocking", so I remove the explicit dependency and close this task
