Community
Participate
Working Groups
Since the fix of bug 467539 (Mars.2 and Neon Mx), there is a regression for the Properties view. The analysis has been done from bug 467539 comment 16 to bug 467539 comment 18. To resume the steps to reproduce (with Sirius) are: * Launch a new runtime with product "org.eclipse.platform.ide" --> The default perspective is "Resource". * Close the Welcome page * Switch to "Modeling" perspective (from Sirius project) * Open the "Properties" view * Create a new java project * Switch to "Resource" perspective * Restart the workbench * Expand the java project * Select the java project * Switch to "Modeling" perspective * An error is logged in the "Error Log" view : "Could not acquire INavigatorContentService: Project Explorer not found." * After this problem, there is no title in the "Properties" view of "Resource" perspective. The problem is not the same if you replace the "Modeling" perspective with the "Java" perspective in the scenario. In case of "Java" perspective, the "Project" view is in the list of placeholders in WorkbenchPage.getViewReferences(boolean): > List<MPlaceholder> placeholders = modelService.findElements(window, null, MPlaceholder.class, null, scope); This is not the case with "Modeling" perspective. This explains why the problem is not reproductible with "Java" perspective.
(In reply to Laurent Redor from comment #0) > Since the fix of bug 467539 (Mars.2 and Neon Mx), there is a regression for > the Properties view. The analysis has been done from bug 467539 comment 16 > ... > * An error is logged in the "Error Log" view : "Could not acquire > INavigatorContentService: Project Explorer not found." This is the issue which is "uncovered" now because we have no NPE. > * After this problem, there is no title in the "Properties" view of > "Resource" perspective. This is interesting one. Laurent, are you sure that fix for bug 467539 *caused* the problem, not just uncovered another one? I mean, the fix https://git.eclipse.org/r/64171 is only avoiding NPE, not changing any other behavior. If you have the IDE right now: revert commit 98016ac5063fac8e347d1358d9544d1b1f60f0cc - you should have an NPE now, but is the *second* problem still there?
For information, Sirius is not mandatory to reproduce the problem. You can replace "Modeling" perspective by more commons perspectives: "Team Synchronizing" or "CVS Repository Exploring"
> If you have the IDE right now: revert commit > 98016ac5063fac8e347d1358d9544d1b1f60f0cc - you should have an NPE now, but > is the *second* problem still there? By reverting the commit 98016ac5, I have the below error and also a problem with the title in the "Properties" view of "Resource" perspective. org.eclipse.ui.views.properties.tabbed Error Thu Mar 10 10:28:39 CET 2016 Contributor org.eclipse.ui.navigator.ProjectExplorer cannot be created. org.eclipse.core.runtime.CoreException: Plug-in "org.eclipse.ui.navigator.resources" was unable to instantiate class "org.eclipse.ui.internal.navigator.resources.workbench.TabbedPropertySheetTitleProvider". at org.eclipse.core.internal.registry.osgi.RegistryStrategyOSGI.throwException(RegistryStrategyOSGI.java:194) ... at org.eclipse.equinox.launcher.Main.run(Main.java:1515) at org.eclipse.equinox.launcher.Main.main(Main.java:1488) Caused by: java.lang.NullPointerException at org.eclipse.ui.internal.navigator.resources.workbench.TabbedPropertySheetTitleProvider.<init>(TabbedPropertySheetTitleProvider.java:56) at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method) ... ... 134 more
(In reply to Laurent Redor from comment #3) > By reverting the commit 98016ac5, I have the below error and also a problem > with the title in the "Properties" view of "Resource" perspective. Which means that the regression is not coming from 98016ac5. Have you time/interest to bisect the problem and to find the offending commit? The question is: is this really Mars.2 regression, and not a bug existed in Mars.1 or even Luna already?
That is a quite amazing bug! The problem must be much older as 4.5.2, probably it was always there in e4, but I have no time to bisect this. First, the steps to reproduce are trivial: 1) Start with the perspective which contains Project Explorer but does not contain Properties view (Resources perspective). 2) Switch to the perspective which contains *visible* Properties view but does not contain Project Explorer => kaboom. Longer steps: 1) Open "Team Synchronizing" view and open Properties view. 2) Switch to the Resources view 3) Restart Eclipse 4) Switch to the "Team Synchronizing" view => kaboom. The problem is caused by the fact, that PartServiceImpl.getActivePart() (and so the WorkbenchPart.getActivePart()) during perspective switch return part from the "old" perspective, because the org.eclipse.e4.ui.workbench.renderers.swt.PerspectiveStackRenderer.showTab(MUIElement) first creates the UI for the "new" perspective and only after UI is created calls org.eclipse.e4.ui.workbench.modeling.EPartService.switchPerspective(MPerspective). So if the client code in the UI from the "new" perspective during part creation asks for a currently active part (to get the selection etc), it receives the part from a different perspective! So the Properties view asks via org.eclipse.ui.views.properties.PropertySheet.getBootstrapPart() on creation if there is an active part to populate the content. In one switches from Resources to Team perspective, the previously active part (Project Explorer) is returned, so the PropertySheet asks for the the adapter and tries to initialize the content. This leads to TabbedPropertySheetTitleProvider initialization, which calls window.getActivePage().findView(ProjectExplorer.VIEW_ID) and receives ... null, because findView() searches for views in the *current* (new) perspective, which does not contain project explorer. I have a patch. @Brian, Stefan, Tom: I have pretty limited or non existent e4 knowledge, can you please check if my conclusion / following patch are OK?
New Gerrit change created: https://git.eclipse.org/r/68181
Your analysis makes sense, but the fix seems rather grotty. We definitely need a test to verify the behaviour.
I think I have an easier and more localized fix.
New Gerrit change created: https://git.eclipse.org/r/68222
From the description above, I think it's appropriate to be returning the previously active part during the perspective switch. If we swap the active part before the perspective switch, it may not be fully initialized and activated at the time it's returned from getActivePart, so if other workbench elements try to interact with it, they'd be getting back an uninitialized part. It seems preferable to finish initializing the new perspective and then swap the active part. I know it's a bit old, but I think this doc is still relevant. See the "General Conventions" section, 5.1, here: https://eclipse.org/articles/Article-UI-Workbench/workbench.html Quote: "Some objects require several public methods to be called in a specific order before they are considered fully initialized. For example, to initialize an IViewPart, it is necessary to call the constructor, setInitializationData, init, and createPartControl before the part is considered initialized. Until all of the above have happened successfully, it must not be possible to reach the object through any API method. Keep in mind that the object may trigger other client code during its own initialization, so it should not even be possible for an object to find itself during construction." ...so the workbench APIs shouldn't be returning stuff from the new perspective until it's fully initialized. Instead of returning the new part earlier, couldn't the code that depends upon the active part listen to the appropriate change events so that they're aware of the fact that the active part changed when this happens at the end of the perspective switch?
Actually, I just looked at the patches and I see that it would return null during the perspective switch rather than returning the new part (so it won't have the problem of returning an uninitialized part). Sorry, I should have checked that first. However, the "return null during perspective switch" approach may have a different problem. A perspective switch might not actually change the active part, in which case we don't (and wouldn't want) to fire the change event indicating the part changed. However, with this approach, if the active part was unchanged then some code could check the active part during the perspective switch and discovered it to be null. Then there would be no follow-up event to indicate that the active part had been set to non-null and it would incorrectly believe that there is no active part. It seems plausible to me that the bug is in the of getActivePart() (in that it's not listening for change events) rather than in its implementation. Is there a reason to rule it out?
Wow. A lot of typos in that comment: > It seems plausible to me that the bug is in the of getActivePart() should have been: It seems plausible to me that the bug is in the CALLER of getActivePart()
(In reply to Stefan Xenos from comment #12) > Wow. A lot of typos in that comment: > > > It seems plausible to me that the bug is in the of getActivePart() > > should have been: > > It seems plausible to me that the bug is in the CALLER of getActivePart() The original problem wss reported for the Properties view, which, being created during perspective switch, checks which part is active right now. The code is absolutely valid. Current state is, that getActivePart API returns active part from a different perspective, and this cannot be OK or a client error.
Please test the following scenario: Like the properties view, another part is also asking for the active part during initialization... and it's being well behaved and is listening for events that would indicate that the active part has changed. Let's say it displays the name of the active part and updates that name whenever the active part changes. It seems to me that if such a part were created during a perspective switch, it would display the message "The active part is null" and never change that message since there would never be a change event telling it that the active part was no longer null. > The code is absolutely valid. I assume you're right and that I'm just missing something. :-) Why is it a problem for the view to receive a part which was active in the previous perspective as long as it receives a subsequent change event when the active part changes to point to something in the new perspective? If it's not too much trouble, could someone paste the stack trace?
(In reply to Stefan Xenos from comment #14) > Please test the following scenario: I will. > Why is it a problem for the view to receive a part which was active in the > previous perspective as long as it receives a subsequent change event when > the active part changes to point to something in the new perspective? Because if the receiver of the wrong part starts to perform some initialization based on a wrong assumption, we will see an error, see comment 5 with steps to reproduce. > If it's not too much trouble, could someone paste the stack trace? This is the stack from bug 467539.
(In reply to Stefan Xenos from comment #14) > Please test the following scenario: > > Like the properties view, another part is also asking for the active part > during initialization... and it's being well behaved and is listening for > events that would indicate that the active part has changed. > > Let's say it displays the name of the active part and updates that name > whenever the active part changes. > > It seems to me that if such a part were created during a perspective switch, > it would display the message "The active part is null" and never change that > message since there would never be a change event telling it that the active > part was no longer null. This is fortunately not the case, and this can be also seen with the Properties view, which first receives "null" during initialization but then receives the actual active part from the part change event flying during the perspective switch. So we are safe here. My first version of the patch and the patch from Brian both were ignoring the fact that for editors we shouldn't care about perspective at all. Also my first patch and the patch from Bnrian both add lot of overhead to the very often used code, so while initially I was preferring Brian's patch over my, after debugging it I think that will be a performance regression. So I've modified my original patch and borrowed the Brian's idea to remember the active perspective. https://git.eclipse.org/r/68181 > If it's not too much trouble, could someone paste the stack trace? Here is the fresh stack trace where the error is logged for a reproducer from comment 5: java.lang.Exception at org.eclipse.ui.internal.navigator.resources.workbench.TabbedPropertySheetTitleProvider.<init>(TabbedPropertySheetTitleProvider.java:58) at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method) at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62) at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45) at java.lang.reflect.Constructor.newInstance(Constructor.java:422) at java.lang.Class.newInstance(Class.java:442) at org.eclipse.core.internal.registry.osgi.RegistryStrategyOSGI.createExecutableExtension(RegistryStrategyOSGI.java:184) at org.eclipse.core.internal.registry.ExtensionRegistry.createExecutableExtension(ExtensionRegistry.java:905) at org.eclipse.core.internal.registry.ConfigurationElement.createExecutableExtension(ConfigurationElement.java:243) at org.eclipse.core.internal.registry.ConfigurationElementHandle.createExecutableExtension(ConfigurationElementHandle.java:55) at org.eclipse.ui.internal.views.properties.tabbed.view.TabbedPropertyRegistry.<init>(TabbedPropertyRegistry.java:129) at org.eclipse.ui.internal.views.properties.tabbed.view.TabbedPropertyRegistryFactory.createRegistry(TabbedPropertyRegistryFactory.java:74) at org.eclipse.ui.views.properties.tabbed.TabbedPropertySheetPage.initContributor(TabbedPropertySheetPage.java:405) at org.eclipse.ui.views.properties.tabbed.TabbedPropertySheetPage.<init>(TabbedPropertySheetPage.java:294) at org.eclipse.ui.views.properties.tabbed.TabbedPropertySheetPage.<init>(TabbedPropertySheetPage.java:273) at org.eclipse.ui.internal.navigator.resources.workbench.TabbedPropertySheetAdapterFactory.getAdapter(TabbedPropertySheetAdapterFactory.java:33) at org.eclipse.core.internal.adapter.AdapterFactoryProxy.getAdapter(AdapterFactoryProxy.java:82) at org.eclipse.core.internal.runtime.AdapterManager.getAdapter(AdapterManager.java:295) at org.eclipse.ui.part.WorkbenchPart.getAdapter(WorkbenchPart.java:143) at org.eclipse.ui.navigator.CommonNavigator.getAdapter(CommonNavigator.java:456) at org.eclipse.core.runtime.Adapters.adapt(Adapters.java:59) at org.eclipse.core.runtime.Adapters.adapt(Adapters.java:100) at org.eclipse.ui.views.properties.PropertySheet.doCreatePage(PropertySheet.java:205) at org.eclipse.ui.part.PageBookView.createPage(PageBookView.java:400) at org.eclipse.ui.part.PageBookView.partActivated(PageBookView.java:743) at org.eclipse.ui.views.properties.PropertySheet.partActivated(PropertySheet.java:361) at org.eclipse.ui.part.PageBookView.showBootstrapPart(PageBookView.java:926) at org.eclipse.ui.part.PageBookView.createPartControl(PageBookView.java:484) at org.eclipse.ui.views.properties.PropertySheet.createPartControl(PropertySheet.java:158) at org.eclipse.ui.internal.e4.compatibility.CompatibilityPart.createPartControl(CompatibilityPart.java:151) at org.eclipse.ui.internal.e4.compatibility.CompatibilityView.createPartControl(CompatibilityView.java:143) at org.eclipse.ui.internal.e4.compatibility.CompatibilityPart.create(CompatibilityPart.java:341) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.lang.reflect.Method.invoke(Method.java:497) at org.eclipse.e4.core.internal.di.MethodRequestor.execute(MethodRequestor.java:54) at org.eclipse.e4.core.internal.di.InjectorImpl.processAnnotated(InjectorImpl.java:966) at org.eclipse.e4.core.internal.di.InjectorImpl.processAnnotated(InjectorImpl.java:931) at org.eclipse.e4.core.internal.di.InjectorImpl.inject(InjectorImpl.java:151) at org.eclipse.e4.core.internal.di.InjectorImpl.internalMake(InjectorImpl.java:375) at org.eclipse.e4.core.internal.di.InjectorImpl.make(InjectorImpl.java:294) at org.eclipse.e4.core.contexts.ContextInjectionFactory.make(ContextInjectionFactory.java:162) at org.eclipse.e4.ui.internal.workbench.ReflectionContributionFactory.createFromBundle(ReflectionContributionFactory.java:105) at org.eclipse.e4.ui.internal.workbench.ReflectionContributionFactory.doCreate(ReflectionContributionFactory.java:74) at org.eclipse.e4.ui.internal.workbench.ReflectionContributionFactory.create(ReflectionContributionFactory.java:56) at org.eclipse.e4.ui.workbench.renderers.swt.ContributedPartRenderer.createWidget(ContributedPartRenderer.java:129) at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.createWidget(PartRenderingEngine.java:973) at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.safeCreateGui(PartRenderingEngine.java:649) at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine$1.run(PartRenderingEngine.java:534) at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:42) at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.createGui(PartRenderingEngine.java:518) at org.eclipse.e4.ui.workbench.renderers.swt.ElementReferenceRenderer.createWidget(ElementReferenceRenderer.java:70) at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.createWidget(PartRenderingEngine.java:973) at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.safeCreateGui(PartRenderingEngine.java:649) at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.safeCreateGui(PartRenderingEngine.java:755) at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.access$0(PartRenderingEngine.java:726) at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine$2.run(PartRenderingEngine.java:720) at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:42) at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.createGui(PartRenderingEngine.java:704) at org.eclipse.e4.ui.workbench.renderers.swt.StackRenderer.showTab(StackRenderer.java:1311) at org.eclipse.e4.ui.workbench.renderers.swt.LazyStackRenderer.postProcess(LazyStackRenderer.java:115) at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.safeCreateGui(PartRenderingEngine.java:667) at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.safeCreateGui(PartRenderingEngine.java:755) at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.access$0(PartRenderingEngine.java:726) at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine$2.run(PartRenderingEngine.java:720) at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:42) at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.createGui(PartRenderingEngine.java:704) at org.eclipse.e4.ui.workbench.renderers.swt.SWTPartRenderer.processContents(SWTPartRenderer.java:70) at org.eclipse.e4.ui.workbench.renderers.swt.SashRenderer.processContents(SashRenderer.java:142) at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.safeCreateGui(PartRenderingEngine.java:663) at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.safeCreateGui(PartRenderingEngine.java:755) at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.access$0(PartRenderingEngine.java:726) at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine$2.run(PartRenderingEngine.java:720) at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:42) at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.createGui(PartRenderingEngine.java:704) at org.eclipse.e4.ui.workbench.renderers.swt.SWTPartRenderer.processContents(SWTPartRenderer.java:70) at org.eclipse.e4.ui.workbench.renderers.swt.SashRenderer.processContents(SashRenderer.java:142) at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.safeCreateGui(PartRenderingEngine.java:663) at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.safeCreateGui(PartRenderingEngine.java:755) at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.access$0(PartRenderingEngine.java:726) at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine$2.run(PartRenderingEngine.java:720) at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:42) at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.createGui(PartRenderingEngine.java:704) at org.eclipse.e4.ui.workbench.renderers.swt.SWTPartRenderer.processContents(SWTPartRenderer.java:70) at org.eclipse.e4.ui.workbench.renderers.swt.PerspectiveRenderer.processContents(PerspectiveRenderer.java:49) at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.safeCreateGui(PartRenderingEngine.java:663) at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.safeCreateGui(PartRenderingEngine.java:755) at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.access$0(PartRenderingEngine.java:726) at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine$2.run(PartRenderingEngine.java:720) at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:42) at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.createGui(PartRenderingEngine.java:704) at org.eclipse.e4.ui.workbench.renderers.swt.PerspectiveStackRenderer.showTab(PerspectiveStackRenderer.java:82) at org.eclipse.e4.ui.workbench.renderers.swt.LazyStackRenderer$1.handleEvent(LazyStackRenderer.java:78) at org.eclipse.e4.ui.services.internal.events.UIEventHandler$1.run(UIEventHandler.java:40) at org.eclipse.swt.widgets.Synchronizer.syncExec(Synchronizer.java:233) at org.eclipse.ui.internal.UISynchronizer.syncExec(UISynchronizer.java:145) at org.eclipse.swt.widgets.Display.syncExec(Display.java:4773) at org.eclipse.e4.ui.internal.workbench.swt.E4Application$1.syncExec(E4Application.java:211) at org.eclipse.e4.ui.services.internal.events.UIEventHandler.handleEvent(UIEventHandler.java:36) at org.eclipse.equinox.internal.event.EventHandlerWrapper.handleEvent(EventHandlerWrapper.java:201) at org.eclipse.equinox.internal.event.EventHandlerTracker.dispatchEvent(EventHandlerTracker.java:197) at org.eclipse.equinox.internal.event.EventHandlerTracker.dispatchEvent(EventHandlerTracker.java:1) at org.eclipse.osgi.framework.eventmgr.EventManager.dispatchEvent(EventManager.java:230) at org.eclipse.osgi.framework.eventmgr.ListenerQueue.dispatchEventSynchronous(ListenerQueue.java:148) at org.eclipse.equinox.internal.event.EventAdminImpl.dispatchEvent(EventAdminImpl.java:135) at org.eclipse.equinox.internal.event.EventAdminImpl.sendEvent(EventAdminImpl.java:78) at org.eclipse.equinox.internal.event.EventComponent.sendEvent(EventComponent.java:39) at org.eclipse.e4.ui.services.internal.events.EventBroker.send(EventBroker.java:94) at org.eclipse.e4.ui.internal.workbench.UIEventPublisher.notifyChanged(UIEventPublisher.java:60) at org.eclipse.emf.common.notify.impl.BasicNotifierImpl.eNotify(BasicNotifierImpl.java:374) at org.eclipse.e4.ui.model.application.ui.advanced.impl.PerspectiveStackImpl.setSelectedElement(PerspectiveStackImpl.java:135) at org.eclipse.e4.ui.model.application.ui.advanced.impl.PerspectiveStackImpl.setSelectedElement(PerspectiveStackImpl.java:1) at org.eclipse.e4.ui.workbench.addons.perspectiveswitcher.PerspectiveSwitcher$12.widgetSelected(PerspectiveSwitcher.java:593) at org.eclipse.swt.widgets.TypedListener.handleEvent(TypedListener.java:249) at org.eclipse.swt.widgets.EventTable.sendEvent(EventTable.java:84) at org.eclipse.swt.widgets.Display.sendEvent(Display.java:4374) at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1081) at org.eclipse.swt.widgets.Display.runDeferredEvents(Display.java:4192) at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3780) at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine$4.run(PartRenderingEngine.java:1119) at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:336) at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.run(PartRenderingEngine.java:1020) at org.eclipse.e4.ui.internal.workbench.E4Workbench.createAndRunUI(E4Workbench.java:150) at org.eclipse.ui.internal.Workbench$5.run(Workbench.java:692) at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:336) at org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Workbench.java:605) at org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:148) at org.eclipse.ui.internal.ide.application.IDEApplication.start(IDEApplication.java:138) at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:196) at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:134) at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:104)
(In reply to Andrey Loskutov from comment #16) > My first version of the patch and the patch from Brian both were ignoring > the fact that for editors we shouldn't care about perspective at all. Also > my first patch and the patch from Bnrian both add lot of overhead to the > very often used code, so while initially I was preferring Brian's patch over > my, after debugging it I think that will be a performance regression. We can definitely improve the performance of ModelServiceImpl#getActivePerspective by checking for the common MWindow <—> MPerspectiveStack setup first. But you're right that this approach should better handle the situation where the same part remains active across the switch. > So I've modified my original patch and borrowed the Brian's idea to remember > the active perspective. > https://git.eclipse.org/r/68181 I don't like this approach for a number of reasons: - it's placing information into the model to communicate information about the rendering of the model; it doesn't feel right. - setting these selection flags in LazyStackRenderer, as opposed to PerspectiveStackRenderer, feels wrong - E4 shouldn't know anything about compatibility views
New Gerrit change created: https://git.eclipse.org/r/68294
(In reply to Brian de Alwis from comment #17) > > So I've modified my original patch and borrowed the Brian's idea to remember > > the active perspective. > > https://git.eclipse.org/r/68181 > > I don't like this approach for a number of reasons: > > - it's placing information into the model to communicate information about > the rendering of the model; it doesn't feel right. > - setting these selection flags in LazyStackRenderer, as opposed to > PerspectiveStackRenderer, feels wrong > - E4 shouldn't know anything about compatibility views Thanks for fast feedback! I've updated patch https://git.eclipse.org/r/68181 and I hope that I've addressed all your concerns there.
@Brian: do you think you can review it in M6 or should we set the target to M7?
I think we need to hold off to M7. Although your patch may fix the issue, I really don't like the approach: - It pushes implementation information into the model: the renderers are SWT implementation, but the tag and the EPartService are model. The JavaFX renderers may not have this problem. - It pulls in knowledge of expected behaviour from the Compat Layer ("View") which it should never do. - LazyStackRenderer has nothing to do with this issue and should not be involved. (In reply to Stefan Xenos from comment #12) > It seems plausible to me that the bug is in the CALLER of getActivePart() In this case, I don't think the bug is in the caller: the perspective *has* changed. This problem is being triggered from the notification that the MPerspectiveStack's selectedElement has changed. The crux of the issue here is that the window's PartServiceImpl is temporarily out-of-sync with the reality of the model state. It will be reconciled once PartServiceImpl#switchPerspective has completed. There are three approaches I think might work: - Change PerspectiveSwitcher to use EPartService#switchPerspective to trigger the perspective change. I suspect PartServiceImpl#switchPerspective will still have the same race condition, but we can fix null out the activePart until we finish recomputing it. - Fix my previous patch by having PartServiceImpl hold onto the MPerspectiveStack instance so that it can test the selectedElement directly. This should be very fast. - Cause the PerspectiveStackRenderer's event explicitly tell the PartServiceImpl that the perspective has changed.
Brian, how you think we can fix the issue you mention that e4 should not differentiate between view and editor parts? I see no solution for that, because views do not remain active after switching perspectives.
> because views do not remain active after switching perspectives If you have the outline view visible in two perspectives, you give it focus, and you switch perspectives, the outline view remains visible. Doesn't that mean views can remain active when switching perspectives? Or are you referring to something else?
(In reply to Stefan Xenos from comment #23) > > because views do not remain active after switching perspectives > > If you have the outline view visible in two perspectives, you give it focus, > and you switch perspectives, the outline view remains visible. Doesn't that > mean views can remain active when switching perspectives? Or are you > referring to something else? This happens because the target perspective includes the Outline. If it would not have the Outline view, some other view will be activated. So I meant basically there is no guarantee for views to remain active after switching perspectives.
(In reply to Andrey Loskutov from comment #22) > Brian, how you think we can fix the issue you mention that e4 should not > differentiate between view and editor parts? I see no solution for that, > because views do not remain active after switching perspectives. The editor area is shared across perspectives, so if an editor was active it should remain active. Right?
(In reply to Brian de Alwis from comment #25) > (In reply to Andrey Loskutov from comment #22) > > Brian, how you think we can fix the issue you mention that e4 should not > > differentiate between view and editor parts? I see no solution for that, > > because views do not remain active after switching perspectives. > > The editor area is shared across perspectives, so if an editor was active it > should remain active. Right? Yes.
No time for a better fix.
I have no time to working on this.
Gerrit change https://git.eclipse.org/r/68294 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=73ab782b53934e16aac318ce88435056fce9ee01
Probably related, see 414544 comment 8 for details Updates of the context are deferred while activating a new part. The active part in the PartService is derived from the context. So until the context is in sync again, the PartService will return the previous active part.
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet. As such, we're closing this bug. If you have further information on the current state of the bug, please add it and reopen this bug. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant. -- The automated Eclipse Genie.
