Community
Participate
Working Groups
Steps to reproduce : - install EcoreTools from http://download.eclipse.org/modeling/emft/ecoretools/updates/milestones/2.0.0RC4a/luna - restart Eclipse making sure nothing from Sirius is loaded (you should not be in the modeling perspective or have a View from Sirius opened), the bug has been reproduced successfully with the Java perspective - import a project with a Ecore file (importing ExtLibrary from the examples with the New-> wizard for instance) - right click on the .ecore file, "Initialize Ecore Diagram" - go through the steps to create a new representation result : nothing happens and you have a error which is logged in the error log view. expected result : the representation is opened The bug has not been reproduced on Linux systems so far. NPEs are coming from : Caused by: java.lang.NullPointerException at org.eclipse.sirius.ui.tools.internal.actions.creation.CreateRepresentationAction$1.run(CreateRepresentationAction.java:126) at org.eclipse.jface.operation.ModalContext$ModalContextThread.run(ModalContext.java:122) Looking at the code, this action expect an EditingSession to be here but none has been created yet. This is probably a side effect of the lazyness recently introduced. Workaround in EcoreTools is to explicitely call IEditingSession uiSession = SessionUIManager.INSTANCE .getOrCreateUISession(existingSession); uiSession.open(); Before reusing the CreateRepresentationAction.
the EditingSession creation used to be done by org.eclipse.sirius.ui.tools.internal.views.common.navigator.SiriusCommonContentProvider.createAndOpenUiSession(Session) which, in that case, has not been used and as such not loaded, it did not register the CommonSessionManagerListener which would have triggered the EditingSession creation and opening.
Issue reproduced with the steps to reproduce [1] of Cédric [1] https://bugs.eclipse.org/bugs/show_bug.cgi?id=437251#c0
A simple fix in Sirius is to call IEditingSession editingSession = SessionUIManager.INSTANCE.getOrCreateUISession(session); instead of IEditingSession editingSession = SessionUIManager.INSTANCE.getUISession(session); in org.eclipse.sirius.ui.tools.internal.actions.creation.CreateRepresentationAction.run(). This problem reflects the known problem of dependency between UI and non UI plugins mentioned in Modularization page [1]. It will be a good candidate to start this refactoring but with a wider scope than this bug. [1] http://wiki.eclipse.org/Sirius/Modularization#Modularity_Criteria
Well, I don't agree. There are many SessionUIManager.INSTANCE.getUISession calls in the Sirius codebase itself, so fixing this particular example doesn't mean we fix all of them. I would say the problem is more that a critical part of the EditingSession : its initialization, right now is coupled with the CommonSessionManagerListener which is only installed if the content provider has been called before. Either we make getUISession() automatically create and open a non existing session, or we make sure the ui sessions are created before without relying on the content provider for the Package explorer.
> Well, I don't agree. There are many SessionUIManager.INSTANCE.getUISession calls in the Sirius codebase itself, so fixing this particular example doesn't mean we fix all of them. Yes > getUISession() automatically create and open a non existing session This is unsafe because some calls to this method do not expect that it create the UISession if it does not exist. This bug is linked to the session lifecycle, a problem difficult to solve plainly...
What about registering CommonSessionManagerListener (well, another class actually which only take care about the editing session initialization) through the dedicated extension point ?
(In reply to Cedric Brun from comment #6) > What about registering CommonSessionManagerListener (well, another class > actually which only take care about the editing session initialization) > through the dedicated extension point ? Wouldn't this trigger the loading and initialization of most/all Sirius as soon as you open a Project Explorer (or any CNF-based view)? Or maybe I misunderstood which extension point you refer to.
(In reply to Laurent Redor from comment #5) > > Well, I don't agree. There are many SessionUIManager.INSTANCE.getUISession calls in the Sirius codebase itself, so fixing this particular example doesn't mean we fix all of them. > > Yes > > > getUISession() automatically create and open a non existing session > > This is unsafe because some calls to this method do not expect that it > create the UISession if it does not exist. > Did you find a single one example were the calling code expect a null reference ? I could not, and that makes sense because before it was pretty much the first thing Sirius did, so the "null" case did not happened.
(In reply to Pierre-Charles David from comment #7) > (In reply to Cedric Brun from comment #6) > > What about registering CommonSessionManagerListener (well, another class > > actually which only take care about the editing session initialization) > > through the dedicated extension point ? > > Wouldn't this trigger the loading and initialization of most/all Sirius as > soon as you open a Project Explorer (or any CNF-based view)? Or maybe I > misunderstood which extension point you refer to. I might have been not precise enough. Right now CommonSessionManagerListener do some UI stuffs *and* will create an EditingSession once a Session is added in the SessionManager. This listener is registered by the content provider, problem is all the Sirius code expect the EditingSession to be there and initialized, and now we have no guarantee the content provider is going to be called. My proposal was to have a specific sessionmanagerlistener just doing the EditingSession creation, and registering it through the extension point. Then we are sure it will be called as soon as a session is added in the SessionManager. But again, I'm not convinced we *really* need such a convoluted things, but that would be the closest we could have to the current behavior. In the end the EditingSession was created at first only here to keep track of the link between editors and their Session instance, EditingSession might not even need to be statefull like they are right not. Granted, it evolved in something several responsabilities : keepint track of the editors + their session and "unifying" the behavior of the editors to have a global save & close.
For 1.0.1, we could make the simple fix in CreateRepresentationAcion as proposed by Laurent. For 2.0.0, we could look into the solution proposed by Cedric: a specific SessionManagerListener. But note that SessionUIManagerImpl is already a SessionManagerListener. So we could move the ui session creation logic from CommonSessionManagerListener to SessionUIManagerImpl it. We should not forget to init all ui sessions corresponding to the already opened session during the initalization of the SessionUIManagerImpl. Then this initialization would be done at the first call of SessionUIManager.INSTANCE. If an sooner initalization is required, we could do it during Sirius UI plugin start (in the activator for example). (Eventually with this logic, we could remove the getOrCreateUISession().)
(In reply to Maxime Porhel from comment #10) > For 1.0.1, we could make the simple fix in CreateRepresentationAcion as > proposed by Laurent. > And at least in all the other calls from Sirius itself which might not expect a null there. But the "least impacting compared to the previous Sirius release" change is what I proposed with the SessionManagerListener. > For 2.0.0, we could look into the solution proposed by Cedric: a specific > SessionManagerListener. But note that SessionUIManagerImpl is already a > SessionManagerListener. So we could move the ui session creation logic from > CommonSessionManagerListener to SessionUIManagerImpl it. We should not > forget to init all ui sessions corresponding to the already opened session > during the initalization of the SessionUIManagerImpl. Then this > initialization would be done at the first call of SessionUIManager.INSTANCE. > If an sooner initalization is required, we could do it during Sirius UI > plugin start (in the activator for example). > For 2.0 I would prefer to get rid of this altogether or at least make sure we don't really need to keep it in sync with Session instances through a specific sessionmanager listener. > (Eventually with this logic, we could remove the getOrCreateUISession().)
Simple fix / workaround proposition: https://git.eclipse.org/r/#/c/29987/ (v1.0.x) Complete fix https://git.eclipse.org/r/29988 (master) The complete fix seems safe enough to me to be integrated in v1.0.x instead of the workaround.
As proposed by Cedric, we could also replace the implementation of the getUISession(Session) by getOrCreateUISession(Session) (so we would not need a session manager listener to open the UI session when a session is opened). The UI session are currently created "behind the scene" and "as soon as possible" (when the Model Explorer View's content provider has been initialized, this becoming when the SessionUIManager has been initalized with my second proposed patch). But with Cedric proposal, it would be created on demand. For the v1.0.x branch this would change the behavior of our current implementation of SessionUIManager.getUISession(Session) (concrete call: SessionUIManager.INSTANCE.getUISession(Session)) so I would prefer the listener solution on branch v1.0.x For the master branch, we should (as proposed by Cedric): > to get rid of this altogether or at least make sure we don't really need to keep > it in sync with Session instances through a specific sessionmanager listener. ie. try to know if SessionUIManager is the good answer to all aspects managed by the IEditingSession (refresh filter, editors which have to explicitly call attach/detach, why RestoreToSavePointListener is installed only when an editor is opened? ...)
See https://git.eclipse.org/r/30411 for the editing session initalization from the SessionUIManagerImpl.
Corrected on 1.0.1 by commit a0edcdef6339577979c27554da8da03af7247723
Available in Sirius 1.0.1.
